379093 - [FIX]Onload can fire before page layout is complete

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 378975 comment 6.

I guess we could try dealing with this by revoking reflow events when a reflow flush occurs.  Or we could explicitly flush out layout before invoking the onload handler.

Comment 1

[Boris Zbarsky [:bzbarsky]]

A third option is to block onload while there is a reflow event pending (and make sure to call DoneRemovingDirtyRoots() from ProcessReflowCommands even if we didn't remove any, since that may just indicate that the reflow event fired).  I don't quite like this approach because there is no guarantee that it converges: if reflows keep posting new reflow events, onload will never fire.  That seems undesirable.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

In general we don't want to keep revoking reflow events to keep down on the churn during, say, DHTML animations that also flush layout all the time.  But during pageload, I think we need to do this...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Simpler option

This just goes ahead and flushes out reflow before firing the onload event.  This is simpler than trying to maintain the document loading state, esp. since that breaks down if partway through the load a display:none iframe gets a presshell or some such.

Comment 4

[Eli Friedman]

I don't think it's generally safe to be keeping around a raw pointer to the window through a FlushPendingNotifications call. Is there some reason it's safe here?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Good catch

Patch on top of "simpler option" (which I checked in).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note: the checkin of the "simpler option" patch didn't significantly affect Tp or Tp2 on the Linux boxen...  Which is interesting, since it certainly affected pageload times here.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this needed on branches? If it's a bug on branches, then I think perhaps we should take this on branches, for the sake of being able to compare Tp between trunk and branch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

It probably _could_ be useful on branches, though I'm not sure we're hitting this problem on branches on Tp per se.

But as far as comparing Tp, we want to compare to various 1.9 milestones as well, so I filed bug 379233 a few days ago.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in the second patch too.