Page MenuHomePhabricator

Regression: Module mobile.site.styles is not being loaded with mobile.site module
Closed, ResolvedPublic3 Story Points

Description

On itwiki we noticed that, on certain pages, mobile style isn't loaded. For instance, this happens here, where the class sinottico isn't loaded and thus breaks the infobox. I did a little bit of investigation on my local machine and found out that the culprit might be https://gerrit.wikimedia.org/r/#/c/420405/. Specifically, this if evaluates to false and style isn't loaded; actually, when should it evaluate to true? It asks for a global variable which defaults to false and which, as far as I could see, is never changed. Anyway, I'm not sure that the one I exposed is the real cause, but certainly inverting that if makes the style come back.

From the user's perspective, the symptom is Mobile.css being not applied.

Developer notes

Acceptance criteria

  • Loading mobile.site also loads mobile.site.styles

Event Timeline

Daimona created this task.Apr 6 2018, 7:27 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 6 2018, 7:27 AM
Daimona added a comment.EditedApr 6 2018, 4:14 PM

I realised that such global variable is meant to be used in the future. The problem is shifted to includes/modules/MobileSiteModule.php, where I think getDependencies isn't called (while getPages is), although I can't figure out why.

Is it related that ResourceLoaderWikiModule, unlike similar classes, doesn't have a getDependencies method?

Daimona moved this task from Backlog to Bugs on the MobileFrontend board.Apr 7 2018, 8:18 AM
Daimona added subscribers: Jdlrobson, Krinkle, Catrope.

Sounds like sometimes the CSS is loaded, sometimes not and sometimes only some rules are loaded. This may lead to corrupted mobile visualization. Adding as subscribers people from previous task.

What is the problem? Its not clear from the description what is happening.

@Jdlrobson the main problem is that mobile style, i.e. Mediawiki:Mobile.css isn't loaded, thus breaking page style.

Jdlrobson renamed this task from Mobile style isn't loaded to Module mobile.site.styles is not being loaded with mobile.site module.Apr 8 2018, 4:20 AM

I'll look into this Monday, but yes it seems like the dependencies inside https://gerrit.wikimedia.org/r/#/c/420405/4/includes/modules/MobileSiteModule.php,unified are not being honoured for some reason.

As a workaround you can put this code into MediaWiki:Mobile.js to get this to work:

mw.loader.using('mobile.site.styles')
Nirmos added a subscriber: Nirmos.Apr 8 2018, 6:15 AM

@Jdlrobson Thanks for the hint, I applied the fix.

Restricted Application added a project: Performance-Team. · View Herald TranscriptApr 8 2018, 10:56 AM
Jdlrobson renamed this task from Module mobile.site.styles is not being loaded with mobile.site module to Regression: Module mobile.site.styles is not being loaded with mobile.site module.Apr 9 2018, 5:11 PM
Jdlrobson triaged this task as High priority.
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.

@Jdlrobson I looked at this over the weekend. I believe the culprit is the isKnownEmpty() logic.

The wiki previously did not have a Common.js or Mobile.js page. As such, their mobile.site was previously loading with an empty script, and the CSS only. That's fine.

When we split the modules into mobile.site and mobile.site.styles (with the former depending on the latter), the former became empty. ResourceLoader would normally load both modules at once in the same batch just like before (given dependency resolution happens ahead of time). But this isn't working because WikiModule, unlike FileModule, has an isKnownEmpty() method that makes the module skip itself if none of its associated pages exist on the wiki.

This makes it so that modules like site.styles and mobile.site.styles, which require loading in a separate http request, only happen from the HTML if said feature is enabled (wgUseSiteCSS) and used (the on-wiki page exists).

The problem is that, while this works fine for style modules, it poses a problem for JS modules, as skipping them means we also don't look at the dependencies anymore.

As far as I can tell, the fix would be to not apply isKnownEmpty to dynamic loading, but only to hardcoded http links. I recall this being what it was designed for, so I'm unsure of as to why and how it got to apply to dynamic loading. But should be a simple fix.

Imarlier moved this task from Inbox to Radar on the Performance-Team board.Apr 9 2018, 8:04 PM
Imarlier edited projects, added Performance-Team (Radar); removed Performance-Team.
whym awarded a token.Apr 10 2018, 2:13 PM
whym rescinded a token.
whym awarded a token.
whym updated the task description. (Show Details)Apr 10 2018, 2:16 PM
whym added a subscriber: whym.

Description edited - hopefully this is now more discoverable by keywords like 'Mobile.css'.

@Krinkle the mobile.site module in extension.json (https://gerrit.wikimedia.org/r/#/c/420405/4/extension.json) appears to be specifying its own dependencies which clobber the default module dependencies.

pmiazga set the point value for this task to 3.Apr 10 2018, 4:13 PM
Krinkle added a comment.EditedApr 10 2018, 4:39 PM

@Krinkle the mobile.site module in extension.json (https://gerrit.wikimedia.org/r/#/c/420405/4/extension.json) appears to be specifying its own dependencies which clobber the default module dependencies.

Yeah, that's a separate issue. That dependency should probably be moved to the MobileSiteModule class. However, note that the code being referred to here already didn't work. Even before this commit, mobile.site was using class MobileSiteModule, which extends WikiModule without implementing an dependencies option, and without a getDependencies implementation. So as far as I can tell, this has not been working for quite some time. It may be safest to simply remove it.

// Location: https://en.m.wikipedia.org/wiki/Main_Page

mw.loader.moduleRegistry['mobile.site']
//> {module: {…}, version: "1g4dpfx", dependencies: Array(1), group: null, source: "local", …}

.dependencies
//> ["mobile.site.styles"]

Change 425570 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Do not rely on dependencies to load mobile.site/mobile.site.styles

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

I've intentionally kept the fix for this as simple as possible ^

@Jdlrobson, it looks like there's an open question in code review as to how to proceed. Should we split this task so that the original tracks the revert and the split tracks the proper fix?

Krinkle added a comment.EditedApr 18 2018, 6:13 PM

I'll fix the bug in core which is about as much effort as a revert, but without all the regressions.

But I'd like to have a failing test in RLClientHtml to work from. That should be easy to set-up, and I can help with that too (ping me on IRC).

Change 428545 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] isKnownEmpty modules with dependencies cannot be empty

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

Krinkle assigned this task to Jdlrobson.Apr 24 2018, 2:43 AM

Change 428545 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Consider having dependencies as non-empty in WikiModule::isKnownEmpty

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

Minor follow up in https://gerrit.wikimedia.org/r/425570 needs to be merged. Once that's done we can move this to QA.

Change 425570 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] mobile.site should not have dependencies

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

Jdlrobson closed this task as Resolved.Apr 25 2018, 5:49 PM

This can skip sign off as it's tricky to test.
https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page is now running site css (note the logo "BLabs")

@Jdlrobson @Krinkle et al., thanks for fixing. However there's a little problem: the first patch was deployed with wmf.1, the other one will be in wmf.2. In the meanwhile, keeping the mobile.js as suggested above will load mobile style also in desktop version, while removing the manual RL call we still have the initial problem reported in this task. Could someone please take a look and deploy the last patch on wmf.1 if necessary?

@Daimona the first patch won't do anything. The second patch is the one that's needed.
The suggestion in https://phabricator.wikimedia.org/T191596#4114628 will load the styles in the mean time on the mobile. It will not load the styles in desktop - MediaWiki:Mobile.js is only loaded for mobile devices. You can use that workaround safely with or without the second patch. If you are seeing that JS running on desktop we have a bigger problem here! :)

@Jdlrobson I had applied the suggestion to manually call RL, but after yesterday's deployment of wmf.1 many users started complaining for wrong graphics. It turned out that the mobile style was loaded on desktop and deleting Mobile.js brought everything back to normal. Of course, without that manual call, mobile users still don't have the CSS. So, I'm afraid we actually have another problem here, although I'm not sure about how big it could be :-)

@Daimona Hmmm.. I'm not seeing how this could happen. I just verified now that it's impossible for the mobile script to load on desktop so I'm not sure what could have caused this.

If the Mobile.js page is restored and the issue returns I'll be happy to debug that and work out what's going on for you but it should be safe to restore that page. If not, we should identify the bug here and get that fixed immediately.

@Jdlrobson Huh, I restored Mobile.js and the problem is gone, both for desktop and mobile. Maybe it's been some cache-ish problem, but at least now everything's fine :-)

Obelus added a subscriber: Obelus.Sep 20 2018, 3:37 PM