1420547 - Should the binding manager really be notified of content removals before the pres shell?

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Boris, wdyt of comment 0?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1420547: Notify the pres shell specially, instead of via mutation observers.

Review commit: https://reviewboard.mozilla.org/r/207150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207150/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1420547: Notify the pres shell before everything else on removals.

Review commit: https://reviewboard.mozilla.org/r/207152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207152/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1420547: Add assertions for the assumptions we're making.

Review commit: https://reviewboard.mozilla.org/r/207154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207154/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1420547: Fix removal of <area> elements from an image map.

This code was relying on nsImageFrame::ContentRemoved running before it.

Review commit: https://reviewboard.mozilla.org/r/207200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207200/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936445 [details]
Bug 1420547: Notify the pres shell specially, instead of via mutation observers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207150/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936446 [details]
Bug 1420547: Notify the pres shell before everything else on removals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207152/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936447 [details]
Bug 1420547: Add assertions for the assumptions we're making.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207154/diff/1-2/

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(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 13

[Boris Zbarsky [:bzbarsky]]

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 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8936525 [details]
Bug 1420547: Fix removal of <area> elements from an image map.

https://reviewboard.mozilla.org/r/207200/#review214624

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936445 [details]
Bug 1420547: Notify the pres shell specially, instead of via mutation observers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207150/diff/2-3/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936525 [details]
Bug 1420547: Fix removal of <area> elements from an image map.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207200/diff/1-2/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936446 [details]
Bug 1420547: Notify the pres shell before everything else on removals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207152/diff/2-3/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936447 [details]
Bug 1420547: Add assertions for the assumptions we're making.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207154/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Natalia Csoregi [:nataliaCs]]

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