Page MenuHomePhabricator

ResourceLoader must bypass MemoizedCallable cache for CSSMin::remap when files change
Closed, ResolvedPublic

Description

While testing scenarios for T127208, I ran into this bug.

  1. Install MediaWiki core + Vector skin. Ensure APC is enabled and wgMainCacheType is set to something).
  2. Load a page with an external link (this warms up caches).
  3. Change Vector/images/external-link-ltr-icon.svg. For example by copying user-icon.svg to it.
  4. Reload the page with an external link

No matter how hard I try from the client-side, the stylesheet remains unchanged, with the same image url, with the same outdated sha1 hash in its query string, and with the same outdated embedded data uri.

This is most likely caused by the following code in ResourceLoaderFileModule::getStyleFile():

return MemoizedCallable::call( 'CSSMin::remap',
	[ $style, $localDir, $remoteDir, true ]
);

It uses the stylesheet as input, but makes the mistake that these variables result in a deterministic response. Unlike minification, remapping (which includes embedding and query-string augmentation) is not a pure function. Its effect depend on the local disk and the content of any referenced images.

The call is memoized for 1 hour which is too long. Either we need to bypass this when any of the referenced files changed (how?). Or we need to drastically reduce the cache expiry. I'm not sure how long would be safe. Probably less than a minute.

The call to CSSMin::getAllLocalFileReferences is not memoized (that one probably could be, though). The file module correctly incorporates all current files and their content hashes in the overall module version hash. Due to this being a low-level function call, it ends up polluting other caching layers as well (the response is headered with a new version hash, but contains partially stale contnet due to MemoizedCallable). (T47877)

Event Timeline

Krinkle created this task.Mar 2 2016, 11:38 PM
ori updated the task description. (Show Details)Mar 2 2016, 11:55 PM

Change 274604 had a related patch set uploaded (by Krinkle):
resourceloader: Don't cache CSSMin::remap() calls

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

Krinkle updated the task description. (Show Details)Mar 3 2016, 12:03 AM

Change 274604 merged by jenkins-bot:
resourceloader: Don't cache CSSMin::remap() calls

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

Krinkle moved this task from Inbox to Doing on the Performance-Team board.
Krinkle closed this task as Resolved.
Krinkle claimed this task.