Page MenuHomePhabricator

Unexpected general module "mobile.messageBox" and "mobile.mainMenu" in styles queue.
Closed, ResolvedPublic2 Story Points

Description

This warning has been in local Vagrant logs for a long time but due to it not happening on every page view, it was presumably not noticed before. Since 976943c9913b, the severity of these violations has been raised to level "Warning" in production - in preparation for removing support for this error entirely.

This is being logged at a regular frequency from production wikis in the resourceloader channel. It's simple to fix (see parent task for more information.)

resourceloader.log
Unexpected general module "mobile.messageBox" in styles queue.
Unexpected general module "mobile.mainMenu" in styles queue.

A/C

  • The styles are pulled out into mobile.messageBox.styles and mobile.mainMenu.styles modules
  • mobile.mainMenu should have a dependency on mobile.mainMenu.styles
  • mobile.messageBox should have a dependency on mobile.messageBox.styles
  • On special pages where the JS and templates are not required only load the styles modules

Event Timeline

Krinkle created this task.May 10 2017, 12:49 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 10 2017, 12:49 AM

The mobile.messageBox module has both a stylesheet, and a JS file, HTML template, and a module dependency.

The module is loaded in three ways, it seems this is probably not done so intentionally but happens to work right now (or is broken already?)

  1. SkinMinerva::getSkinStyles - Uses addModuleStyles. Loads in the blocking stylesheet without JS. Ignores the JS, template and dependency.
  2. onBeforePageDisplay hook - Uses addModuleStyles. Loads in the blocking stylesheet without JS. Ignores the JS, template and dependency.
  3. Indirectly as dependency of mobile.nearby or mobile.editor.common - Depending on whether the page loaded the module as styles-only already, this may or may not actually deliver the HTML and JS.

A module can either be a styles-module intended for loading in a blocking stylesheet, or it is a general module that has (or may also have) JS content and supports dependencies, messages, and templates. You cannot load a general module's stylesheet through addModuleStyles() and ignore the rest of the module (which cannot be loaded with CSS). For example, right now this module is loaded with addModuleStyles in two places, which suggests that the styles of this module apply to server-generated HTML (not related to dynamically created elements by JavaScript). But they're also in the same module as a JavaScript file, which suggests this needs those styles somehow as well.

When the module is dependent on this, does that mean it depends on the styles, or the JS as well? Should the third case load the entire module again (thus loading the styles for a second time) or was the first time "really" done.

Soon, the latter will be assumed per T92459, which fixes many bugs. However that will break this use case.

Perhaps the script is unused and the module is treated as styles-only? Or perhaps it needs to be split up, or have the JS portions moved to a different module.

Krinkle renamed this task from Unexpected general module "mobile.messageBox" in styles queue. to Unexpected general module "mobile.messageBox" and "mobile.mainMenu" in styles queue..May 10 2017, 1:43 AM
Krinkle updated the task description. (Show Details)
Jdlrobson moved this task from To Triage to Upcoming on the Readers-Web-Backlog board.
ovasileva set the point value for this task to 2.May 16 2017, 3:31 PM
phuedx claimed this task.

Change 354222 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Break out mobile.messageBox styles module

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

Change 354225 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/MobileFrontend@master] rl: Break out mobile.mainMenu style module

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

phuedx added a subscriber: bmansurov.

Thanks for your review @bmansurov!

Change 354222 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] rl: Break out mobile.messageBox styles module

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

Change 354225 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] rl: Break out mobile.mainMenu style module

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

Jdlrobson claimed this task.
Jdlrobson added a subscriber: phuedx.

I'm still seeing errors in the logs today, but last instance 04:54:23. Let's wait till there are no instances in last 24hrs before signing off to check these are gone completely.

Krinkle closed this task as Resolved.May 22 2017, 5:44 PM
Krinkle reopened this task as Open.EditedJul 6 2017, 7:00 PM
Krinkle raised the priority of this task from High to Unbreak Now!.

The Unexpected general module "mobile.messageBox" in styles queue. warning returned to the logs yesterday.

It seems to only happen on pt.wikipedia.org and wikitech.wikimedia.org.

Re-opening and raising priority to UBN because per T92459, this restriction is now enforced as of last week. This means the module won't get loaded. If the page still works fine, the addModuleStyles() call simply be removed.

Restricted Application added subscribers: Jay8g, TerraCodes. · View Herald TranscriptJul 6 2017, 7:00 PM

Change 363651 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Only message box styles should be loaded on editor

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

Change 363651 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Only message box styles should be loaded on editor

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

Change 363741 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@wmf/1.30.0-wmf.7] Only message box styles should be loaded on editor

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

greg added a subscriber: greg.Jul 6 2017, 10:57 PM

Change 363741 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.30.0-wmf.7] Only message box styles should be loaded on editor

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