Last modified: 2014-09-23 17:35:56 UTC
Created attachment 13596 [details] 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§ion=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 )
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.
Created attachment 13597 [details] 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?
Changing component again, this time to i18n. Doesn't appear that translate specific, translate just manages to trigger an edge case.
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.
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
*** Bug 44608 has been marked as a duplicate of this bug. ***
(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)
(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.
Created attachment 13717 [details] 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.
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.
> > 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.
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
(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).
Change 94098 merged by jenkins-bot: Do not transform message of TPException as work around for php bug https://gerrit.wikimedia.org/r/94098
Patch merged; leaving open as per comment 13.
(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 )?
No, that patch is unrelated. The patch in comment 12 fixed the test case. The larger issue is still an issue.
(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]?
(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] > [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]
*** Bug 71110 has been marked as a duplicate of this bug. ***
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.
Change 161994 had a related patch set uploaded by Anomie: Break accidental references in Parser::__clone https://gerrit.wikimedia.org/r/161994
Change 161994 merged by jenkins-bot: Break accidental references in Parser::__clone https://gerrit.wikimedia.org/r/161994
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.
