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.
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 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.)
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.
12 years ago
12 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.
Fixed on trunk.
Comment on attachment 197369 [details] [diff] [review] Updated to comments Approved per 9/26 bug triage meeting.
Fixed on branch.
(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.
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.
> 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?
(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.