453801 - [FIX]Deferred (defer) external javascript fails to launch window.onload() if external CSS has been loaded after the deferred script

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Mikko Rantalainen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.14eol) Gecko/20080715 Ubuntu/dapper-security Firefox/1.5.0.12eol
Build Identifier: Windows NT 5.1, Gecko/20080902033133 Minefield/3.1b1pre

See the attached test case

Reproducible: Sometimes

Steps to Reproduce:
1. Open the attached test case
2. Click the "link to self" link
3. Notice that alert() does not show up if the page comes from the cache
Actual Results:  
The alert() shows sometimes

Expected Results:  
The alert() dialog should appear after every click (the page should launch window.onload())

Tested on Windows XP only. I'm reporting the problem from another computer running Ubuntu 6.06 which cannot run Firefox 3 or any trunk version because of too old libraries.

Comment 1

[Mikko Rantalainen]

Attachment: External JS file for the test case (displays an alert() on window.onload())

Comment 2

[Mikko Rantalainen]

Attachment: The test case itself

May work or may not work through bugzilla. The test case links to external CSS file and it seems that the test case does not work if the external CSS file is requested with HTTPS protocol.

Comment 3

[Mikko Rantalainen]

Attachment: Fixed test case (with the correct external JS URL)

Comment 4

[Mikko Rantalainen]

The test case seems to fail if external JS file is requested with HTTPS protocol. I've setup a temporary test case at http://semyo329k.ktl.jyu.fi/test/test.xhtml for this. Just click the "link to self" link repeatedly and the alert() should popup only sometimes (some kind of race condition?).

Comment 5

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows XP, latest trunk.
Works: 2008080416
Fails: 2008080504
Caused by Bug 28293 ?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, i know what's going on here. As soon as the script finishes loading we remove it from the loadgroup. This means that onload can fire as soon as we reach the end of the page. However we won't execute the defered script until we're at the end of the page, which means that onload might fire before the defered script executes.

I think.

Though it's weird that this can happen since we execute defered scripts as soon as DOMContentLoads fires which I thought always fired before onload...

Comment 7

[Mikko Rantalainen]

Note that the test case required external CSS file to be loaded *after* the external deferred JS file to trigger the bug. Perhaps there's something wrong in case page is loading something else but JS file (which is not deferred) after the deferred JS?

Comment 8

[Mikko Rantalainen]

Any progress with this regression?

Comment 9

[Boris Zbarsky [:bzbarsky]]

DOMContentLoaded does in fact always fire before onload.  And the testcase seems to work for me with a current trunk build...

Comment 10

[Mikko Rantalainen]

I'm still able to reproduce at http://semyo329k.ktl.jyu.fi/test/test.xhtml with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081012 Minefield/3.1b2pre running on HP OmniBook 6000 laptop with a 550 MHz CPU.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, so the deal here is that we are NOT running this script from DOMContentLoaded.  What happens is that we parse the page, start the defer script load, start the stylesheet load, get to DOMContentLoaded...  At this point the stylesheet hasn't loaded yet, so we don't run the script (because the stylesheet load puts an execute blocker on the scriploader).  We _do_ fire DOMContentLoaded at this point, of course.

Then later we finish loading the stylesheet, and call RemoveExecuteBlocker(), which calls ProcessPendingRequestsAsync, which posts an event to run the script.  Immediately after that we remove the stylesheet request from the loadgroup, and since nothing else is in flight, we fire onload.  Then the event fires and we execute the script, but onload has already fired.

Jonas, would it make sense to either block onload on the event we post in ProcessPendingRequestsAsync or on the deferred script or some such?  Blocking it on the event seems to me like it would be pretty simple and reasonably safe: if we post such an event before onload has fired we shouldn't fire onload before any scripts that the event will trigger have executed, right?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, say

Comment 13

[Mikko Rantalainen]

I'm still able to reproduce at http://semyo329k.ktl.jyu.fi/test/test.xhtml with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081211 Shiretoko/3.1b3pre running on HP OmniBook 6000 laptop with a 550 MHz CPU.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yes.  That would be why this bug ins't marked fixed, right?

Comment 15

[Boris Zbarsky [:bzbarsky]]

That said, we have got to fix this in the next beta.  Jonas, any chance of reviews sometime?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I think this bug should be a P1, not a P2.  We desperately need web compat testing here...

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm sort of thinking that I'd prefer to add a onloadblocker when we add the first defered script, and remove it when we're done deferring (so at the end of EndDeferringScripts say). Feels like that better corresponds to what we really want to accomplish (don't fire onload until defered scripts have run). Or simply add the blocker in StartDeferringScripts would work too.

Though maybe it's scary to have the blocker hanging around for that long?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. that wouldn't work I realized.. pondering some more

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So here's an alternative fix:

Add an onloadblocker in BeginDeferringScripts
In EndDeferringScripts, set a flag saying that we need to remove that blocker
In ProcessPendingRequests, once we've processed the last request, and the flag is
  set, remove the onloadblocker

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd also be fine with going with the existing patch, i think that works for the currently existing code. The only problem i can think of if there is a script-loader-blocker that doesn't also block onload.

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK, I tried to write a regression test for this, but the problem is that the script load needs to start before the stylesheet load, end before the stylesheet load, and both need to end after DOMContentLoaded.

I can't seem to trigger this, unfortunately.  I'll attach what I have.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Test attempt

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can't you use an inline deferred script? That doesn't solve the problem of stylesheet vs. doc timing though

Comment 24

[Boris Zbarsky [:bzbarsky]]

Inline script doesn't seem to help.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch per comment 19

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 357039 [details] [diff] [review]
Patch per comment 19

Looks great. Wondering how come you can get EndDeferring without a matching BeginDeferring though...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Because the XUL content sink never calls BeginLoad() on the document.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1a8ce361d90a

Comment 29

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/edabde5fc73a

Comment 30

[Mikko Rantalainen]

The test case at http://semyo329k.ktl.jyu.fi/test/test.xhtml has regressed. At least the build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090216 Minefield/3.2a1pre displays following in the body:

**
XML Parsing Error: 
Location: http://semyo329k.ktl.jyu.fi/parse-error.xhtml
Line Number 12, Column 1:

^
**

Don't have exact regression date, unfortunately.

Seems to be related to deferred script in the start of the page.

Comment 31

[Mikko Rantalainen]

Minimized test case for the regression at http://semyo329k.ktl.jyu.fi/test/parse-error.xhtml

Comment 32

[Boris Zbarsky [:bzbarsky]]

Mikko, good catch.  I filed bug 478889 on that.