[Bug] Many MobileFormatter lead paragraph transform alerts starting on Feb 11, 2020
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT
Actions

Assigned To

Authored By

	• Niedzielski
	Feb 13 2020, 3:30 PM

Description

There are in excess of 50 thousand MobileFormatter lead paragraph transform alerts per 24 hour window starting on Feb 11:

Screenshot_2020-02-13 Kibana.png (400×1 px, 19 KB)

https://logstash.wikimedia.org/goto/04520c8fceeb291e778f6cfcf4daf6dc

Developer notes

The issue seems to occur when Title::getLatestRevID is 0 which can return 0 when !Title::getArticleID( 0 )

It's unclear when this happens (maybe in a special page?), but MobileFormatter::canApply should return 0 if this is the case.
I'd suggest that ExtMobileFrontend::domParse considers revision before applying a mobile formatter.
This will suppress the logs and ensure we don't run the MobileFormatter on pages it is not needed.

Details

	Subject	Repo	Branch	Lines +/-
	Disable mobileFormatter on titles with a revision ID of 0.	mediawiki/extensions/MobileFrontend	master	+42 -20

Customize query in gerrit

Related Objects

Mentioned In: T248493: Lead paragraph transform does not apply in edit preview mode
T248141: Regression: MobileFrontend overrides non-existent user page handling in desktop view
Mentioned Here: T248493: Lead paragraph transform does not apply in edit preview mode
T218056: Infobox does not get moved behind the lead paragraph if it's a DIV element

Event Timeline

• Niedzielski created this task.Feb 13 2020, 3:30 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 13 2020, 3:30 PM

ovasileva claimed this task.Feb 17 2020, 2:18 PM

ovasileva moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.

@Niedzielski, @Jdlrobson - could this be connected to changes we made in T218056: Infobox does not get moved behind the lead paragraph if it's a DIV element?

At first I thought maybe... in which case I would expect to see more reports for wikis which added the infobox class to div elements that were not infoboxes however looking closely at the reports this is not the case. I instead see requests for strange URIs such as Famotidine/static/apple-touch/wikipedia.png and deleted pages such as 2044_Summer_Olympics, Jessica Garza, Magic Roundabout (Colchester

All the requests have one thing in common - they log "rev:0" - so they are running on a page that shouldn't exist.

It seems like they shouldn't be reported in the first place. The URI "/w/index.php?title=Can-Am_Rugby_Tournament&action=edit&redlink=1+and+1=1" for example should never trigger the lead paragraph transform and why it is is mysterious to me. The MobileFormatter (which is an expensive backend operation) is running on HTML it shouldn't be.

We can and should suppress these logs and can do easily by checking revision ID when we create the MobileFormatter.
To understand why this will require further investigation

This URI shows what the logs will look like with the fix to check revision: https://logstash.wikimedia.org/goto/b0598345ef61bf83ce9e3398b44d628f

I'd strongly recommend a swift response to this error one way or the other.

ovasileva edited projects, added Web-Team-Backlog (Kanbanana-2019-20-Q3); removed Web-Team-Backlog.Feb 17 2020, 5:44 PM

Jdlrobson updated the task description. (Show Details)Feb 17 2020, 7:03 PM

Jdlrobson moved this task from Needs Analysis to Upcoming on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.

ovasileva lowered the priority of this task from High to Medium.Feb 18 2020, 5:41 PM

ovasileva set the point value for this task to 3.Feb 18 2020, 5:45 PM

@ovasileva I'm bumping up to high as the MobileFormatter seems to be running on CSS rather than HTML! This is bad - lead paragraph code should not be running on CSS (or potentially JS) - who knows what problems that could be causing!

Found infobox wrapped with container on Linear-gradient(transparent, transparent), https://en.m.wikipedia.org/w/load.php (rev:0)

ovasileva moved this task from Upcoming to Ready for Development on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.Mar 3 2020, 6:14 PM

Change 577231 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Disable mobileFormatter on titles with a revision ID of 0.

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

gerritbot added a project: Patch-For-Review.Mar 5 2020, 12:51 PM

Jdrewniak claimed this task.Mar 5 2020, 12:53 PM

Jdrewniak moved this task from Ready for Development to Doing on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.

Jdrewniak added a subscriber: ovasileva.

Jdlrobson moved this task from Doing to Needs More Work on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.Mar 5 2020, 9:05 PM

All the requests have one thing in common - they log "rev:0" - so they are running on a page that shouldn't exist.

I'm not super familiar with this part of the codebase, but it looks like rev:0 pages are actually passed to the mobileFormatter in the case of blank user-pages.
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/9de18f7d9d9f01518e54a569e14ac52b882fa11e/includes/ExtMobileFrontend.php#L47

(I'm assuming checking for revision ID is kinda the same as $title->exists() )

So I think the solution here might involve more than preventing these pages from being passed to mobileFormatter.

In T245160#5953835, @Jdrewniak wrote:

All the requests have one thing in common - they log "rev:0" - so they are running on a page that shouldn't exist.

I'm not super familiar with this part of the codebase, but it looks like rev:0 pages are actually passed to the mobileFormatter in the case of blank user-pages.
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/9de18f7d9d9f01518e54a569e14ac52b882fa11e/includes/ExtMobileFrontend.php#L47

(I'm assuming checking for revision ID is kinda the same as $title->exists() )

So I think the solution here might involve more than preventing these pages from being passed to mobileFormatter.

These pages don't need the MobileFormatter at all - the code only targets user pages that do not exist and renders mostly static content. I've proposed moving this logic out of ExtMobileFrontend into the hook level. In future I think we could refactor this further, but given the goal is suppressing the errors I think this is enough. Take a look at my changes and let me know what you think.

phuedx moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.Mar 11 2020, 3:01 PM

Jdlrobson moved this task from Needs More Work to Needs Code Review on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.Mar 11 2020, 11:12 PM

Jdrewniak removed Jdrewniak as the assignee of this task.Mar 11 2020, 11:54 PM

Jdrewniak assigned this task to Edtadros.

Jdrewniak removed Edtadros as the assignee of this task.

Jdrewniak moved this task from Needs Code Review to Ready for Signoff on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.

Jdrewniak moved this task from Ready for Signoff to Needs QA on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.

Jdrewniak added a subscriber: Edtadros.

Jdrewniak subscribed.

@Jdlrobson thanks for handling this edge case! (I figured the blank user page shouldn't be in MobileFormatter, but didn't know where to put it).

In terms of QA, I guess we needs verify that the number of errors has been reduced (that's probably something an engineer can check).

Change 577231 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Disable mobileFormatter on titles with a revision ID of 0.

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.24; 2020-03-17).Mar 12 2020, 1:00 AM

Blocked on train. This was meant to go out Thursday 19th, but likely to be longer now. Possibly might want to SWAT the fix out of the normal train.

Should be live now.

Jdlrobson mentioned this in T248141: Regression: MobileFrontend overrides non-existent user page handling in desktop view.Mar 20 2020, 3:04 PM

Jdlrobson moved this task from Needs QA to Ready for Signoff on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.Mar 25 2020, 12:11 AM

Jdlrobson claimed this task.Mar 25 2020, 5:23 PM

Jdlrobson mentioned this in T248493: Lead paragraph transform does not apply in edit preview mode.Mar 25 2020, 5:43 PM

We are now down to a very manageable < 25 errors a day. The errors also seem more accurate than before possibly due to T248493.

	F31614589: Screenshot_2020-02-13 Kibana.png
	Feb 13 2020, 3:30 PM