309986 - Async restyle handling can cause content doubling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If we post a restyle event, then parse some more content (but don't notify on it
yet), then the restyle event fires, we may end up constructing frames for the
new content (if it's a child of what we're restyling, say).  Then we'll
construct frames again when the notification happens, leading to content doubling.

What we should do, therefore, is flush out the content sink before processing
restyle events.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Source of the script in the URL field

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

While poking around at this stuff, I also noticed that we seem to be flushing
out the sink on UPDATE_CONTENT_STATE changes, which we don't need to do.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

>Index: layout/base/nsCSSFrameConstructor.cpp
>+  // Make sure that any restyles that happen from now on will go into
>+  // a new event.
>   constructor->mRestyleEventQueue = nsnull;
>+  // Force flushing of any pending content notifications that might have queued
>+  // up while our event was pending.  That will ensure that we don't construct
>+  // frames for content right now that's still waiting to be notified on,
>+  constructor->mPresShell->GetDocument()->
>+    FlushPendingNotifications(Flush_ContentAndNotify);
>+

Why not do these two in the reverse order?

>Index: content/html/document/src/nsHTMLContentSink.cpp
>+  // continue.  Note that UPDATE_CONTENT_STATE notifications never
>+  // cause synchronous frame construction, so we never have to worry
>+  // about them here.

This is because they cause posting of restyle events, right?  Should you
comment that the handling code does cause a flush (due to the code you're
adding), which makes this ok?

>+  if (aUpdateType != UPDATE_CONTENT_STATE && 

These tests make it look like aUpdateType isn't a bitmask.  I'm tempted to ask
you to write (aUpdateType & ~nsUpdateType(UPDATE_CONTENT_STATE)), even though
that's slower.	(It's guaranteed not to be zero, right?  Should there be
assertions that it's nonzero and that it's a subset of UPDATE_ALL?  Probably
those assertions would be sufficient documentation that it's a bitmask.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

> Why not do these two in the reverse order?

I had a long and it turns out incorrect argument for the ordering.  ;)

> Should you comment that the handling code does cause a flush 

Done.

> Probably those assertions would be sufficient documentation that it's a
> bitmask

Added the asserts.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197369 [details] [diff] [review]
Updated to comments

Requesting 1.8b5 approval for just the second part of the nsCSSFrameConstructor
changes (which is the smallest change that fixes this bug).  It should be quite
safe.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 7

[Mike Schroepfer]

Comment on attachment 197369 [details] [diff] [review]
Updated to comments

Approved per 9/26 bug triage meeting.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 9

[walter sheets]

(In reply to comment #6)
> Fixed on trunk.
This check-in breaks some (not all) java applets, unfortunately.  In some cases,
an applet never starts running, and sometimes can be started by reloading the
web page several times (race condition?).

This web site is one example of the problem:
http://www.ghcc.msfc.nasa.gov/GOES/
Click on any of the satellite images and then click on the 'Animate image below'
button.  Sometimes the applet starts, but often it won't.

I built firefox about two dozen times in order to narrow the problem down to
this one check-in.

Comment 10

[walter sheets]

BTW, firefox crashed just after I submitted my previous comment -- I sent a 
talkback report referring to this bug number because I realize now that this
problem with loading java applets has been causing firefox to crash when viewing
an applet in one tab and doing something else in another tab.  This is the
third crash I've seen just lately under the same circumstances.

Comment 11

[timeless]

(In reply to comment #10)

filed as bug 312055

Comment 12

[Boris Zbarsky [:bzbarsky]]

> This check-in breaks some (not all) java applets, unfortunately.

In which build?  Branch or trunk?

Please file a new bug for the issue with clear steps to reproduce and a URI, and
CC me?

Comment 13

[walter sheets]

(In reply to comment #12)
> Please file a new bug for the issue with clear steps to reproduce and a URI, and CC me?

Opened bug 313617, thanks.