I wrote this patches while looking at bug 1395719, and they make easier to do the correct fix.

Boris said he would take a look, since Mats is probably (hopefully, actually :P) on the weekend. Btw, the unexpected passes in the try run above are due to the patches from bug 1384232, not these, so no behavior change, really.

Comment on attachment 8903671 [details] Bug 1396018: Remove the hacky removeflags check we do for display: contents and XBL. https://reviewboard.mozilla.org/r/175442/#review181442 r=me

Comment on attachment 8903672 [details] Bug 1396018: Don't pass aFlags all the way through RecreateFramesForContent. https://reviewboard.mozilla.org/r/175444/#review181448 ::: commit-message-c23ed:3 (Diff revision 1) > +Bug 1396018: Don't pass aFlags all the way down RecreateFramesForContent. r?mats > + > +We only need aFlags to determine whether a content is being removed to reframe Do we only care about the first ContentRemoved call? Why do we not care if it gets propagated up to a ContentRemoved on an ancestor?

Comment on attachment 8903673 [details] Bug 1396018: Remove REMOVE_DESTROY_FRAMES. https://reviewboard.mozilla.org/r/175446/#review181450 ::: commit-message-da8f2:3 (Diff revision 1) > +Bug 1396018: Remove REMOVE_DESTROY_FRAMES. r?mats > + > +We don't use the frame tree state anyway. Um... Then how do we restore things like scroll state across reconstructs?

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7) > Comment on attachment 8903672 [details] > Bug 1396018: Don't pass aFlags all the way down RecreateFramesForContent. > > https://reviewboard.mozilla.org/r/175444/#review181448 > > ::: commit-message-c23ed:3 > (Diff revision 1) > > +Bug 1396018: Don't pass aFlags all the way down RecreateFramesForContent. r?mats > > + > > +We only need aFlags to determine whether a content is being removed to reframe > > Do we only care about the first ContentRemoved call? Why do we not care if > it gets propagated up to a ContentRemoved on an ancestor? Because in that case we'll reframe the siblings of the removed content anyway.

Comment on attachment 8903672 [details] Bug 1396018: Don't pass aFlags all the way through RecreateFramesForContent. https://reviewboard.mozilla.org/r/175444/#review181448 > Do we only care about the first ContentRemoved call? Why do we not care if it gets propagated up to a ContentRemoved on an ancestor? OK, so here's what I think this commit message should say about ContentREmoved: We only need the aFlags argument of ContentRemoved to determine when a frame is being removed due to its element being removed from the DOM, so we reframe its now-possibly-no-longer-suppressed whitespace siblings as needed. In all other cases, our ContentRemoved call will be followed by a ContentInserted call, which will end up doing AddTextItemIfNeeded() to generate the relevant textframes if they're necessary. Since we only need to tell apart the "DOM removal" and "anything else" cases, we don't need to thread the aFlags argument through all the ways ContentRemoved can call itself (on an ancestor). All those cases should just be treated as "not DOM removal". In particular, even if the original call _was_ for a DOM removal, if we convert it to an ancestor reframe, which will call ContentInserted on the ancestor as well, we don't need to do anything with text siblings of the ancestor.

Comment on attachment 8903672 [details] Bug 1396018: Don't pass aFlags all the way through RecreateFramesForContent. https://reviewboard.mozilla.org/r/175444/#review181928 r=me ::: commit-message-c23ed:3 (Diff revision 2) > +Bug 1396018: Don't pass aFlags all the way through RecreateFramesForContent. > + > +We only need the aFlags argument of ContentRemoved to determine when a frame is Given that we actually have two uses, "only" is odd here, I guess. How about: We currently use the aFlags argument of ContentRemoved for two purposes: 1) To determine when a frame is being removed [etc]. 2) For saving the frame tree state [etc].

Comment on attachment 8903673 [details] Bug 1396018: Remove REMOVE_DESTROY_FRAMES. https://reviewboard.mozilla.org/r/175446/#review181930 r=me ::: layout/base/nsCSSFrameConstructor.h:328 (Diff revision 2) > * aFlags == REMOVE_FOR_RECONSTRUCTION means the caller will reconstruct the > * frames later. > * In both the above cases, this method will in some cases try to reconstruct > * the frames (and true will be returned in that case), it's just that in the > * former case aChild isn't in the document so no frames will be created for > - * it. Ancestors may have been reframed though. > + * it. It'll try to do synchronously or async using the value of How about: In both the above cases, this method will in some cases try to reconstruct frames on some ancestor of aChild. This can happen regardless of the value of aFlags. The return value indicates whether this "reconstruct an ancestor" action took place. If true is returned, that means that the frame subtree rooted at some ancestor of aChild's frame was destroyed and either has been reconstructed or will be reconstructed async, depending on the value of aInsertionKind. ?

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c18cf6c6ebd Remove the hacky removeflags check we do for display: contents and XBL. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/911c5220cc3e Don't pass aFlags all the way through RecreateFramesForContent. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0b2d0e08aa Remove REMOVE_DESTROY_FRAMES. r=bz

https://hg.mozilla.org/mozilla-central/rev/5c18cf6c6ebd https://hg.mozilla.org/mozilla-central/rev/911c5220cc3e https://hg.mozilla.org/mozilla-central/rev/6b0b2d0e08aa