Closed Bug 477880 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
And yes, those reftests fail without the patch.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #361619 - Flags: superreview?(roc)
Attachment #361619 - Flags: review?(jst)
Summary: Onload can fire before we've kicked off layout-dependent loads → [FIX]Onload can fire before we've kicked off layout-dependent loads
Attachment #361619 - Flags: superreview?(roc) → superreview+
Blocks: ireflow
Attachment #361619 - Flags: review?(jst) → review+
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.
> 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.
Sadly, I can't think of a reliable way to write a test for that reentry scenario.  :(
Attachment #361619 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/1190db3643e4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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.
Attachment #361698 - Flags: approval1.9.1?
Blocks: 486242
Attachment #361698 - Flags: approval1.9.1? → approval1.9.1+
Duplicate of this bug: 486242
Depends on: 581240
Depends on: 1282985
You need to log in before you can comment on or make changes to this bug.