For content insertion, notifying the binding manager makes total sense, since that way the frame constructor and all the stuff can see the final flattened tree state. However, for content removal, maybe it should happen the other way around. See bug 1420533, for example, where the content is already gone from the flat tree when we try to destroy its frames. Seems to me that we should try to call into the frame constructor before that happens, otherwise we cannot assert invariants that should otherwise hold. Maybe we should just wait for XBL to go away... But in the meantime, maybe it makes sense to do that. It would also allow us to fix bug 1420546.

Boris, wdyt of comment 0?

We used to have it that way, actually, because it seemed to make sense. We changed the behavior in bug 382376. Worth reading through that bug so I don't have to retype various details. But the salient point is this: a remove notification on CSSFC can do a reframe, which _constructs_ frames, which needs the insertion point situation to be updated to the removal first. Now that was back then. In today's world, if all the reframe paths coming through ContentRemoved do an async insert, then we can probably rearrange the ordering to be sane again. The actual mechanics would be a bit complicated, because we've gone back to just having the binding manager be the first observer, not the thing that notifies all other observers, so ordering is a little harder to control....

Comment on attachment 8936445 [details] Bug 1420547: Notify the pres shell specially, instead of via mutation observers. https://reviewboard.mozilla.org/r/207150/#review214608 ::: dom/base/nsDocument.h:1110 (Diff revision 2) > virtual bool IsThirdParty() override; > > -#define NS_DOCUMENT_NOTIFY_OBSERVERS(func_, params_) \ > +#define NS_DOCUMENT_NOTIFY_OBSERVERS(func_, params_) do { \ > - NS_OBSERVER_ARRAY_NOTIFY_XPCOM_OBSERVERS(mObservers, nsIDocumentObserver, \ > + NS_OBSERVER_ARRAY_NOTIFY_XPCOM_OBSERVERS(mObservers, nsIDocumentObserver, \ > - func_, params_); > + func_, params_); \ > + /* FIXME(emilio): Apparently we can keep observing from the BFCache? That \ So this changed notification order so the presshell is notified after all other observers, right? I guess that could already be the case, more or less, if the iframe containing the document went through display:none and back. Worth talking about this change, and why it's ok, in the commit message, though.

Comment on attachment 8936446 [details] Bug 1420547: Notify the pres shell before everything else on removals. https://reviewboard.mozilla.org/r/207152/#review214610 ::: commit-message-d48a1:1 (Diff revision 2) > +Bug 1420547: Notify the pres shell before everything else on removals. r?bz This _really_ needs comments in the commit message explaining why it's ok to do this (e.g. that removals never lead to sync frame construction, assuming that's true). If that is in fact true, we should consider adding asserts to that effect in the frame constructor. ::: dom/base/nsNodeUtils.cpp:82 (Diff revision 2) > - if (last == doc) { \ > - if (nsIPresShell* shell = doc->GetObservingShell()) { \ > - shell->func_ params_; \ > - } \ > - } \ > - if (needsEnterLeave) { \ > + node = shadow->GetHost(); \ > + } else { \ > + node = node->GetParentNode(); \ > + } \ > + } while (node); \ > + if (remove_ == IsRemoveNotification::No && last == doc) { \ Can we assert, debug-only, that last == doc here if and only if node->GetComposedDoc() == doc at the beginning of this function?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11) > Comment on attachment 8936446 [details] > Bug 1420547: Notify the pres shell before everything else on removals. > > https://reviewboard.mozilla.org/r/207152/#review214610 > > ::: commit-message-d48a1:1 > (Diff revision 2) > > +Bug 1420547: Notify the pres shell before everything else on removals. r?bz > > This _really_ needs comments in the commit message explaining why it's ok to > do this (e.g. that removals never lead to sync frame construction, assuming > that's true). > > If that is in fact true, we should consider adding asserts to that effect in > the frame constructor. Yes, will do. > ::: dom/base/nsNodeUtils.cpp:82 > (Diff revision 2) > > - if (last == doc) { \ > > - if (nsIPresShell* shell = doc->GetObservingShell()) { \ > > - shell->func_ params_; \ > > - } \ > > - } \ > > - if (needsEnterLeave) { \ > > + node = shadow->GetHost(); \ > > + } else { \ > > + node = node->GetParentNode(); \ > > + } \ > > + } while (node); \ > > + if (remove_ == IsRemoveNotification::No && last == doc) { \ > > Can we assert, debug-only, that last == doc here if and only if > node->GetComposedDoc() == doc at the beginning of this function? Next commit :)

Comment on attachment 8936447 [details] Bug 1420547: Add assertions for the assumptions we're making. https://reviewboard.mozilla.org/r/207154/#review214618 Ah, here we go. ;) r=me

Comment on attachment 8936525 [details] Bug 1420547: Fix removal of <area> elements from an image map. https://reviewboard.mozilla.org/r/207200/#review214624

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/46e5fc9628c8 Notify the pres shell specially, instead of via mutation observers. r=bz https://hg.mozilla.org/integration/autoland/rev/e482d3fc80e4 Fix removal of <area> elements from an image map. r=bz https://hg.mozilla.org/integration/autoland/rev/04a9c149d299 Notify the pres shell before everything else on removals. r=bz https://hg.mozilla.org/integration/autoland/rev/24e90541f056 Add assertions for the assumptions we're making. r=bz

https://hg.mozilla.org/mozilla-central/rev/46e5fc9628c8 https://hg.mozilla.org/mozilla-central/rev/e482d3fc80e4 https://hg.mozilla.org/mozilla-central/rev/04a9c149d299 https://hg.mozilla.org/mozilla-central/rev/24e90541f056