Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#8976 closed defect (fixed)

FF3: dojo.addOnLoad can run before dojo.require finishes

Reported by: Douglas Hays Owned by: James Burke
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0b3
Keywords: Cc: Adam Peller, alex
Blocked By: Blocking:

Description

Several customers have been having random timing problems with dojo.addOnLoad running before dojo.require finishes and their web app then dies. I've been telling customers who want to load certain widgets (based on user input) after the initial page loads to do a normal dojo.require and then instantiate the widget inside a dojo.addOnLoad. This seems to work almost all the time. But randomly this fails. I think I've been able to recreate it on at least Firefox 3 (WinXp? and Mac). The customer's server generates 1 script tag containing the dojo.require and a separate script tag containing the dojo.addOnLoad call. This split script structure seems to be an important component. I've tried to recreate this by attaching the require call to onmousedown and the addOnLoad to onmouseup and then having the user click the associated button. I've set up the testcase here - just run wiht FF3 and click the button, an Exception should popup:
http://doughays.dojotoolkit.org/doctype/asyncbug.html

Attachments (8)

asyncbug.html (854 bytes) - added by Douglas Hays 10 years ago.
testcase that runs dojo.addOnLoad() immediately after dojo.require()
DOMContentLoaded.diff (716 bytes) - added by alex 10 years ago.
possible fix
8976.patch (664 bytes) - added by James Burke 10 years ago.
Possible fix for the issue, using _inFlight in normal loader paths.
8976.2.patch (1.5 KB) - added by Douglas Hays 10 years ago.
tweaked patch to call _callLoaded from _loadUri since the last addOnLoad is queued but no one else is listening
8976.3.patch (2.1 KB) - added by Douglas Hays 10 years ago.
new patch that delays an addOnLoad() until after multiple sequential require()s
8976.4.patch (2.1 KB) - added by James Burke 10 years ago.
Like patch 3, but not using dojo.hitch
8976.5.patch (2.6 KB) - added by Douglas Hays 10 years ago.
Like patch 4, but uses "d" instead of "this" and added better comments to explain need for setTimeout
asyncbug.2.html (923 bytes) - added by Douglas Hays 10 years ago.
attaching test file for archival purposes that works with patch 5 but fails with patch 2 (without setTimeout)

Download all attachments as: .zip

Change History (33)

Changed 10 years ago by Douglas Hays

Attachment: asyncbug.html added

testcase that runs dojo.addOnLoad() immediately after dojo.require()

comment:1 Changed 10 years ago by dante

this sounds like the FF/DomContentLoaded/ExecuteScriptsOutOfOrder issue that has been causing pain since 3.0.2ish, fwiw. Allegedly fixed in 3.0.6, though getting reports intermittently of being broken still.

comment:2 Changed 10 years ago by Douglas Hays

I'm testing on 3.0.7 fwiw - is there aother ticket for this?

comment:3 Changed 10 years ago by alex

Cc: alex added

I suggest we just disable DOMContentLoaded entirely on Firefox. It has been a continual source of end-user frustration, and it's just not worth risking FF's janktastic networking behavior any more.

Changed 10 years ago by alex

Attachment: DOMContentLoaded.diff added

possible fix

comment:4 Changed 10 years ago by Adam Peller

hang on, I'm not sure that's the problem here. In Doug's example, the dojo.requires happen as the result of a user action in the body. I think djConfig.afterOnLoad is what he needs. There are no dojo.requires in the head, so addOnLoad fires right away, as it should.

comment:5 Changed 10 years ago by Douglas Hays

I added afterOnLoad:true inside my dojo.js script tag, and the page locks up with the hourglass mouse forever and my web page blanked out. The test with that boolean set is here: http://doughays.dojotoolkit.org/doctype/asyncbug2.html.

comment:6 Changed 10 years ago by James Burke

There is a new bug filed related to script loading here: https://bugzilla.mozilla.org/show_bug.cgi?id=478277

The old one was here: https://bugzilla.mozilla.org/show_bug.cgi?id=444322

That said, as peller indicated, I do not believe it is a DOMContentLoaded issue, but a script loading issue (which may surface during DOMContentLoaded, but hopefully is much more rare now).

doughays: you do not need djConfig.afterOnLoad in this page: that is only useful for loading dojo.js after page load. What I think peller meant to say is:

If you do a dojo.require() as part of a user action/after page load, put the logic you want to execute after the dojo.require finishes in a dojo.addOnLoad() callback. This is required for xdomain loading. Technically it may not be needed if you are using async loading, but then you are also using a non-build layer.

In any case the "always follow dojo.require calls with a dojo.addOnLoad() callback for require calls after page load" advice should still be followed and is likely to fix the issue.

comment:7 Changed 10 years ago by Douglas Hays

I tried the DOMCOntentLoaded patch and it didn't seem to help this case.

jburke: does the addOnLoad after require need to be in the same script? The failing testcase does the addOnLoad in a separate script, but is technically executed after the require has already started, but does not wait for the require to finish.

comment:8 Changed 10 years ago by James Burke

doughays: Yes, I just tried your test page and doing the dojo.addOnLoad() after the dojo.require works in the same script block.

The original test case looks a bit unique/bizarre in that the dojo.addOnLoad() call happens onmouseup (and the on mousedown does the require). I think that is confusing things -- not sure how the dom event callbacks get interleaved with the networking stuff/dojo.require call.

comment:9 Changed 10 years ago by Douglas Hays

The customer claims to have a requirement to have separate script blocks due to the way the code is being generated from fragments. So is the separate blocks for require and addOnLoad explicitly forbidden, or just strange but should work? I suppose that the crux of this ticket.

comment:10 Changed 10 years ago by Douglas Hays

So the separate script blocks ended up being an unintended diversion. I changed the testcase to have 2 script blocks, each with require+addOnLoad and the exception still happens. Mouse down on the button for approx 1/2-1 second, then release. The exception still appears. This is really just a variation on the original testcase but bypasses the split script block discussion. http://doughays.dojotoolkit.org/doctype/asyncbug3.html

comment:11 Changed 10 years ago by Douglas Hays

I'm not sure if this is contributing to the problem or not, but it looks like dojo.provide marks the module as loaded (even though the prereq modules have not been downloaded yet), and so the 2nd script block's dojo.require looks the module up in the array and just returns since it appears the module is fully loaded, then its addOnLoad happily proceeds. It looks like the 2nd block's addOnLoad should have been blocked by some inFlight counter.

comment:12 Changed 10 years ago by Douglas Hays

The customer verified that they are not using a build. Does this affect the inFlight checking?

comment:13 Changed 10 years ago by Douglas Hays

A testcase with just a single large file downloaded: http://doughays.dojotoolkit.org/doctype/asyncbug4.html

comment:14 Changed 10 years ago by Douglas Hays

I had to clear my cache after each run to see the problem consistently, but asyncbug4 seems to fail on both FF2.0.0.20 and FF3.0.7.

Changed 10 years ago by James Burke

Attachment: 8976.patch added

Possible fix for the issue, using _inFlight in normal loader paths.

comment:15 Changed 10 years ago by James Burke

Milestone: tbd1.4
Owner: changed from anonymous to James Burke

The issue seems to be that the second script block/second script execution takes place before the dojo.require from the first one finishes its work. dojo.addOnLoad checks for:

if(d._postLoad && d._inFlightCount == 0 && !d._loadNotifying){
    d._callLoaded();
}

These conditions are all true when the second addOnLoad comes it so it fires the addOnLoad callbacks.

For sync xhr loading, it does not set the _inFlightCount, that is used in the xdomain loader when we have a better idea of when all the modules load. However, I think we can simulate the effect by just setting _inFlightCount to a positive value in dojo._loadUri, then when loadUri is done set it back to 0.

I attached a patch with this change. This seemed to fix the asyncbug.html and asyncbug4.html test cases (didn't try the others, those were the ones I had locally).

I think this patch should be fairly safe unless there is no content loaded by the dojo.require file, like maybe if dojo.require("some.thing", true) is used, there might be a problem, so something to check, but for the major use case it should be fine.

And for xd support, that version of dojo._loadUri is not used so I think it should be safe for the xd case, debugAtAllCosts case too. Hopefully I can push this change for the 1.4 release after checking the dojo.require("some.thing", true) case.

comment:16 Changed 10 years ago by Douglas Hays

I was able to figure out how the customer is creating a "multi-threaded" environment on FF. They do a lot of asynchronous xhrGet calls and the "load" callbacks can interrupt a synchronous xhrGet. So they kick off 4 asynchronous xhrGets, and each "load" callback does a dojo.require (synchronous) + addOnLoad. So they end up with 4 dojo.requires and 4 addOnLoads happening semi-concurrently.

comment:17 Changed 10 years ago by Douglas Hays

jburke: in the patch, why inFlight=0 and not inFlight-- to cancel the inFlight++ ?

comment:18 Changed 10 years ago by Douglas Hays

The original test asyncbug.html no longer throws an error partly because the addOnLoad function isno longer being run at all which is bad. There should be a console message that says tabcontroller = blahblah http://doughays.dojotoolkit.org/8976patch/asyncbug.html

Changed 10 years ago by Douglas Hays

Attachment: 8976.2.patch added

tweaked patch to call _callLoaded from _loadUri since the last addOnLoad is queued but no one else is listening

comment:19 Changed 10 years ago by Douglas Hays

conflicting results from 2nd patch: customer claims the added _calloaded() in _loadUri causes problems, and asyncbug.html needs it to work correctly.

Changed 10 years ago by Douglas Hays

Attachment: 8976.3.patch added

new patch that delays an addOnLoad() until after multiple sequential require()s

comment:21 Changed 10 years ago by Douglas Hays

1 customer reported that the problem seemed to be fixed with the 8976.3.patch, but another said the problem became much less frequent (which is good and bad) but still was present. My setTimeout/inFlightCount check may not be as bullet proof as I hoped but I couldn't figure out how to make a testcase that caused it to fail.

Changed 10 years ago by James Burke

Attachment: 8976.4.patch added

Like patch 3, but not using dojo.hitch

comment:22 Changed 10 years ago by James Burke

I did a small update to patch 3 to remove the dojo.hitch reference, and called it 8976.4.patch. dojo.hitch may not be available in the loader based on the kind of build done.

The patch looks like it is targeting the right area, not sure what else is needed without a test case. Although, I'm not sure we need the setTimeout call before calling dojo._callLoaded, since _callLoaded does its own setTimeout. I would prefer to remove that setTimeout in the _loadUri, but since doughays has access to the test cases/partners with the issue, we will wait to see what works for those scenarios.

Changed 10 years ago by Douglas Hays

Attachment: 8976.5.patch added

Like patch 4, but uses "d" instead of "this" and added better comments to explain need for setTimeout

Changed 10 years ago by Douglas Hays

Attachment: asyncbug.2.html added

attaching test file for archival purposes that works with patch 5 but fails with patch 2 (without setTimeout)

comment:23 Changed 10 years ago by Douglas Hays

Both customers have reported that patch 5 fixes their loader problems.

comment:24 Changed 10 years ago by Douglas Hays

Soliciting comments to loader.js patch 8976.5.patch...
The customers that I patched with this change haven't reported any problems in 2 months. I would suggest committing this soon into dojo trunk to give users a chance to exercise this code as much as possible.

comment:25 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [17694]) Fixes #8976: patch from doughays, thanks for the patch and test case.

Note: See TracTickets for help on using tickets.