Page MenuHomePhabricator

ResourceLoader: Don't add a version hash param in debug mode
Open, Stalled, MediumPublic

Description

As a workaround for T235667 and other possible cases where you might get a load.php URL while in debug mode.

In debug mode the cache control headers are set to no-cache, and many of the files served are direct paths to raw JS/CSS files. The same should apply to load.php URLs, as this would at least allow developers to set breakpoints without them being lost every time the hash changes (in the absence of source maps).

Event Timeline

Esanders created this task.Oct 16 2019, 4:16 PM

In debug mode the cache control headers are set to no-cache

I'm not 100% sure that they are, but if they're not, they should be. And in any case, dropping the version param will lower the cache time to 5 minutes.

// See RFC 2616 § 14.19 ETag
// https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.19
header( 'ETag: ' . $etag );
if ( $context->getDebug() ) {
	// Do not cache debug responses
	header( 'Cache-Control: private, no-cache, must-revalidate' );
	header( 'Pragma: no-cache' );
} else {
	header( "Cache-Control: public, max-age=$maxage, s-maxage=$smaxage" );
	$exp = min( $maxage, $smaxage );
	header( 'Expires: ' . wfTimestamp( TS_RFC2822, $exp + time() ) );
}
Esanders added a subscriber: Krinkle.

a version hash that changes the URL every time the file is modified

We could work around this by dropping the version param whenever packageFiles and debug mode are used. Raw files in debug mode don't get a version param, so this would be making the behaviour match that.

Raw files in debug mode don't get a version param [..]

Can you elaborate? I agree we should make breakpoints easy and sticky, but as I understand it, urls change with classic file modules and debug mode as well. E.g. https://en.wikipedia.org/wiki/Main_Page?debug=true loads https://en.wikipedia.org/w/extensions/Echo/modules/api/mw.echo.api.NetworkHandler.js?6dc51, etc.

Hmm, not for me:

Interesting, that seems like a bug. Possibly configuration related.
I don't have a preference for whether these urls have version parameters. However, note that their current presence in debug mode is not due to some generic code applying there. Rather, it was intentionally added to solve the problem outlined in T90983: ResourceLoader debug urls should bypass cache when they change. If we remove these, we'll have to re-solve that in a different way.
In a nut shell:

  • Static files from wgResourceBasePath are expected to have long-caching headers set at the web server level (depending on complexity of a dev setup, this is entirely optional, but it is something we do need to support in prod, in beta and when testing with MW-Vagrant with the Varnish module enabled).
  • In production we have multiple versions deployed at once from a single docroot, which means we need to disambiguate which version the current URL is for in a way that doesn't vary by hostname (all /w/resources and /static urls are cached in Varnish with host-agnostic cache key) . This applies mainly to font files and SVG icons.
  • Given raw debug files are served from the same directory, we need to cache-bust them, as well as to disambiguate. Using a content address/ hash param, solved both.

So it appears I am getting query strings for files in core, but not extensions. This might be because I have a non-standard directory structure (/extensions is a sibling folder of /core).

So I've been spared this annoying behaviour on all my files, due to a bug it seems.

That said adding breakpoints to files and expecting them to still be there when I make a small change seems like a pretty common workflow for devs.

It seems like removing the query string param, possibly using a separate config if required, would be a much simpler fix than implementing source maps.

Krinkle added a comment.EditedOct 16 2019, 6:00 PM

@Esanders Can you check whether it works in latest Chrome when setting a breakpoint in a raw core file (with its version hash), and whether it remains after changing its content? I suspect this might work actually, and that the issue is with packageFiles having "load.php" as its filename which is ambiguous in this context (vs raw files having each their own url where the parameter can be ignored). In particular because a similar issue mentioned at T90983#2083366 was fixed upstream in Chromium.

Removing version hashes from load.php-debug responses seems fine by me. Would require a small change in the JS client to omit that param. The headers are already set to no-cache. Doing it for "raw" files is difficult given we don't control the response headers (per my previous comment).

Gilles assigned this task to Krinkle.Oct 23 2019, 7:57 PM
Gilles moved this task from Inbox to Doing on the Performance-Team board.
Krinkle triaged this task as Medium priority.Oct 24 2019, 2:39 PM
Krinkle changed the task status from Open to Stalled.Mon, Feb 17, 7:57 PM
Krinkle removed Krinkle as the assignee of this task.
Krinkle moved this task from Confirmed Problem to Backlog on the MediaWiki-ResourceLoader board.