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

RESOLVED FIXED in mozilla1.9.1b3

Status

()

Core
DOM: Core & HTML
P1
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Mikko Rantalainen, Assigned: bz)

Tracking

({fixed1.9.1, regression, testcase})

Trunk
mozilla1.9.1b3
x86
Windows XP
fixed1.9.1, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
Created attachment 337018 [details]
External JS file for the test case (displays an alert() on window.onload())
(Reporter)

Comment 2

9 years ago
Created attachment 337021 [details]
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.
(Reporter)

Comment 3

9 years ago
Created attachment 337023 [details]
Fixed test case (with the correct external JS URL)
Attachment #337021 - Attachment is obsolete: true
(Reporter)

Comment 4

9 years ago
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?).
Confirmed on Windows XP, latest trunk.
Works: 2008080416
Fails: 2008080504
Caused by Bug 28293 ?
Blocks: 28293
Flags: blocking1.9.1?
Keywords: regression, testcase
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
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...
Assignee: nobody → jonas
Flags: blocking1.9.1? → blocking1.9.1+
(Reporter)

Comment 7

9 years ago
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?

Updated

9 years ago
Priority: -- → P2
(Reporter)

Comment 8

9 years ago
Any progress with this regression?
DOMContentLoaded does in fact always fire before onload.  And the testcase seems to work for me with a current trunk build...
(Reporter)

Comment 10

9 years ago
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.
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?
Created attachment 345094 [details] [diff] [review]
Like so, say
Assignee: jonas → bzbarsky
Status: NEW → ASSIGNED
Attachment #345094 - Flags: superreview?(jonas)
Attachment #345094 - Flags: review?(jonas)
Summary: Deferred (defer) external javascript fails to launch window.onload() if external CSS has been loaded after the deferred script → [FIX]Deferred (defer) external javascript fails to launch window.onload() if external CSS has been loaded after the deferred script
(Reporter)

Comment 13

9 years ago
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.
Yes.  That would be why this bug ins't marked fixed, right?
That said, we have got to fix this in the next beta.  Jonas, any chance of reviews sometime?
Target Milestone: --- → mozilla1.9.1b3
I think this bug should be a P1, not a P2.  We desperately need web compat testing here...
Priority: P2 → P1
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?
Hmm.. that wouldn't work I realized.. pondering some more
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
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.
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.
Created attachment 357013 [details] [diff] [review]
Test attempt
Can't you use an inline deferred script? That doesn't solve the problem of stylesheet vs. doc timing though
Inline script doesn't seem to help.
Created attachment 357039 [details] [diff] [review]
Patch per comment 19
Attachment #345094 - Attachment is obsolete: true
Attachment #357039 - Flags: superreview?(jonas)
Attachment #357039 - Flags: review?(jonas)
Attachment #345094 - Flags: superreview?(jonas)
Attachment #345094 - Flags: review?(jonas)
Attachment #357039 - Flags: superreview?(jonas)
Attachment #357039 - Flags: superreview+
Attachment #357039 - Flags: review?(jonas)
Attachment #357039 - Flags: review+
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...
Because the XUL content sink never calls BeginLoad() on the document.
Pushed http://hg.mozilla.org/mozilla-central/rev/1a8ce361d90a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/edabde5fc73a
Keywords: fixed1.9.1
(Reporter)

Comment 30

9 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 31

9 years ago
Minimized test case for the regression at http://semyo329k.ktl.jyu.fi/test/parse-error.xhtml
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Mikko, good catch.  I filed bug 478889 on that.
Depends on: 396226
You need to log in before you can comment on or make changes to this bug.