Last Comment Bug 309986 - Async restyle handling can cause content doubling
: Async restyle handling can cause content doubling
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://landfill.mozilla.org/ryl/slowR...
Depends on:
Blocks: 288064 312055
  Show dependency treegraph
 
Reported: 2005-09-25 12:54 PDT by Boris Zbarsky [:bz]
Modified: 2005-10-24 09:05 PDT (History)
1 user (show)
mtschrep: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Source of the script in the URL field (989 bytes, text/plain)
2005-09-25 12:56 PDT, Boris Zbarsky [:bz]
no flags Details
Proposed patch (5.87 KB, patch)
2005-09-25 12:57 PDT, Boris Zbarsky [:bz]
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
Updated to comments (6.20 KB, patch)
2005-09-25 16:05 PDT, Boris Zbarsky [:bz]
dbaron: review+
dbaron: superreview+
mtschrep: approval1.8b5+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2005-09-25 12:54:36 PDT
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 [:bz] 2005-09-25 12:56:50 PDT
Created attachment 197351 [details]
Source of the script in the URL field
Comment 2 Boris Zbarsky [:bz] 2005-09-25 12:57:48 PDT
Created attachment 197352 [details] [diff] [review]
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-7 (review requests must explain patch) 2005-09-25 14:10:19 PDT
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 [:bz] 2005-09-25 16:05:36 PDT
Created attachment 197369 [details] [diff] [review]
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 [:bz] 2005-09-25 18:29:20 PDT
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 [:bz] 2005-09-25 20:53:50 PDT
Fixed on trunk.
Comment 7 Mike Schroepfer 2005-09-26 14:21:31 PDT
Comment on attachment 197369 [details] [diff] [review]
Updated to comments

Approved per 9/26 bug triage meeting.
Comment 8 Boris Zbarsky [:bz] 2005-09-26 15:30:41 PDT
Fixed on branch.
Comment 9 walter sheets 2005-10-11 05:07:53 PDT
(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 2005-10-11 05:28:48 PDT
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 2005-10-11 09:02:46 PDT
(In reply to comment #10)

filed as bug 312055
Comment 12 Boris Zbarsky [:bz] 2005-10-11 16:08:59 PDT
> 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 2005-10-24 09:05:19 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.