Page MenuHomePhabricator

Incorrect TOC and section edit links rendering in Vector due to ParserCache corruption via ParserOutput::setText( ParserOutput::getText() )
Closed, ResolvedPublic

Description

After upgrading to 1.27.0-wmf11 at least on cswiktionary different HTML (according to folks on IRC it is mobile) at least of headings is being delivered.

Randomly, no pattern found yet.

Servers which delivered the incorrect content so far:

  • mw1257
  • mw1109
  • mw1103

First occurence spotted cca 2016-01-21 19:40 UTC.

Delivered HTML for headline:

<h2><span class="mw-headline" id=".C4.8De.C5.A1tina">čeština</span><span><a href="#/editor/1" title="Editovat sekci: čeština" data-section="1" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page icon-32px">Editovat</a></span></h2>

Expected HTML for headline:

<h2><span class="mw-headline" id=".C4.8De.C5.A1tina">čeština</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/w/index.php?title=leetspeak&amp;action=edit&amp;section=1" title="Editovat sekci: čeština">editovat</a><span class="mw-editsection-bracket">]</span></span></h2>

The delivered HTML causes sections unable to be edited, since clicking on the edit link triggers no action (and goes nowhere, since it is a page fragment URL).


Further issues description in {T124356#2050230} and {T124356#2050285}

Details

SubjectRepoBranchLines +/-
operations/mediawiki-configmaster+0 -1
mediawiki/tools/releasemaster+4 -1
mediawiki/extensions/MobileFrontendwmf/1.27.0-wmf.16+22 -2
mediawiki/extensions/MobileFrontendwmf/1.27.0-wmf.15+23 -2
mediawiki/extensions/MobileFrontendmaster+22 -2
mediawiki/extensions/MobileFrontendmaster+7 -1
mediawiki/tools/releasemaster+1 -0
mediawiki/corewmf/1.27.0-wmf.15+15 -0
mediawiki/extensions/MobileFrontendmaster+1 -1
mediawiki/corewmf/1.27.0-wmf.14+13 -0
mediawiki/corewmf/1.27.0-wmf.14+3 -1
mediawiki/extensions/MobileFrontendmaster+2 -1
operations/mediawiki-configmaster+1 -0
mediawiki/extensions/MobileFrontendmaster+18 -0
mediawiki/corewmf/1.27.0-wmf.10+12 -1
mediawiki/coremaster+12 -1
mediawiki/extensions/Scribuntomaster+16 -3
mediawiki/extensions/TemplateSandboxmaster+5 -1
mediawiki/extensions/MobileFrontendwmf/1.27.0-wmf.11+18 -3
mediawiki/extensions/MobileFrontendwmf/1.27.0-wmf.10+18 -3
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Note I've seen pages on the mobile site where the desktop table of contents is in the HTML for mobile (it shouldn't be)
You may want to test for this as well.

Ummm, can you provide an example of this? Were the edit section links also from desktop?

Note I've seen pages on the mobile site where the desktop table of contents is in the HTML for mobile (it shouldn't be)
You may want to test for this as well.

Ummm, can you provide an example of this? Were the edit section links also from desktop?

https://nl.m.wikipedia.org/wiki/Het_Zuiden_%28korfbalvereniging%29

Schermafdruk 2016-03-02 11.52.49.png (535×187 px, 28 KB)

No edit section links from desktop.

Appears to be unrelated - see T128702

I take this back.. it may or may not be related. Doesn't seem to be an issue in MobileFrontend.

https://www.mediawiki.org/wiki/Manual:MediaWiki_architecture was effected by this until I did ?action=purge on it around 2016-03-04T17:03.

And this page on huwiki until (?action=purge) yesterday evening.
In this case only for logged out users, but earlier this happened for logged in users, too (reported by Tacsipacsi here).

Hrm...so once we remove the FlaggedRevs instances from the logs and the page on ruwiki with a bug report...we're left with no log entries since Feb 28th, even though it's still happening. :S

(legoktm@fluorine:/a/mw-log/archive$ zcat T124356.log-20160* |grep -v 6189293 | grep -v FlaggedRevsUIHooks)

Change 275110 had a related patch set uploaded (by Jdlrobson):
Store mobileformatted text on OutputPage rather than ParserOutput

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

@Legoktm a full revert isn't possible but this patch should do the same thing ^

Uh. We need LESS hacks, not more. It's already extremely convoluted to follow the path of how MF mangles all the text, lets not make it worse. Why is a full revert impossible?

A revert firstly cannot be done without some conflicts.
Secondly a revert would removes the #mw-content-text wrapper which is added at the skin level (that's also a bug in my opinion that should be addressed in the skin)
The issue as I see it in the patch you want to revert is we went from storing in HTML cache to using ParserOutput. This is likely to be what's causing the issue. We just need to lean back on HTML caching.

I tried to revert this previously, but you said it had nothing to do with the parser cache and I abandoned it:
https://gerrit.wikimedia.org/r/#/c/266337/6

So now I'm a little confused with what you actually want.
That above patch is actually less hacky imo so I've restored it in case you want to consider it.

Note that https://gerrit.wikimedia.org/r/#/c/266337/6 can be done much cleaner once https://gerrit.wikimedia.org/r/275141 is merged.. It's entirely possible the additional logic may have confused you initially and taken away attention from the real purpose of the patch to move cache from parser to HTML.

https://de.wikipedia.org/wiki/Tantalcarbid still has the old style #/editor/ links so it won't have shown up in our logging.

A revert firstly cannot be done without some conflicts.
Secondly a revert would removes the #mw-content-text wrapper which is added at the skin level (that's also a bug in my opinion that should be addressed in the skin)
The issue as I see it in the patch you want to revert is we went from storing in HTML cache to using ParserOutput. This is likely to be what's causing the issue. We just need to lean back on HTML caching.

I tried to revert this previously, but you said it had nothing to do with the parser cache and I abandoned it:
https://gerrit.wikimedia.org/r/#/c/266337/6

So now I'm a little confused with what you actually want.
That above patch is actually less hacky imo so I've restored it in case you want to consider it.

@Jdlrobson, it's very good of @Legoktm to help out with this, but responsibility for seeing to it that this is resolved promptly falls squarely on your team, from my perspective. Please see to it that this is fixed. I don't care what needs to be reverted or how.

@ori I don't appreciate your tone here. I've been attentive on this bug up until I was told our change was unrelated to the bug. Now that we've been asked to revert it again I've again been attentive and already submitted https://gerrit.wikimedia.org/r/#/c/266337/6 and https://gerrit.wikimedia.org/r/275141 as solutions.

Bearing in mind, I'm currently the only available member of my team that understands this code and my team is very understaffed right no I'm not sure what more you can expect me to do right now. I'm going to need help reviewing those patches which should fix the problem if it is indeed the culprit.

Change 266337 had a related patch set uploaded (by Jdlrobson):
Improved revert of "Breaking change: Move logic for section wrapping out of skin into hook"

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

Change 275701 had a related patch set uploaded (by Legoktm):
Add "MF cache pollution debug log" live hack

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

Change 275701 merged by jenkins-bot:
Add "MF cache pollution debug log" live hack

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

I can replicate this locally now. https://gerrit.wikimedia.org/r/#/c/275141/ and https://gerrit.wikimedia.org/r/266337 fix the problem (note dependent patch - without which you'll break section collapsing on the mobile site). Can anyone commit to reviewing given this is unbreak now?

If I can get https://gerrit.wikimedia.org/r/#/c/266337/ merged in the next 2 hrs I will get it SWATed ... anyone?

Change 275110 abandoned by Jdlrobson:
Store mobileformatted text on OutputPage rather than ParserOutput

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

Change 276009 had a related patch set uploaded (by Jdlrobson):
Fix parser cache issues

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

SWAT scheduled for today 4pm PST

Change 266337 merged by jenkins-bot:
Fix parser cache issues

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

Change 276057 had a related patch set uploaded (by Jdlrobson):
Fix parser cache issues

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

Change 276009 merged by jenkins-bot:
Fix parser cache issues

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

Change 276057 merged by jenkins-bot:
Fix parser cache issues

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

SWATed to wmf15+16
Confirmed fixed. I could see the table of contents in the Barack Obama article:

id="toc" 
``
After a purge it was gone. I expect there will be many pages that need purging but hopefully this is the last of new cases.

I still see it at https://en.m.wikipedia.org/wiki/White_Rock_(album) and a cache-busting parameter doesn't help which makes me think that parser cache rejection filter doesn't work as intended.

Cache busting as in action=purge?
Note dirty entries will be in cache until purged.

Cache busting as in ?somerandomstring which bypasses Varnish but doesn't purge it.

Cache busting as in ?somerandomstring which bypasses Varnish but doesn't purge it.

Yeh that would suggest parser cache rejection filter is not working for already polluted entries.

Some observations. The VPT thread is now archived to Archive 144.

The problem of "Edit" links appearing without square brackets, and hard against the section header still occurs, and it is not confined to Vector: I have always used MonoBook, and have seen the problem several times in recent weeks. It's also nothing to do with the mobile interface, as I noted in the archived thread (search for "bet your sweet bippy").

I decided to see if it was generated by a particular server, so I waited for it to happen again, which it did just now at Gas Light. However, when I use my browser's "View source" feature, and search for "NewPP limit report" there isn't one. The normal page content ends, there are then seven blank lines, and then the line
</div></div><div class="printfooter">
In that seven-line gap, I would normally expect to see the "NewPP limit report" including "Parsed by ..." identifying the server concerned. So I can't assist there.

The problem of "Edit" links appearing without square brackets, and hard against the section header still occurs, and it is not confined to Vector: I have always used MonoBook, and have seen the problem several times in recent weeks. It's also nothing to do with the mobile interface, as I noted in the archived thread (search for "bet your sweet bippy").

I should clarify that the issue is not tied to how the page was edited but how it was viewed. If a page was not in cache (e.g. expired/purged) and then viewed with the mobile site, it would have polluted the cache for the desktop equivalent and vice versa (I'm actually seeing a lot more problems with the mobile site then desktop but none of these are as visible as desktop as CSS/JS is fixing the problem coincidentally)

I am 90% confident that we will not see any new reports and this is now just a caching problem but I am a little concerned that the parser cache rejection filter does not appear to be working. Please advise editors to continue to manually parse pages as they encounter the issue and hopefully we will be up and running again as normal in no time.

So… is the parser cache rejection thing that was talked about even deployed? I can't seem to find anything like that anywhere at all.

@matmarex I didn't deploy it. I remember having a conversation about doing that but I too cannot find a record of it actually happening (and I'm not actually sure what the parser cache rejection thing is). This bug has been a little confusing so I wouldn't be surprised if bad assumptions were made.

If someone can confirm this didn't happen and point me at the patch that needs deploying I'm happy to pick this up and make sure it gets SWATed later.

Hi. I am still seeing this in https://ca.wikipedia.org/wiki/Corts_Generals

We need to work out how to reject old values via Parser in T125841 - help needed!

Jdlrobson lowered the priority of this task from Unbreak Now! to Medium.Mar 24 2016, 8:28 PM

Due to less reports. Can we safely presume this is fixed now?

matmarex assigned this task to Jdlrobson.

https://gerrit.wikimedia.org/r/#/c/274165/ used to be cherry picked for the wmf branch. As of wmf/1.27.0-wmf.23 that causes a conflict and since this task is marked as resolved I am assuming the debug log is no more needed.

Change 286635 had a related patch set uploaded (by Hashar):
make-wmf-branch: drop mw cherry pick

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

Change 286635 merged by jenkins-bot:
make-wmf-branch: drop mw cherry pick

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

Change 302114 had a related patch set uploaded (by Addshore):
Remove T124356 debug logging

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

Change 302114 merged by jenkins-bot:
Remove T124356 debug logging

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

Mentioned in SAL [2016-08-01T15:27:32Z] <addshore@tin> Synchronized wmf-config/InitialiseSettings.php: [[gerrit:302114|Remove T124356 debug logging]] (duration: 00m 25s)