Closed
Bug 309986
Opened 19 years ago
Closed 19 years ago
Async restyle handling can cause content doubling
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
()
Details
(Keywords: fixed1.8)
Attachments
(2 files, 1 obsolete file)
989 bytes,
text/plain
|
Details | |
6.20 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
mtschrep
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
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.
Attachment #197352 -
Flags: superreview?(dbaron)
Attachment #197352 -
Flags: review?(dbaron)
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.)
Assignee | ||
Comment 4•19 years ago
|
||
> 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.
Assignee | ||
Updated•19 years ago
|
Attachment #197369 -
Flags: superreview?(dbaron)
Attachment #197369 -
Flags: review?(dbaron)
Assignee | ||
Updated•19 years ago
|
Attachment #197352 -
Attachment is obsolete: true
Attachment #197352 -
Flags: superreview?(dbaron)
Attachment #197352 -
Flags: superreview-
Attachment #197352 -
Flags: review?(dbaron)
Attachment #197352 -
Flags: review-
Attachment #197369 -
Flags: superreview?(dbaron)
Attachment #197369 -
Flags: superreview+
Attachment #197369 -
Flags: review?(dbaron)
Attachment #197369 -
Flags: review+
Assignee | ||
Comment 5•19 years ago
|
||
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.
Attachment #197369 -
Flags: approval1.8b5?
Assignee | ||
Comment 6•19 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 7•19 years ago
|
||
Comment on attachment 197369 [details] [diff] [review] Updated to comments Approved per 9/26 bug triage meeting.
Attachment #197369 -
Flags: approval1.8b5? → approval1.8b5+
Updated•19 years ago
|
Flags: blocking1.8b5+
Comment 9•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
(In reply to comment #10) filed as bug 312055
Assignee | ||
Comment 12•19 years ago
|
||
> 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•19 years ago
|
||
(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.
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•