Page MenuHomePhabricator

Module site.styles generates different output depending on mobile cookie, if $wgMFSiteStylesRenderBlocking = true;
Open, Needs TriagePublic

Description

After enabling $wgMFSiteStylesRenderBlocking = true; I started receiving inconsistent styling on the page. Sometimes, MediaWiki:Common.css was being loaded on the mobile site (which shouldn't), and sometimes MediaWiki:Mobile.css was being loaded on desktop site.

After a lot of digging, surprisingly, I found the culprit was a load.php URL that was loading the CSS of the site. The output was one or the other depending on the presence of the mf_useformat cookie, that determines if it should render the mobile skin or desktop. The URL is similar to:

http://dev.wiki.local.wmftest.net:8080/w/load.php?lang=en&modules=site.styles&only=styles&skin=minerva&version=asda8

That URL has a Cache-control: public, max-age=300, s-maxage=300 response header, which means the same user switching from mobile to desktop may get inconsistent styles, and if there's a frontend cache, it may cache wrong responses.

Steps to reproduce

  1. Use MediaWiki-vagrant to set up a test site, enabling mobilefrontend and minerva roles.
  1. Add a settings.d/99-misc.php file with this contents:
<?php
$wgMFSiteStylesRenderBlocking = true;
  1. Open the wiki on a browser, login, and edit the [[MediaWIki:Common.css]] page, adding:
.commoncss{border:2px solid red;background:yellow;color:darkblue}
  1. Edit the [[MediaWIki:Mobile.css]] page, adding:
.mobilecss{border:2px solid green;background:darkblue;color:yellow}
  1. Create a [[Test]] page, adding:
<div class="commoncss">Styles from common.css</div>
<div class="mobilecss">Styles from mobile.css</div>
  1. Open the URL http://dev.wiki.local.wmftest.net:8080/w/load.php?lang=en&modules=site.styles&only=styles&skin=minerva&version=v1 on a new tab. It should display contents from step 3
  1. Click on the "mobile view" link in the footer
  1. Open the URL http://dev.wiki.local.wmftest.net:8080/w/load.php?lang=en&modules=site.styles&only=styles&skin=minerva&version=v2 (note the different version) on a new tab. It should display contents from step 4

(un)Expected results

A ResourceLoader URL that's not user-private, should be cacheable and fairly static, and depend only on its params. This one seems to depend on a cookie (set when manually switching to mobile view from a desktop browser or viceversa), which causes issues when this URL is cached.

The mobile site should load a different RL module for its site CSS, to prevent the described problems.

Additional info

I don't know why, but this seems to be reproduced more easily on the browser when you also set $wgMFAutodetectMobileView = false;, you deactivate the cache of your browser, and use the useformat=desktop or useformat=mobile parameters on the URL to view the page on the desktop/mobile view without actually switching.

Also, it's somehow reproducible even if $wgMFSiteStylesRenderBlocking = false (the default), if you manually load the URLs, but from my tests it's hardly reproducible on a browser, maybe because it uses more distinct URLs between desktop and mobile.

This is also reproducible using the REL1_35 branch.

Event Timeline

Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald TranscriptDec 20 2020, 11:03 PM
Restricted Application added a project: Performance-Team. · View Herald TranscriptDec 21 2020, 3:27 PM

There are several things going on here but I don't know where the problem lies right now.

The page used in the module varies based on whether mobile or desktop is used:
https://github.com/wikimedia/mediawiki/blob/master/includes/resourceloader/ResourceLoaderSiteModule.php#L45

https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/d2a8dea69d984feea4679367b80366beb4a37794/includes/MobileFrontendHooks.php#L404
In the urls I should note the target= query string parameter is missing for some reason which I find suspect. Perhaps
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/d2a8dea69d984feea4679367b80366beb4a37794/includes/MobileFrontendHooks.php#L173 needs to be set earlier?
In the case of mobile the uri should contain target=mobile

I see it for script urls but not style urls. That may be a bug in the stylesheet URL generation for resourceloader

If I understand it correctly, this is what's happening:

  1. MediaWiki generates load.php URLs that the browser uses to fetch CSS and JavaScript. Those URLs contains the parameters: lang, modules, skin, version. It tells which skin is the module for, but not if the module is being loaded for a mobile view or a browser view.
  2. When the site or site.styles module is requested to MediaWiki, the ResourceLoaderSiteStylesModulePages hook is used by MobileFrontend to modify the MediaWiki pages it should load code for. It uses the MobileContext class to know whether it's using the mobile or desktop view, using cookies or an URL parameter to detect this. But in point 1, there's no URL parameter for that, so it relies on cookies.

I indeed see a target=mobile as a parameter, but only for the startup module. It's missing in all other load.php URLs.

Change 653973 had a related patch set uploaded (by Martineznovo; owner: Martineznovo):
[mediawiki/core@master] ResourceLoader: Make ResourceLoaderContext aware of target

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

Change 653974 had a related patch set uploaded (by Martineznovo; owner: Martineznovo):
[mediawiki/extensions/MobileFrontend@master] Use ResourceLoaderContext to get the actual target

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

I have ventured to write a patch that should fix this. However, I'm not sure if they'll get accepted, since it changes a couple of hooks to pass the ResourceLoaderContext, required to know whether the modules have been loaded from target=mobile.

Looking at the linked task, I can see that this problem (which I'm experiencing only since MediaWiki 1.35) seems to be caused by the change from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/562380

dpifke moved this task from Inbox to Radar on the Performance-Team board.Mon, Jan 4, 7:18 PM
dpifke edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle added a subscriber: Krinkle.EditedMon, Jan 4, 10:12 PM

Varying the module definition by mobile mode is not officially supported. This patch calls shouldDisplayMobileView() which inspects data that is intentionally and by design not supported by MediaWiki to be inspected from static asset contexts such as these.

This happens to work for WMF because our traffic layer (Varnish) is configured to strip cookies from these requests, and because we serve MobileFrontend from a separate domain (for now, ref T214998). This only works can work for third-parties (though equally unsupported) so long as you also do both of those things.

The target system is currently implemented as a mostly client-side concept, with the exception of the startup registry that we fragment between the two modes. Everything else derives from that. Caches are generally not explicitly fragmented by target or mobile mode.

Changes uploaded (by Martineznovo; owner: Martineznovo):
[mediawiki/core@master] ResourceLoader: Make ResourceLoaderContext aware of target
https://gerrit.wikimedia.org/r/653973

[mediawiki/extensions/MobileFrontend@master] Use ResourceLoaderContext to get the actual target
https://gerrit.wikimedia.org/r/653974

These patches propose to pass down the target parameter to all static requests, thus allowing more server-side code to see its state (by accident, or intentionally), which significantly increases awareness of this deprecated system. For this to work, we would need to make numerous other changes to ensure it is considered everywhere as part of cache keys. Otherwise, there may be leakage between unrelated contexts (including for example, the localStorage cache).

As I understand it, use of shouldDisplayMobileView() or target from within module definitions or module responses, has never worked for third-parties. As such, I don't think we should make new investments at this time to introduce new support for that, only to have to deprecate and remove it along with the rest of it as part of T127268.

It's up to Reading Web to decide whether to support this feature without a separate domain. If they do, my recommendation would be to make a small investment now to make this feature become one of the MobileFrontend features that don't have reliance on the target system - instead of incurring additional technical debt across MediaWiki for this to work in even more situations. I will also note that adding this to ResourceLoader would add on-going maintenance overhead that thus further delays and slows down my ability to support the Vue.js roll out and related on-going projects, I don't know if I can or should sign off on something like that by myself.

[[MediaWiki:Mobile.css]] was mostly invented so we didn't have to deal with the technical debt of loading a large [[MediaWiki:Common.css]] on Wikimedia deployed wikis such as Wikipedia in an environment they hadn't run in before.

I think at this point, it's best for 3rd parties to use [[MediaWiki:Common.css]] and skin [[MediaWiki:Minerva.css]] to style mobile rather than inherit that technical debt.
This is also sitting in nicely with the long term goal of T248416. Long term we don't want this to exist.

If that sounds good, I would happily review and backport a patch that:

  • exits early from the ResourceLoaderSiteModulePages and onResourceLoaderSiteStylesModulePages( hook when a new configuration variable $wgMFCustomSiteModules is enabled. e.g. if ( $ctx->shouldDisplayMobileView() && $config->get('MFCustomSiteModules') ) {
  • Defaults the new configuration variable MFCustomSiteModules to false with a warning in the README.md that it should only be used when running mobile site on a different domain.

Note: This is not a priority from my team's side, so this would be me reviewing as part of my volunteer capacities one evening.

Change 653973 abandoned by Martineznovo:
[mediawiki/core@master] ResourceLoader: Make ResourceLoaderContext aware of target

Reason:
Not the desired path to go

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

Change 653974 abandoned by Martineznovo:
[mediawiki/extensions/MobileFrontend@master] Use ResourceLoaderContext to get the actual target

Reason:
Not the desired path to go

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

This happens to work for WMF because our traffic layer (Varnish) is configured to strip cookies from these requests, and because we serve MobileFrontend from a separate domain (for now, ref T214998). This only works can work for third-parties (though equally unsupported) so long as you also do both of those things.

Hmmm, I'm not sure if that's the case:

> curl -s -D - -o /dev/null 'https://en.wikipedia.org/w/load.php?lang=en&modules=site.styles&only=styles&skin=vector&x1' | grep content-length
content-length: 17193

> curl -s -D - -o /dev/null --cookie 'mf_useformat=true' 'https://en.wikipedia.org/w/load.php?lang=en&modules=site.styles&only=styles&skin=vector&x2' | grep content-length
content-length: 0

> curl -s -D - -o /dev/null --cookie 'mf_useformat=false' 'https://en.wikipedia.org/w/load.php?lang=en&modules=site.styles&only=styles&skin=vector&x3' | grep content-length
content-length: 17193

> curl -s -D - -o /dev/null 'https://en.wikipedia.org/w/load.php?lang=en&modules=site.styles&only=styles&skin=vector&x2' | grep content-length
content-length: 0

(note how this returns a cached "mobile" version although the cookie is no longer present!)

Yes, I had to add a cache booster at the end of the URL to bypass the cache, but results differ depending on the cookie. I guess someone can test those URLs with the mobile cookie and happen to get that mobile version cached instead of the desktop one... and cause some confusion to desktop users that won't see any CSS customization

This happens to work for WMF because our traffic layer (Varnish) is configured to strip cookies from these requests, and because we serve MobileFrontend from a separate domain (for now, ref T214998). This only works can work for third-parties (though equally unsupported) so long as you also do both of those things.

Hmmm, I'm not sure if that's the case: […] (note how this returns a cached "mobile" version although the cookie is no longer present!)

Correct, and we are saying the same thing in different words. Plus, I think you found a bug, or two.

If there was no Varnish or other CDN cache, this bug would not happen (naturally).

If there was a simple Varnish instance without WMF's config, the bug would (I think) also not happen. In such setup, the request with the cookie would not be cacheable at all, or (if you allowed caching of cookied-responses) then the responses would be stored by Varnish under their own separate cache key and everything would work functionally fine. It would, however, not be very efficient as that would mean the entire corpus of HTTP traffic (including load.php) could be stored with hunderds of thousands of duplicates for all possible cookie combinations, including those that might not influence the responses. It would also mean logged-in users don't enjoy caching of these assets.

WMF's Varnish configuration "strips cookies" in so far that they ignore a lot of variability for non-pageview URLs so that when you're logged in, the load.php requests look as if they came from a logged-out user (more or less).

The useformat=mobile query parameter is fairly harmless as it naturally varies the cache. The mobileaction=toggle_view_desktop links ("Switch to desktop mode", from mobile) could also be fairly harmless if they only issued a redirect and did so from supported/reasonable contexts only.

In actually, those switch links are sticky. That seems sensible from a UX perspective and with some effort could probably be done in a way that's not broken. For example, with a dedicated cookie specific to the redirect handling, only considered when and where such a redirect would normally be about to occur. It seems this was actually implemented, there is a stopMobileRedirect cookie that appears to work in this way and makes sense.

Unfortunately, that's not all. There appears to also be a mf_useformat cookie. It seems to duplicate the purpose of stopMobileRedirect, and extend it with a more flexible ability that works in both directions. That could still be safe if it was limited to informing whether and where to redirect. Or if it was limited to non-WMF cases where there is no CDN, to make it all work on the same domain without redirects.

But instead, that mf_useformat cookie is indeed blindly read on all production traffic by MobileFrontend and informs the return value of shouldDisplayMobileView() and then does so without informing Varnish or MediaWiki of the fact that it just inspected a cookie and influenced how the web server responds to this request (e.g. it does not issue a Vary header), which means in your example Varnish is saving the response and returning it to other users regardless of whether the mf_useformat cookie is set or which value it has.

As said, load.php requires that responses only header by site (taken as domain, which means MF works for WMF currently) + module + (skin, lang debug). Even if MobileFrontend issues the Vary header correctly, you would likely still see this issue when other layers within MW or ResourceLoader cache by this fragmentation.

Yes, I know how this mf_useformat cookie works, because on my site there's no mobile domain, and I rewrite all URLs internally based on this cookie, giving users the same URL on both desktop and mobile but having only 2 versions of the page in varnish cache (mobile and desktop). This allows users share a link to the page, and users opening that link will see a consistent page according to their device (WMF approach here is terrible). My Varnish is configured to strip all cookies for load.php URLs, and that's how I found this bug.

Before MediaWiki 1.35, MobileFrontend had a separate module for mobile, and things were consistent. With the removal of this module, and relying on the common "styles" RL module that changes depending on a cookie, I have no control now about this in Varnish. Even if the URL includes the skin, it's not useful when you can switch skins while being logged-in and also view the site from a mobile device (which triggers the MobileFrontend transformations). The most sensible solution would be to have distinct URLs for mobile, and adding target=mobile was good for this. However, your team have deprecated this parameter and mobile distinction.

I understand why you want to remove the target system, but I think the whole concept of MobileFrontend without that parameter, while working on WMF wikis, is conceptually wrong. MobileFrontend does changes in the contents of pages (not only skin) and there's no reliable way to tell whether MobileFrontend transformations are being applied on the page or not, at least from Resource Loader modules.

I'm refactoring and moving my styles from MediaWiki:Mobile.css to MediaWiki:Common.css, skin styles and template styles, as Jdlrobson suggested, which I think it makes sense. However, this is a rather painful move, and still there are several styles from Common.css that I'd like to avoid loading on mobile altogether. Maybe I'll have to dig into moving them in gadgets loaded conditionally from JavaScript (since the target system is going to disappear from there too).

@Ciencia_Al_Poder That's a nice approach. Fragmenting the cache (explicitly) by the cookie rather than (implicitly, like WMF) by an m-dot domain makes a lot of sense. I hope we'll do something similar like that when we take on T214998.

Apologies if this if the following is obvious or already covered, but... would it make sense to include load.php in your cache fragmentation for your mobile detection cookie logic? It seems like that's semanticallly closest to what we have and would address your needs, I think. (It also will be more stable than what we currently use..., which we need to fix by either improving our VCL code or by not reading the cookie in MF for WMF via a config flag or some such)