Last modified: 2014-07-24 14:59:04 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T64602, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 62602 - Global JS module version not updating properly
Global JS module version not updating properly
Status: VERIFIED FIXED
Product: MediaWiki extensions
Classification: Unclassified
GlobalCssJs (Other open bugs)
unspecified
All All
: Unprioritized normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: javascript
Depends on:
Blocks: 57891
  Show dependency treegraph
 
Reported: 2014-03-13 14:04 UTC by Helder
Modified: 2014-07-24 14:59 UTC (History)
10 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Helder 2014-03-13 14:04:35 UTC
I created a global.js page at
http://meta.wikimedia.beta.wmflabs.org/w/index.php?oldid=551
and it works when I visit any of these pages in debug mode
http://en.wikipedia.beta.wmflabs.org/wiki/Special:BlankPage?debug=1
http://meta.wikimedia.beta.wmflabs.org/wiki/Special:BlankPage?debug=1

BUT the global code is not loaded if I'm not in debug mode:
http://en.wikipedia.beta.wmflabs.org/wiki/Special:BlankPage
http://meta.wikimedia.beta.wmflabs.org/wiki/Special:BlankPage

I tested using Google Chrome 33.0.1750.149 and Firefox 27.0.1.

On Chrome I tried CTRL+F5, SHITFT+F5, CTRL+SHIFT+R, and also with the option "Disable cache (while DevTools is open)", and nothing worked.

On
http://en.wikipedia.beta.wmflabs.org/wiki/Special:BlankPage
Google Chrome shows the following link in the Sources panel:
http://bits.beta.wmflabs.org/meta.wikimedia.beta.wmflabs.org/load.php?debug=false&lang=en&modules=ext.globalcssjs.user&skin=vector&user=Helder.wiki&version=20140310T100305Z&*

Its content is:
mw.loader.implement("ext.globalcssjs.user",function(){},{},{});
/* cache key: metawiki:resourceloader:filter:minify-js:7:25e245bf16e0037a7248f80fd384e529 */

If I remove the "&version=20140310T100305Z&*" from the URL, the content changes to
mw.loader.implement("ext.globalcssjs.user",function(){$(function(){$('#mw-content-text').prepend('<strong style="color: #cc0000; font-size: 150%;">Your <a href="//meta.wikimedia.beta.wmflabs.org/w/index.php?title=Special:MyPage/global.js&action=edit">global scripts</a> were loaded!</strong>');});someTest1='global.js';var someTest2='global.js';window.someTest3='global.js';;},{},{});
/* cache key: metawiki:resourceloader:filter:minify-js:7:101b9d1a9e616e1315c94ace38794a15 */

Is this a problem in the extension? Or maybe it needs some configuration related to Resource Loader?
Comment 1 Helder 2014-03-13 14:08:24 UTC
Ops... I was doing some other tests, so the last example has a few someTest* variables which were not in the version of global.js I linked. The correct example is this: If I remove the "&version=20140310T100305Z&*" from the URL, the content changes to
mw.loader.implement("ext.globalcssjs.user",function(){$(function(){$('#mw-content-text').prepend('<strong style="color: #cc0000; font-size: 150%;">Your <a href="//meta.wikimedia.beta.wmflabs.org/w/index.php?title=Special:MyPage/global.js&action=edit">global scripts</a> were loaded!</strong>');});;},{},{});
/* cache key: metawiki:resourceloader:filter:minify-js:7:94d6fa5683df6cab0cdc82575eb686ba */
Comment 2 MZMcBride 2014-04-05 18:36:57 UTC
Lego: thoughts on this bug?
Comment 3 Helder 2014-04-09 13:35:08 UTC
So, I haven't touched that JS page for ~4 weeks and now, when I access any of
http://meta.wikimedia.beta.wmflabs.org/wiki/Special:BlankPage
http://en.wikipedia.beta.wmflabs.org/wiki/Special:BlankPage
the global JS is loaded.

But then I decided to check how it is dealing with updates: I made a small change in the message my global code was displaying
http://meta.wikimedia.beta.wmflabs.org/w/index.php?diff=584

* After the page was saved and finished reloading, the old message was still displayed
* CTRL+SHIFT+R, SHIFT+Reload and CTRL+F5 were not enough to force the message to be updated (both on meta.wikimedia.beta.wmflabs.org and on en.wikipedia.beta.wmflabs.org).
* The new message was immediately visible in debug mode, on these pages:
http://meta.wikimedia.beta.wmflabs.org/wiki/Special:BlankPage?debug=1
http://en.wikipedia.beta.wmflabs.org/wiki/Special:BlankPage?debug=1
but the old one would still show up when I removed the "?debug=1" from these URLs.
Comment 4 MZMcBride 2014-04-11 01:43:22 UTC
Helder: What happens when you update a non-global CSS/JS page (such as /monobook.js or /vector.css or whatever)? The caching issue you're having may be specific to Beta Labs, not the GlobalCssJs extension.
Comment 5 Helder 2014-04-11 02:01:23 UTC
(For the record, right now I see the most recent version of the script I mentioned in the test from comment 3. So it doesn't need ~1 month to be updated)

A made a similar test in my common.js and my vector.js
http://meta.wikimedia.beta.wmflabs.org/w/index.php?diff=587
http://meta.wikimedia.beta.wmflabs.org/w/index.php?diff=588
and the new code was executed as soon as the page finished reloading, after pressing the save button.

But on global.js it still doesn't works:
http://meta.wikimedia.beta.wmflabs.org/w/index.php?diff=589
Comment 6 Krinkle 2014-04-11 17:24:48 UTC
(In reply to Helder from comment #3)
> * After the page was saved and finished reloading, the old message was still
> displayed
> * CTRL+SHIFT+R, SHIFT+Reload and CTRL+F5 were not enough to force the
> message to be updated (both on meta.wikimedia.beta.wmflabs.org and on
> en.wikipedia.beta.wmflabs.org).
> * The new message was immediately visible in debug mode, on these pages:
> http://meta.wikimedia.beta.wmflabs.org/wiki/Special:BlankPage?debug=1
> http://en.wikipedia.beta.wmflabs.org/wiki/Special:BlankPage?debug=1
> but the old one would still show up when I removed the "?debug=1" from these
> URLs.


Doing all that cache clearing in your browser won't do anything because it's cached server-side. The urls are unique by version, and those are permanently cached. Then, once a content update is detected (within 5 minutes using the startup module) you get a new url which naturally bypasses cache because it's a new url.

I'd recommend you try making another edit, then don't clear anything, wait for upto 2*5 minutes and verify it does indeed not automatically update.

If after 10 minutes it is still giving the old version, then we've found a bug in GlobalJsCss related to cache versioning, presumably its logic to build a timestamp is not working.
Comment 7 Helder 2014-04-11 18:34:47 UTC
But why would it update instantaneously for common.js and vector.js (which is what I expect) but take more than 10 minutes for global.js?

It has been more than 10 minutes since my last edit to global.js and the JS is not updated.
Comment 8 Kunal Mehta (Legoktm) 2014-04-12 07:19:06 UTC
Ok, so I've tested this a bit, and can easily reproduce this on beta labs, but the version (and cache) updates properly on my local vagrant instance.

(In reply to Krinkle from comment #6)

> If after 10 minutes it is still giving the old version, then we've found a
> bug in GlobalJsCss related to cache versioning, presumably its logic to
> build a timestamp is not working.

The code path it uses to come up with the version number is the same as ResourceLoaderWikiModule, except it makes a call to a foreign db since we override $this->getDB.

I played around with eval.php on beta labs and traced that code path through, and everything worked as expected even with the remote database, so I'm really not sure what isn't working.
Comment 9 Ori Livneh 2014-04-18 05:23:32 UTC
The URLs for loading the user and user.group modules is constructed in PHP, in lines 2906 - 2929 of OutputPage.php (@829886b10a). (The modules are not versioned via the startup module and are not fetched by the JavaScript-based module loader.)  You'll most likely need to find a way to include the global user scripts in that URL. (You could declare them as dependencies of the user module, though that'd be a bit evil.)
Comment 10 Gerrit Notification Bot 2014-04-19 23:01:20 UTC
Change 127457 had a related patch set uploaded by Legoktm:
Add OutputPageScriptsForBottomQueue hook

https://gerrit.wikimedia.org/r/127457
Comment 11 Kunal Mehta (Legoktm) 2014-04-19 23:07:19 UTC
I looked into turning it into a dependency, but I don't think that would be possible since the 'user' module is registered before GlobalCssJs would have a chance to alter it.

Ica631fd0639c13b937be2d338b9a3868d0b8e3f7 adds a hook at the same point where the user and site modules are loaded, allowing extensions to add their own modules at that point. The downside of doing it this way is that it would require an additional request client-side to load the global modules.
Comment 12 Gerrit Notification Bot 2014-04-20 06:39:02 UTC
Change 127457 abandoned by Legoktm:
Add OutputPageScriptsForBottomQueue hook

https://gerrit.wikimedia.org/r/127457
Comment 13 MZMcBride 2014-04-20 06:43:26 UTC
From <https://gerrit.wikimedia.org/r/127457>:

---
As discussed on IRC, setting group => user and position => top in the resource definition will have the same effect without requiring a hook.
---
Comment 14 Gerrit Notification Bot 2014-04-20 06:46:58 UTC
Change 127461 had a related patch set uploaded by Legoktm:
Set group=>user and position=> top for ext.globalCssJs.user

https://gerrit.wikimedia.org/r/127461
Comment 15 Ori Livneh 2014-04-25 17:58:21 UTC
From <https://gerrit.wikimedia.org/r/127457> again:

---
Timo, the reason for position => top is to be found in OutputPage::getHeadScripts. To ensure the module version is always up-to-date, the load.php link must be generated in PHP (rather than JavaScript, which depends on the version set in the startup module). And getHeadScripts will only generate links for modules in the bottom load queue if  $wgResourceLoaderExperimentalAsyncLoading is set.
IMO, the proper way to fix this is to make getHeadScripts generate an inline mw.loader#register script in <head> just below the startup module for all private / user modules. It would override the version set in the startup module, which would allow us to let JavaScript code construct the load.php URLs.
(This is already kinda happening: if you update the user JS you'll notice that the version for the user module in the startup module takes the usual time to update but that doesn't matter because the load.php URL changes instantly to reference the correct version. Though we might simply want to modify ResourceLoaderStartupModule so that it doesn't register private / user modules at all.)
@Legoktm: I thought I would be OK with merging this because I figured that the only issue posed by position => top would be a performance hit, and I am confident we can mitigate it using the strategy I described above. But there's a bigger worry, which is that when we switch these modules to the bottom queue a whole bunch of global scripts could break.
I think we can use MassMessage as a test-case before modifying core. So what I would suggest is this: add a new ResourceLoaderModule subclass called something like 'ResourceLoaderUncachedRegistrationModule'. Set its group to 'private' and its position to 'top', so that it gets inlined. The module's getScript function should consist of a call to ResourceLoader::makeLoaderRegisterScript( 'ext.globalCssJs.user' ).
This would mean that even if the version of the ext.globalCssJs.user module specified in the startup module is stale, it would get overridden by the correct version in the inline script just below. You would still be setting group => user on ext.globalCssJs.user, but not position => top.
---
Comment 16 Krinkle 2014-06-18 06:49:38 UTC
Okay, there's tons of messages back and forth and implementation has changed many times.

A fresh brain dump:

1) The startup module can't be relied upon for user specific modules. We already don't do this for the (local) 'site' and 'user' modules either for they are called directly from the page html and (for 'user') *with* a version timestamp:

view-source:https://www.mediawiki.org/

<script src="//bits.wikimedia.org/www.mediawiki.org/load.php?debug=false&amp;lang=en&amp;modules=site&amp;only=scripts&amp;skin=vector&amp;*"></script>
<script src="//bits.wikimedia.org/www.mediawiki.org/load.php?debug=false&amp;lang=en&amp;modules=user&amp;only=scripts&amp;skin=vector&amp;user=Krinkle&amp;version=20140617T190748Z&amp;*"></script>
</body></html>

2) I'm not sure where this inline registration comes from. It looks like a hack and I forgot what that is supposed to work around. Whatever the reason is, the way it is proposed in patch set 3 of Ieb6cfe2bf9a86a4 looks wrong because it doesn't compute a module version (it passes null), which means that either it is broken or makes a request without a version in the url query, which means the request will be cached between 5 and 30 minutes and not invalidated when you want it to.

3) I'd recommend simply going for the same approach as for local 'user'. Make the <script> and <link> directly. Have a ResourceLoaderModule class that retrives the style/scrript from the foreign database and compute a valid module version (happens automatically when extending ResourceLoaderWikiModule), and generate a load.php request inside the page html to the foreign wiki including &version= in the url. That way it is cached for 30 days and yet immediately invalidated when the module changes (for the 'version' value will no longer be the same then).
Comment 17 Gerrit Notification Bot 2014-06-22 08:55:15 UTC
Change 141282 had a related patch set uploaded by Legoktm:
Add OutputPageScriptsForBottomQueue hook

https://gerrit.wikimedia.org/r/141282
Comment 18 Gerrit Notification Bot 2014-06-22 09:02:22 UTC
Change 141284 had a related patch set uploaded by Legoktm:
Use new OutputPageScriptsForBottomQueue hook

https://gerrit.wikimedia.org/r/141284
Comment 19 Kunal Mehta (Legoktm) 2014-06-22 09:04:57 UTC
(In reply to Krinkle from comment #16)

> 3) I'd recommend simply going for the same approach as for local 'user'.
> Make the <script> and <link> directly. Have a ResourceLoaderModule class
> that retrives the style/scrript from the foreign database and compute a
> valid module version (happens automatically when extending
> ResourceLoaderWikiModule), and generate a load.php request inside the page
> html to the foreign wiki including &version= in the url. That way it is
> cached for 30 days and yet immediately invalidated when the module changes
> (for the 'version' value will no longer be the same then).

Ok, that sounds good. I uploaded a patch to add a core hook to let us add a module there, and another updating GlobalCssJs to use it.
Comment 20 Gerrit Notification Bot 2014-07-20 01:23:31 UTC
Change 141282 merged by jenkins-bot:
Add OutputPageScriptsForBottomQueue hook

https://gerrit.wikimedia.org/r/141282
Comment 21 Gerrit Notification Bot 2014-07-20 01:24:25 UTC
Change 127461 abandoned by Legoktm:
Create ResourceLoaderUncachedRegistrationModule

https://gerrit.wikimedia.org/r/127461
Comment 22 MZMcBride 2014-07-21 02:50:59 UTC
(In reply to Gerrit Notification Bot from comment #18)
> Change 141284 had a related patch set uploaded by Legoktm:
> Use new OutputPageScriptsForBottomQueue hook
> 
> https://gerrit.wikimedia.org/r/141284

For anyone following the bug, I believe this is the remaining hurdle to clear.
Comment 23 Gerrit Notification Bot 2014-07-23 22:50:23 UTC
Change 141284 merged by jenkins-bot:
Use new OutputPageScriptsForBottomQueue hook

https://gerrit.wikimedia.org/r/141284
Comment 24 Kunal Mehta (Legoktm) 2014-07-23 22:58:05 UTC
Fixed now, thanks to everyone who helped with this! Can be tested on beta labs (in a few minutes probably) or http://legoktm.wmflabs.org/.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links