Last modified: 2012-05-03 02:42:43 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 T36538, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 34538 - DOM modification at module execute versus $wgResourceLoaderExperimentalAsyncLoading
DOM modification at module execute versus $wgResourceLoaderExperimentalAsyncL...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.20.x
All All
: High normal (vote)
: 1.19.0 release
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: 31217 34450
  Show dependency treegraph
 
Reported: 2012-02-20 08:59 UTC by Tim Starling
Modified: 2012-05-03 02:42 UTC (History)
4 users (show)

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


Attachments
Flush and sleep in skins/Vector.php to give time for RL to finish before the main document finishes loading (439 bytes, patch)
2012-02-20 08:59 UTC, Tim Starling
Details

Description Tim Starling 2012-02-20 08:59:43 UTC
Created attachment 10048 [details]
Flush and sleep in skins/Vector.php to give time for RL to finish before the main document finishes loading

With $wgResourceLoaderExperimentalAsyncLoading enabled, modules are loaded from the <head> rather than from the end of the <body>. This causes the modules to be loaded simultaneously with the main HTML, and under certain conditions, module execution may occur before the #mw-panel element appears in the DOM. 

ext.vector.collapsibleNav in particular does not register a $(document).ready() callback, it just tries to modify the DOM at the file level of the module. No errors are logged, $( '#mw-panel' ) just returns an empty list and the styling is not applied. Other modules may have similar issues with $wgResourceLoaderExperimentalAsyncLoading. 

Bug 34450 may possibly be related but there were a lot of issues mixed in there without any reproduction procedure so I decided to file a new bug. 

Whether or not you see the issue depends on the browser, page length, network speed and cache timing details, but it's possible to reliably reproduce it by slowing down page delivery with the attached flush/sleep patch, and setting MW_NO_OUTPUT_BUFFER in LocalSettings.php:

define( 'MW_NO_OUTPUT_BUFFER', 1 );

Using that method, I can reproduce it every time in Firefox or Chrome.
Comment 1 Roan Kattouw 2012-02-20 21:29:07 UTC
(In reply to comment #0)
> ext.vector.collapsibleNav in particular does not register a $(document).ready()
> callback, it just tries to modify the DOM at the file level of the module. No
> errors are logged, $( '#mw-panel' ) just returns an empty list and the styling
> is not applied. Other modules may have similar issues with
> $wgResourceLoaderExperimentalAsyncLoading. 
> 
Are you sure you were looking at an up-to-date version of the code when you wrote this? I fixed this in https://www.mediawiki.org/wiki/Special:Code/MediaWiki/111685 four days ago and got Reedy to deploy it.
Comment 2 Tim Starling 2012-02-20 21:59:39 UTC
Oh.
Comment 3 Tim Starling 2012-02-21 00:43:48 UTC
I found this in a lot more places, so I'm going to call this a MediaWiki issue rather than a Vector issue. I'm currently in the process of auditing all extension JavaScript, and I'm referencing this bug report in my commit messages. Probably nothing needs to be changed in RL itself, except perhaps adding a debugging feature to make failures a bit more obvious.
Comment 4 Krinkle 2012-02-23 23:36:25 UTC
To clarify, this bug is more of a general todo for many modules to fix up bad code, not a tangible solvable bug in itself.
Comment 5 Tim Starling 2012-02-23 23:46:07 UTC
(In reply to comment #4)
> To clarify, this bug is more of a general todo for many modules to fix up bad
> code, not a tangible solvable bug in itself.

Kind of. Either many extensions magically acquired a bug when the core was changed, or there is a b/c break in core which is a bug. So this is either a tracking bug for large numbers of extension bugs, or a bug report about a b/c break. You be the judge.

Calling it a todo would imply that the modules aren't broken, but they are.
Comment 6 Krinkle 2012-02-23 23:53:34 UTC
(In reply to comment #5)
> Calling it a todo would imply that the modules aren't broken, but they are.

Yeah, 'todo' was definitely a bad word choice. By all means, they are valid breakages. But it's also true that code manipulating a dom element from a script that isn't hardcoded inline, should really be in a document-ready wrapper. And fortunately most code is doing that right. Either because they were ported from before ResourceLoader (at which point all was loaded from the <head> and without document-ready hook it wouldn't work at all), or because it was written in the past year and done "right" right away.

It's also unfair to blame the extensions, since it was reliable to assume that modules not loaded on 'top' would be loaded from the 'bottom'[1], and as such execute after the document is ready.

The module queue was, however, split up to allow certain modules to load before the page is rendered, not the other way around (eg. to make sure code is executed after the page is rendered), but regardless it has happened (also in modules written by experienced developers like Roan, Trevor and myself).

But it's also fairly safe to assume that nothing is going to be implemented in core to mitigate it. It doesn't make sense to delay a module's executing by force. A dom-ready wrapper is a trivial fix.

Speaking of which, are there any instances of this problem left that we know of?
Comment 7 Krinkle 2012-02-23 23:54:55 UTC
[1] 'top' and 'bottom' are terrible names for their queues, for so many reasons. more accurate would be 'blocking before page render' and 'non-blocking as soon as possible'.
Comment 8 Tim Starling 2012-02-24 00:03:40 UTC
(In reply to comment #6)
> Speaking of which, are there any instances of this problem left that we know
> of?

No, but I only reviewed about half of the extensions before I ran out of time, and I found bugs in 9 of them, so I'd expect there to be another 9 broken extensions waiting to be found.
Comment 9 Tim Starling 2012-02-27 22:32:57 UTC
Fixes should be done in all extensions now, marking resolved. It's possible I missed one or two, and there were some non-RL extensions that would need to be converted to RL with care, if they are ever converted. If there are more problems discovered, this bug can be reopened.

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


Navigation
Links