1438467 - Avoid reframing the container frame when a display: contents element with pseudos goes away.

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

Now that the display: contents element has references to the pseudo-elements it should be doable.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements.

We just need to use the existing StyleChildrenIterator which iterates over them.

We need to be a bit careful though, since ::before and ::after are owned by
their own frame, and thus could be unbound from the tree after removing the
frame.

Hopefully the only access to the node being removed is innecessary.

There's also the weird thing of the thing we're iterating changing under the
hood. It works fine for this case, but maybe it would be better to handle them
explicitly like:

  if (Element* before = nsLayoutUtils::GetBeforePseudo(aChild)) {
    bool didReconstruct = ContentRemoved(aChild, ...);
    if (didReconstruct) {
      return true;
    }
    MOZ_ASSERT(!nsLayoutUtils::GetBeforePseudo(aChild));
  }

  // Same for ::after.

  StyleChildrenIterator iter(aChild);
  for (..) {
    // Do the rest of the kids, which can't get unbound.
  }

That'd repeat a bunch of code, so not a fan neither... I can point this out more
explicitly in a comment if wanted.

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

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8951192 [details]
Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements.

https://reviewboard.mozilla.org/r/220452/#review226440

This seems fine to me in general assuming it works now,
modulo the "modifying the tree while iterating" thing.
I'd probably handle ::before/::after upfront to avoid that,
but if Boris thinks that's unnecessary then it's fine with me.

::: commit-message-3219c:9
(Diff revision 1)
> +Hopefully the only access to the node being removed is innecessary.
> +

I don't understand that sentence.

::: commit-message-3219c:11
(Diff revision 1)
> +There's also the weird thing of the thing we're iterating changing under the
> +hood. It works fine for this case, but maybe it would be better to handle them
> +explicitly like:

Yeah, this seems scary, especially when the documentation says:
" * Note: it assumes that no mutation of the DOM or frame tree takes place during
 * iteration, and will break horribly if that is not true."
https://searchfox.org/mozilla-central/source/dom/base/ChildIterator.h#270

::: layout/base/nsCSSFrameConstructor.cpp:8730
(Diff revision 1)
> +    // NOTE(emilio): Kid could be unbound from the tree here already if it is a
> +    // ::before or ::after pseudo-element (in which case it's owned by
> +    // childFrame, which is gone already).

nit: it might be clearer if we replace "in which case it's owned by childFrame, which is gone already" with "since in that case it was owned by childFrame, which we just destroyed".

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8951192 [details]
Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements.

https://reviewboard.mozilla.org/r/220452/#review226458

::: commit-message-3219c:9
(Diff revision 1)
> +Hopefully the only access to the node being removed is innecessary.
> +

It just meant to say that the only access to aChild after the RemoveFrame call is pointless, thus the new assert and the removal of that `!aChild->IsRootOfAnonymousSubtree()`.

Will clarify.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951192 [details]
Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements.

https://reviewboard.mozilla.org/r/220452/#review226528

I find it surprising that ContentRemoved on a ::before/::after actually does something sane, but not going to complain about it.

Please do apply the various comment and commit message clarifications Mats asked for.

::: commit-message-3219c:6
(Diff revision 1)
> +Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements. r?mats,bz
> +
> +We just need to use the existing StyleChildrenIterator which iterates over them.
> +
> +We need to be a bit careful though, since ::before and ::after are owned by
> +their own frame, and thus could be unbound from the tree after removing the

More to the point, they can be dead, right?

::: commit-message-3219c:9
(Diff revision 1)
> +
> +We need to be a bit careful though, since ::before and ::after are owned by
> +their own frame, and thus could be unbound from the tree after removing the
> +frame.
> +
> +Hopefully the only access to the node being removed is innecessary.

"unnecessary".

::: commit-message-3219c:11
(Diff revision 1)
> +There's also the weird thing of the thing we're iterating changing under the
> +hood. It works fine for this case, but maybe it would be better to handle them
> +explicitly like:

We really need to document the actual invariants for StyleChildrenIterator and then make sure we're not violating them here.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

This is green modulo that assertion I added in the last moment before submitting and caught bug 1439036.

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc8721df863afa6935f8366ceae9b3999ddff8b

I'm not on a rush to land this so can wait until that one gets review.

Comment 6

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> More to the point, they can be dead, right?

Yes.

Looking at some of the callers, like:
WipeContainingBlock
  RecreateFramesForContent(aFrame->GetContent()
    ContentRemoved

it seems like a real risk too.
It looks like some of the call paths actually do access the content node
(using a plain pointer) after it was passed (indirectly) to ContentRemoved.
I'm not sure exactly which paths could have a ::before/::after though.

Emilio, did you analyze this ownership problem?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

If some of those callers called ContentRemoved with a ::before pseudo-element, they would be buggy already, since this doesn't change what ContentRemoved does. Actually, I'll add assertions to RecreateFramesForContent.

Now, the issue would be if any of the stuff ContentRemoved calls could end up removing the frame and _then_ using the content that was passed to ContentRemoved. I think this is not the case, all the callers go up the frame tree, so they'll reconstruct something up the parent chain, and then immediately return.

Does this answer the question? Wanna make sure I understood it properly.

Comment 8

[Mats Palmgren (inactive)]

Yeah, probably.  I was thinking that perhaps
RecreateFramesForContent(ancestor) use paths that more carefully
holds strong refs on its children than the direct call to
ContentRemoved(aChild) here is doing but, after looking at
the code, that's probably not the case.

OK, so any new problems would be in the local code in ContentRemoved
itself that you're modifying here.  Given that we're inside a
"if (... && GetDisplayContentsStyleFor(aChild))" block here, we know
that UndisplayedNode::mContent is keeping aChild alive until
the UnregisterDisplayContentsStyleFor call after the loop.
So does that method depend on it being alive for the duration of
the call?  It looks like it does actually:
https://searchfox.org/mozilla-central/rev/07d2af421952ef32d0e2d54f88db764d87456016/layout/base/nsFrameManager.cpp#440
The RemoveNodeFor call might remove the last strong reference, so we
might have an UAF inside the ClearAllMapsFor call that follows?
Or am I missing something?

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Pseudo-elements can't be in undisplayed maps to begin with (if they're display: none we don't generate one at all, and they can't be display: contents since they are owned by a frame, and we switch them to display: inline).

And the display: contents element itself (aChild in the _first_ ContentRemoved call) can't go away while at it, it's only the pseudo (in the second ContentRemoved call) what can be gone _only_ after the RemoveFrame call.

Does that make sense?

Comment 10

[Mats Palmgren (inactive)]

Oh ok, so the pseudo is a 'c' in the loop there?
Does StyleChildrenIterator::GetNextChild() cope with the current element going away?

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Right, and yes, that's what is slightly not-awesome, but I'll point it out on a comment.

Comment 12

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2da204077c8f
Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements. r=mats,bz

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2da204077c8f