Page MenuHomePhabricator

action=mobileview sections delivering a div of class "mw-parser-output" without a matching closing div
Closed, ResolvedPublic

Description

First noticed on "enwiki > Tony Blinken".

I think this just started happening today.

Example:
https://test.wikipedia.org/w/api.php?action=mobileview&format=json&page=Bird4&sections=all&prop=text%7Csections

Screen Shot 2017-05-11 at 5.42.43 PM.png (864×2 px, 209 KB)

Probably unbreak now / high priority?

May have been caused by https://gerrit.wikimedia.org/r/#/c/350634/ T37247

Event Timeline

Mhurd triaged this task as Unbreak Now! priority.May 12 2017, 12:59 AM
Mhurd updated the task description. (Show Details)

@Anomie @Jdlrobson
Hey guys I noticed this funny unbalanced div issue today. Any thoughts?

Guessing that new div just shouldn't be added to mobileview sections output?

Mhurd renamed this task from action=mobileview is delivering a div of class "mw-parser-output" without a matching closing div to action=mobileview sections delivering a div of class "mw-parser-output" without a matching closing div.May 12 2017, 1:07 AM

Looks like the mobileview section parser needs to be updated to take the new div into account. I'll tag MCS as well since this change is coming to Parsoid soon, too.

Ya since this new div isn't section html, it probably just needs to be removed from mobileview section output.

rEMFR263f10759583: Update tests for core change If4eb5bf7 was supposed to take care of it. But it looks like the call to WikiPage::getParserOutput() is using the cached version with the wrapper, which wasn't caught by the unit testing. Options:

  • Undo the part of 263f10759 that sets the ParserOptions flag and instead just strip the wrapper in ApiMobileView.
    • Drawback: Assumptions about what core does.
  • Pass true for the $forceParse option to WikiPage::getParserOutput()
    • Drawback: ApiMobileView won't use the cached ParserOutput. But it still does its own caching, so that may not matter much.
  • Have WikiPage::shouldCheckParserCache() return false if $parserOptions->getWrapOutputClass() !== 'mw-parser-output'
    • Drawback: ApiMobileView won't use the cached ParserOutput. But it still does its own caching, so that may not matter much.
  • Have ParserOptions::optionsHash() include the wrapper value.
    • Drawback: Fragments the cache.

You might also have to bump your ApiMobileView cache version constant, or otherwise clear its cache, to clear the bad data.

This is the same problem as T164733. It looks like the change wasn't reverted and rode the train. o_O

Jdlrobson added a subscriber: hashar.

@Mhurd the issue should be fixed now as we swatted a fix for T164733 with help from @hashar
If you experience it again try applying action=purge or editing the page.

Look good?

@Mholloway try again in an incognito window.. probably cached locally.

Screen Shot 2017-05-12 at 2.59.45 PM.png (301×1 px, 221 KB)

Still there in an incognito window...

Screen Shot 2017-05-12 at 10.16.29 AM.png (900×1 px, 730 KB)

There's probably a cache somewhere that's still polluted. The API itself has a level of caching too so you may need to edit several times.
Try cache busting e.g. https://en.wikipedia.org/w/api.php?action=mobileview&format=json&page=Barack_Obama&prop=text&sections=0&cachebust=1

Unfortunately, without an expensive full site Varnish purge from ops we are likely to see these kind of issues for the next 7 days. For mobile web this means section editing is broken on various pages. This is why I was keen to revert the patch in T164733.

I think the cache takes a few days to purge, right?

@Tgr?

Action API responses are not cached unless you use the smaxage parameter. (In general you can avoid Varnish by using the X-Wikimedia-Debug header; there are browser plugins to do that.)

If the API uses the parser cache somehow, then it's cached to up to 30 days (unless someone edits), calling action=purge on the affected page will probably help.

Change 353566 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] ParserOptions: Include wrapping class in options hash

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

Here is a minimal reproduction of the issue:

tgr@terbium:~$ mwscript eval.php --wiki=enwiki

> $html = '<div class="mw-parser-output">foo</div>';

> $title = Title::newFromText( 'Barack Obama' );

> $mf = new MobileFormatter( MobileFormatter::wrapHTML( $html ), $title );

> $mf->filterContent();

> return $mf->getText();
<div class="mw-parser-output">foo</div>

filterContent should have called makeSections which should have removed the wrapper but that does not seem to be happening. At a glance there is no caching involved.

All that seems to be happening here is that, originally, the section transformation was made inside the <body> tag, then at the end MobileFormatter::getText() unwrapped it. d154fa36b5c3
changed the transformation to happen inside <div class="mw-parser-output"> instead but nothing removes that element. MobileFormatter::getText() should just be updated to do it.

d154fa36b5c3 is unrelated.

263f10759 attempted to fix this issue by instructing the parser (via ParserOptions) to not include the wrapper. However, WikiPage::getParserOutput() is returning a cached ParserOutput that does include the wrapper despite the passed ParserOptions saying to exclude the wrapper. Hence my suggested fixes in T165115#3257490.

In local testing https://gerrit.wikimedia.org/r/353566, which implements the fourth bullet from that comment, does fix this bug with no additional change to MobileFrontend.

Yeah the action=purge strategy was mentioned in another thread by Jon Robson so I'm guessing this is in the parser cache.

We may have to do something there.

Just to note, although the change was swatted and removed from production, master still has the change.

Also verified tha TTL is about 1 day for most pages, so the cache should be good to go over the weekend.

Not sure how this affects RESTBase

Hey Services will we need to do any special purging of summary or feed content?

Not sure how this affects RESTBase

A corresponding change was merged into the Parsoid repo but isn't yet deployed. https://gerrit.wikimedia.org/r/#/c/352711/

ETA: This is as pertaining to Parsoid HTML content stored in mobile-sections*, which won't have been affected (yet). The summary endpoint is already receiving bad content and will likely need a purge/regeneration.

Note the Parsoid change adds a class to the existing <body> tag rather than adding a wrapper. Thus both Flow and VE didn't actually pick up the change (tested on Beta Labs), I have no idea whether MCS will similarly ignore it or not.

Not sure if I understand how ParserCache works but it seems like ApiMobileView::getData calling ParserCache::getKey will result in this change getting ignored (at least for the duration of mobileview API result caching) as that seems to only take into account the parser options which had a non-default value when the page was last saved, so updating the wrapper parser option inside the API does not break the cache.

Hey Services will we need to do any special purging of summary or feed content?

Sorry for the late reply, but we have truncated the summary and feed tables on Friday, so all is good there.

Why is this ticket still opened? Is this still an issue?

https://gerrit.wikimedia.org/r/#/c/353566/ still has to be merged, right? And possibly the mobileview API cache needs to be purged or split as well.

Please verify whether this affects clients and other parts of the system in production. If it does not, it's not a UBN situation and the ticket's priority should be lowered.

As far as I know, the current situation is that the bugs caused by the wrapper element are fixed by a revert in 1.30.0-wmf.1. They still occur in master, meaning they can be seen in Beta Labs and will happen again when 1.30.0-wmf.2 is rolled out[1] if fixes aren't made and merged by then.

For this bug in particular, https://gerrit.wikimedia.org/r/#/c/353566/ should fix it if it's merged, although the cache maintained by ApiMobileView might continue to contain broken entries.

[1]: Next week (May 23–25), there's no train this week due to the Hackathon.

Tgr lowered the priority of this task from Unbreak Now! to High.May 15 2017, 2:44 PM

Change 353566 merged by jenkins-bot:
[mediawiki/core@master] ParserOptions: Include wrapping class in options hash

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

I believe this is all fixed now, but I'll let @Anomie confirm.

I believe this is all fixed now, but I'll let @Anomie confirm.

Should be, yes. For example, https://en.wikipedia.beta.wmflabs.org/w/api.php?action=mobileview&page=Asian+black+bear&sections=all&prop=text|sections looks correct now. Do the Mobile people need to do more testing?

It looks good from my side, but the apps rely on this API and web doesn't so it would be best for them to verify.

I believe this is all fixed now, but I'll let @Anomie confirm.

Should be, yes. For example, https://en.wikipedia.beta.wmflabs.org/w/api.php?action=mobileview&page=Asian+black+bear&sections=all&prop=text|sections looks correct now. Do the Mobile people need to do more testing?

It looks good from my side, but the apps rely on this API and web doesn't so it would be best for them to verify.

Looks good to me.

Closing for now. We talked about this a bit at the hackathon and my understanding is that the issues should be resolved in master. Will be monitoring for the wmf.2 train.