477880 - [FIX]Onload can fire before we've kicked off layout-dependent loads

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

So at some point we stopped having reflow events block onload and moved to just flushing onload right before firing the onload event.  This worked fine, sort of, except we now kick off network loads from onload (e.g. downloadable fonts).  I've actually been getting sporadic reftest failures as a result, depending on the HTTP cache state, the phase of the moon, and when reflow gets interrupted.

So I think we should go back to having pending reflows block onload, sort of.  I'm going to add a layout flush to just before we actually try to check for docloader emptiness.  If that flush doesn't start any new loads, we're good to go with onload.  If it does, we'll wait for those requests.  We will continue to not wait on random pending reflow events, and continue to flush before onload firing.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

And yes, those reftests fail without the patch.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 361619 [details] [diff] [review]
Proposed patch

nsDocLoader::DocLoaderIsEmpty():

+    nsresult rv = mLoadGroup->GetActiveCount(&count);
+    if (NS_FAILED(rv) || 0 != count || IsBusy()) {
+      return;

Isn't the GetActiveCount() call and count check redundant here given the IsBusy() check?

r=jst with that checked out.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Isn't the GetActiveCount() call and count check redundant here given the
> IsBusy() check?

Turns out, it actually is.  I'll remove that part.

I did find one issue with this patch, actually.  As written, if during the layout flush someone starts and immediately stops a network load (which is exactly what happens when an already-cached image is requested during the style flush there), we'll immediately fire onload because we're getting our last request removed and aren't busy.  That's onload firing under style resolution.  Then we'll unwind and fire it again.

I'm going to protect against that by just using a boolean to flag when we're flushing and having IsBusy() return true if the boolean is true.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments and to fix onload reentry

Sadly, I can't think of a reliable way to write a test for that reentry scenario.  :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1190db3643e4

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 361698 [details] [diff] [review]
Updated to comments and to fix onload reentry

Given that we have downloadable fonts on 1.9.1, we should probably get this patch in there too, so onload waits for them.

Comment 7

[John Daggett (:jtd)]

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