1.14 KB, application/xhtml+xml
2.35 KB, patch
|Details | Diff | Splinter Review|
4.21 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:126.96.36.199eol) Gecko/20080715 Ubuntu/dapper-security Firefox/188.8.131.52eol 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.
Created attachment 337018 [details] External JS file for the test case (displays an alert() on window.onload())
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.
Created attachment 337023 [details] Fixed test case (with the correct external JS URL)
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 ?
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...
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?
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...
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
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?
I think this bug should be a P1, not a P2. We desperately need web compat testing here...
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.
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
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.
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.
Minimized test case for the regression at http://semyo329k.ktl.jyu.fi/test/parse-error.xhtml
Mikko, good catch. I filed bug 478889 on that.
9 years ago