Closed Bug 477880 Opened 14 years ago Closed 14 years ago

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


(Core :: DOM: Navigation, defect)

Not set





(Reporter: bzbarsky, Assigned: bzbarsky)



(Keywords: fixed1.9.1)


(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
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


+    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
Closed: 14 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
You need to log in before you can comment on or make changes to this bug.