MessageCache cloning of parser breaks stuff if it happens while in the process of parsing something
Closed, ResolvedPublic

Description

stack trace

When a commons user went to edit a section of [[Commons:COM:SCOPE]] they got a fatal exception (Actual exception text courtesy of Reedy. Stack trace attached) :

2013-10-27 14:18:21 mw1050 commonswiki: [4672fc92] /w/index.php?title=Commons:Project_scope&action=edit&section=13 Exception from line 77 of /usr/local/apache/common-local/php-1.22wmf22/includes/parser/StripState.php: Invalid marker:UNIQ7fcfb94d3c323dfe-h-0--QIN

(Presumably somewhere along the exception logging code, the \x7F actually gets treated as a backspace, removing the leading space and the trailing U).

Give that \x7fUNIQ7fcfb94d3c323dfe-h-0--QINU\x7f is a valid strip marker (provided that 7fcfb94d3c323dfe is the correct magic number), this raises somewhat of a mystery. Suggests that maybe something is calling $wgParser->parse somewhere inbetween from a hook or something.

Although everyone seems ready to blame translate, filing in mediawiki as translate doesn't actually appear in the stack trace, so not a lot of evidence yet.

(as an aside, this can be reproduced by putting the exact text from the stack trace into special:expandtemplates. Thus far I have not been able to reproduce locally. Still investigating)

Relavent links on wiki: https://commons.wikimedia.org/wiki/Commons:Village_pump#MediaWiki_error_message_when_editing_Commons:Project_scope ( https://commons.wikimedia.org/w/index.php?title=Commons:Village_pump&oldid=108125046 )


Version: 1.22.0
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=65826

Attached: file_56226.txt

bzimport added a subscriber: wikibugs-l.
bzimport set Reference to bz56226.
Bawolff created this task.Via LegacyOct 27 2013, 7:53 PM
Bawolff added a comment.Via ConduitOct 27 2013, 8:41 PM

More minimal test case:

Put the following in Special:Expandtemplates on Commons:

<nowiki/>
{{ns:0}}
<translate>

The fact the translate tag seems to need to be there, causes me to think translate is to blame. The <nowiki/> seems to be replacable by any parser tag, the {{ns:0}} by any template or parserfunction.

Bawolff added a comment.Via ConduitOct 27 2013, 9:24 PM

Created attachment 13597
patch that seems to fix issue. Make messageCache not clone $wgParser

Issue appears to stem from how MessageCache clones its parser, combined with an exception from translate being the first thing that messageCache transforms, happening during parsing

Here's the backtrace from the point where the uniqPrefix of the parser seems to get changed:

<ul>
<li>Parser.php line 323 calls wfBacktrace()</li>
<li>Parser.php line 4837 calls Parser->clearState()</li>
<li>Parser.php line 628 calls Parser->startParse()</li>
<li>Parser.php line 4864 calls Parser->preprocess()</li>
<li>MessageCache.php line 982 calls Parser->transformMsg()</li>
<li>Message.php line 854 calls MessageCache->transform()</li>
<li>Message.php line 592 calls Message->transformText()</li>
<li>Message.php line 649 calls Message->toString()</li>

<li>TPException.php line 25 calls Message->text()</li>
<li>TranslatablePage.php line 291 calls TPException->__construct()</li>
<li>PageTranslationHooks.php line 32 calls TranslatablePage->getParse()</li>
<li>- line - calls PageTranslationHooks::renderTagPage()</li>
<li>Hooks.php line 199 calls call_user_func_array()</li>
<li>GlobalFunctions.php line 3877 calls Hooks::run()</li>
<li>Parser.php line 405 calls wfRunHooks()</li>
<li>WikitextContent.php line 300 calls Parser->parse()</li>
<li>EditPage.php line 3223 calls WikitextContent->getParserOutput()</li>

<li>EditPage.php line 2169 calls EditPage->getPreviewText()</li>
<li>EditPage.php line 441 calls EditPage->showEditForm()</li>
<li>EditAction.php line 50 calls EditPage->edit()</li>
<li>EditAction.php line 76 calls EditAction->show()</li>
<li>Wiki.php line 448 calls SubmitAction->show()</li>
<li>Wiki.php line 312 calls MediaWiki->performAction()</li>
<li>Wiki.php line 603 calls MediaWiki->performRequest()</li>
<li>Wiki.php line 467 calls MediaWiki->main()</li>
<li>index.php line 49 calls MediaWiki->run()</li>

</ul>

If I change MessageCache to create a new instance of parser instead of cloning, issue goes away. So something funky with how cloning works perhaps?

Attached: file_56226.txt

Bawolff added a comment.Via ConduitOct 27 2013, 9:26 PM

Changing component again, this time to i18n.

Doesn't appear that translate specific, translate just manages to trigger an edge case.

Bawolff added a comment.Via ConduitOct 27 2013, 9:54 PM

Ok, problem is:

if you call clone during parsing, its a shallow clone, so $this->mPreprocessor->parser points to the wrong instance of parser, so the strip item uniq prefix that gets used is wrong.

gerritbot added a comment.Via ConduitOct 27 2013, 10:02 PM

Change 92253 had a related patch set uploaded by Brian Wolff:
Update Parser::mPreprocessor->parser reference when Parser is cloned

https://gerrit.wikimedia.org/r/92253

Bawolff added a comment.Via ConduitOct 27 2013, 10:14 PM
  • Bug 44608 has been marked as a duplicate of this bug. ***
Bawolff added a comment.Via ConduitOct 28 2013, 5:45 PM

(In reply to comment #4)

Ok, problem is:

if you call clone during parsing, its a shallow clone, so
$this->mPreprocessor->parser points to the wrong instance of parser, so the
strip item uniq prefix that gets used is wrong.

Hmm, I think I jumped to a conclusion a little to fast. (The patch I tried did something wrong which coincidentally made my test case work).

What I now think is happening - It appears that replacing the mStripState variable causes it to be replaced on both instances of the parser instead of just the current instance (As if it was a reference to a reference, and you were replacing the wrong layer of indirection)

Bawolff added a comment.Via ConduitOct 28 2013, 8:42 PM

(In reply to comment #7)

(In reply to comment #4)
> Ok, problem is:
>
> if you call clone during parsing, its a shallow clone, so
> $this->mPreprocessor->parser points to the wrong instance of parser, so the
> strip item uniq prefix that gets used is wrong.

Hmm, I think I jumped to a conclusion a little to fast. (The patch I tried
did
something wrong which coincidentally made my test case work).

What I now think is happening - It appears that replacing the mStripState
variable causes it to be replaced on both instances of the parser instead of
just the current instance (As if it was a reference to a reference, and you
were replacing the wrong layer of indirection)

I have no idea how or why, but updated patch which stops issue occurring by doing something that really should have no affect whatsoever. Specificly it does:

$foo = new StripState( $stuff );
$this->mStripState =& $foo;

as opposed to straight assigning StripState to foo. What the difference is, I honestly have no idea.

Seems like some sort of nasty php bug related to cloning object.

Bawolff added a comment.Via ConduitNov 7 2013, 12:40 AM

Created attachment 13717
minimal test case

Here's a simple test script to demonstrate the bug.

When I run it, it gives the following output:

Prior to changing value, the original object is supposed to be 'default value'. Actually is:
default value
Prior to changing value, the cloned object is supposed to be 'default value'. Actually is:
default value

After changing value, the original object is supposed to be 'default value'. Actually is:
Some other value
After changing value, the cloned object is supposed to be 'some other value'. Actually is:
Some other value


However, the second last value print out should be different.

Attached: test.php

Bawolff added a comment.Via ConduitNov 7 2013, 2:58 AM

Now that there is a more minimal test case, I attempted to file a php bug - https://bugs.php.net/bug.php?id=66040 (Bugzilla appearently doesn't let me include php bugs in the see also field)

The minimal test case also suggests a different approach, just remove the & operator from the arguments to those hooks. There's no real need for them, and I'm assuming they are a hold over from PHP4 days.

Bawolff added a comment.Via ConduitNov 7 2013, 3:41 AM

The minimal test case also suggests a different approach, just remove the &
operator from the arguments to those hooks. There's no real need for them,
and
I'm assuming they are a hold over from PHP4 days.

Hmm, I thought that wouldn't result in a behaviour change, but apparently that causes exceptions to be thrown about how the function wants an object (in for example cite).

Perhaps just removing the reference for the &$this->mStripState would fix it with less breakage, since most people subsribing to those hooks don't include that argument in the signature... I'm not really sure.

Or could do that other hacky thing of making an extra reference when doing the assignment to strip state. Not sure.

Mean time, maybe just change Translate to not trigger it.

gerritbot added a comment.Via ConduitNov 7 2013, 4:07 AM

Change 94098 had a related patch set uploaded by Brian Wolff:
Do not transform message of TPException as work around for php bug

https://gerrit.wikimedia.org/r/94098

Bawolff added a comment.Via ConduitNov 7 2013, 4:10 AM

(In reply to comment #12)

Change 94098 had a related patch set uploaded by Brian Wolff:
Do not transform message of TPException as work around for php bug

https://gerrit.wikimedia.org/r/94098

Still not sure what the cleanest way to fix this in core is. However in the meantime while figuring that out, lets fix it in the extension - since it all comes down to transforming a message that is essentially never used, and if it was used probably wouldn't be shown to the user (depending on your wiki's configuration). Thus might as well just plain output instead of running transform from the ParserBeforeStrip hook (Especially because text() and plain() would probably return the same result anyhow).

gerritbot added a comment.Via ConduitNov 7 2013, 9:58 AM

Change 94098 merged by jenkins-bot:
Do not transform message of TPException as work around for php bug

https://gerrit.wikimedia.org/r/94098

Aklapper added a comment.Via ConduitNov 19 2013, 4:53 PM

Patch merged; leaving open as per comment 13.

Nemo_bis added a comment.Via ConduitJul 2 2014, 3:20 PM

(In reply to Bawolff (Brian Wolff) from comment #1)

More minimal test case:

Put the following in Special:Expandtemplates on Commons:

<nowiki/>
{{ns:0}}
<translate>

I don't see anything bad when I do this, now. Was this fixed by the fix for bug 65826 (https://gerrit.wikimedia.org/r/141056 )?

Bawolff added a comment.Via ConduitJul 2 2014, 3:31 PM

No, that patch is unrelated. The patch in comment 12 fixed the test case. The larger issue is still an issue.

Nemo_bis added a comment.Via ConduitJul 2 2014, 3:36 PM

(In reply to Bawolff (Brian Wolff) from comment #17)

The larger issue is still an issue.

Ah, sorry. So this bug can be reproduced with comment 9 / attachment 13717?

Bawolff added a comment.Via ConduitJul 2 2014, 9:16 PM

(In reply to Nemo from comment #18)

(In reply to Bawolff (Brian Wolff) from comment #17)
> The larger issue is still an issue.

Ah, sorry. So this bug can be reproduced with comment 9 / attachment 13717
[details]?

That's a testcase for the underlying php bug.

I guess a minimal MW test case could be:

$wgHooks['ParserBeforeStrip'][] = 'wfTest56226';
$wgExtensionFunctions[] = 'wfForceParse';

function wfTest56226( $parser, &$text, $state ) {

wfMessage( 'tagline' )->parse();

}

function wfForceParse() {

// May or may not be needed. In order to make sure that
// a $wgParse->parse happens before Message cache is initialized.
// Otherwise whether or not bug shows up may depend on load order/extensions installed/etc
$wgParser->parse( '==foo==', Title::makeTitle( NS_MAIN, 'bar' ), new ParserOptions() );

}

[Put that in LocalSettings.php, See if you get an "Inavlid strip marker" exception]

Anomie added a comment.Via ConduitSep 22 2014, 2:45 PM
  • Bug 71110 has been marked as a duplicate of this bug. ***
Anomie added a comment.Via ConduitSep 22 2014, 5:43 PM

I've figured out a minimal test case for this:

$object = new stdclass;
$object->v = 1;

$foo = &$object->v;

$myObj = clone $object;
$myObj->v = 2;

echo "object->v is {$object->v}\n";

What's going on here is that the existence of the reference in $foo is making $object->v *also* be considered a reference. And so the clone is copying the reference as a reference, so the later assignment overwrites the original.

I don't think the PHP developers would consider this a bug, given the explanation at http://php.net/manual/en/language.references.whatdo.php#language.references.whatdo.assign that basically says there's no "from" and "to" when it comes to PHP references.

(In reply to Bawolff (Brian Wolff) from comment #8)

I have no idea how or why, but updated patch which stops issue occurring by
doing something that really should have no affect whatsoever. Specificly it
does:

$foo = new StripState( $stuff );
$this->mStripState =& $foo;

The difference between that and direct assignment is that that rebinds $this->mStripState as a reference to $foo, rather than assigning a new value to the existing reference.

gerritbot added a comment.Via ConduitSep 22 2014, 5:44 PM

Change 161994 had a related patch set uploaded by Anomie:
Break accidental references in Parser::__clone

https://gerrit.wikimedia.org/r/161994

gerritbot added a comment.Via ConduitSep 22 2014, 10:53 PM

Change 161994 merged by jenkins-bot:
Break accidental references in Parser::__clone

https://gerrit.wikimedia.org/r/161994

Anomie added a comment.Via ConduitSep 23 2014, 3:19 PM

This should be fixed now. The fix should be deployed to WMF wikis with 1.24wmf23 or 1.25wmf1 (depending on what we decide to call it), see https://www.mediawiki.org/wiki/MediaWiki_1.24/Roadmap or https://www.mediawiki.org/wiki/MediaWiki_1.25/Roadmap for the schedule.

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.