Page MenuHomePhabricator

After the document is ready mw.loader is broken (calls callback before module is parsed)
Closed, ResolvedPublic

Description

Author: mdale

Description:
Once the document is ready the resource loader does script loading asyncronusyly. The implement call seems to only work in syncronus mode.

In preparing for a patch ran into the following:

The is document ready flow is repeated twice it should be a local function.

Additionally their is shadow overcast scope of the "ready" variable with the .using function can be fixed by renaming.

patch attached.


Version: unspecified
Severity: critical

Details

Reference
bz27023

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 11:17 PM
bzimport set Reference to bz27023.

mdale wrote:

mediawiki pre-patch for loading issue

fixes function shadow of 'ready' variable, abstracts the document ready check for script appending

Attached:

mdale wrote:

I am bumping this to critical ( with hopes that it becomes blocking )... I believe Trevor is working on it now. Its documented as a supported use case of the resource loader that is broken. If not going to be blocking for a targeted release, then the resource loader should at least throw an exception or error if you try to load anything after the DOM is ready rather than issue the 'ready' callback.

Marking as blocker to bug 26611 for discussion

@mdale, is document.write() safe to use in that location ? As far as I know, you shouldn't never use it other than during inline script execution...

mdale wrote:

Setting loader.using to be only pre-dom ready would be bad. The mediaWiki code documentation that says the callback should be issued once the modules are 'ready' and the code includes stubs for checking if it can do a document.write vs a script append. As mentioned above if not supported it should fire an error. But feature wise its something that /should/ be supported, else people will have to wrap the resource loader functionality with ugly hacks anytime they want to load after DOM is ready. If random client code becomes responsible manually loading decencies post DOM ready it will get messy and difficult to avoid re-loading the same module twice.

There are lots of reasons you may want to load dependencies after the DOM is ready. Like a gadget that searches the page for a particular dom id or css class or config, or sub-components invoked on user interaction like an add-media-wizard pulling in an 'upload wizard' interface when you click the 'upload' button on edit pages ( without loading all of this every time you visit the base edit page ) or anytime you have asynchronous build out of components with dynamic dependencies ( ie checking the API or DOM some information that affects the set of interfaces that you load for that page ...
or simply for performance reasons you may want to display the page before every possible user interface and sub component is loaded and blinded.

Any news on this since it is blocking the tarball?

mdale wrote:

I have sent Trevor some more info about the bug, how to recreate it etc.

Hm, Michael, I thought script.onload didn't work on IE? I thought you needed to
use script.onreadystatechange on IE. See also the implementation of jQuery.ajax (line 5075).

Also, could you explain what the $.globalEval issue is that makes you avoid using jQuery.getScript in the first place?

mdale wrote:

Lupo your right. Older versions of IE need onreadystatechange furthermore IE sometimes fire the onreadystatechange event before the script has been fully parsed. So you get symbol undefined errors when you directly run the callback.

jQuery GlobalEval address this issue, but makes debugging more tricky, since you don't always get line numbers for syntax errors.

These issues point to either have per-browser debug mode hacks, or going back to loading resources through the resource loader in debug mode ( so that it can append a ready callback call. That way you can use script append ( without globalEval ) and you can get line numbers for errors.

In the old resource loader I had every class you load named by what it defines, so it would check that the target variable was defined before issuing the callback, ( letting me use script append and not worrying about onload / globalEval issues )... But that of course won't work here, and in general applies too much constraint to resource definitions.

mdale wrote:

So this has partially be solved in trunk by the use of $('body').append( script ); call which issues a synchronous / blocking xhr request and evals the result. Causing the mediawiki.loader.using to block on the loading of scripts and issue the callback at the right time.

This of course is not fun because it blocks all other script execution while the module set is being loaded ( both in debug and non-debug modes ) and when in debug mode is makes it nearly impossible to "debug" since it evals all the scripts that it appends to the document giving syntax errors in the parent script include line instead of the script error line. Trevor and I are scheduled to review a solution to these issues on Monday.

mdale wrote:

Trevor, I had an idea for improved debug mode post dom ready without timeout hacks waiting for parsing and without synchronous XHR eval script execution.

We should include all the scripts by normal document append and instead of waiting for the inconsistently fired "onload" "onreadystatechange" without the files being full parsed, We do normal script appends and after each script we append a separate resource loader "callback" script include. With IE we set the defer attribute to true for the special "callback" script and all the other ~sane~ browsers will execute in the order appended to the dom and issue the onload callback once the script is actually parsed.

This way we retain line numbers for syntax errors and direct reference to raw resources without doing too much of a hack for IE.

Sounds clever. We need to get together to make this happen.

We deployed with this bug, so I don't think it should be a tarball blocker.

Lowering priority so I don't bother with this any more until you two get a chance to fix it.

*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*

mdale wrote:

the globalEval issues discussed in this thread were resolved with a something similar to the patch in this head of this bug applied in r87986

QUnit test suite now contains a test specifically for this.

Link to test:

http://localhost/mediawiki/trunk/phase3/resources/test/?filter=mediawiki.js%3A%20mw.loader

Please re-open if still failing, preferably with a test case to reproduce this in a neutral environment (ie. QUnit)

mdale wrote:

Yes this should be fixed now, the test case looks good.