992521 - ShadowRoot is marked to be in document, although it isn't

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

We really can't have 
http://mxr.mozilla.org/mozilla-central/source/content/base/src/ShadowRoot.cpp?rev=20a329b254ae&mark=76-77#67

We have code all over relying on IsInDoc() meaning that you can get from such
node to document (and that you can get from document to such node, although perhaps
via some xbl stuff).

Comment 1

[Andrew McCreight [:mccr8]]

You can still get from a ShadowRoot to the document via nsNodeInfo, it is just the reverse direction that does not hold.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

That is the ownerdoc edge, which should be there, but there isn't
parent node chain from ShadowRoot to document, AFAIK.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Or perhaps we can have that IsInDoc, but need to go through all the GetCurrentDoc, GetDocument and
IsInDoc checks to ensure that they expect this rather unusual setup.

Comment 4

[Boris Zbarsky [:bzbarsky]]

They absolutely do not.  We assume that IsInDoc() means exactly that for various spec bits....  Why is that line there?

Comment 5

[William Chen [:wchen]]

The spec says:

"All other HTML elements in the shadow trees must behave as if they were part of the document tree."

"other" refers to link and base elements.

http://w3c.github.io/webcomponents/spec/shadow/#html-elements-in-shadow-trees

Anonymous content also lie about being in the document, so the code should already be expecting this unusual case to some extent. Although shadow DOM is even a bit more unusual than anonymous content because you can't reach the document by traversing the parent chain, in addition to being unable to reach the content traversing down child lists from the document.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Anonymous content also lie about being in the document

Anonymous content has the property the going up the GetParent() chain will reach the document.

There are various places where we use IsInDoc() to check whether such a walk makes sense and assume that it does if true is returned.

It sounds like we really need to go through and audit our IsInDoc()/GetDocument() callers and make sure they can deal with a change in the behavior here or something.

Comment 7

[Nicholas Nethercote [inactive]]

khuey thinks this isn't MemShrink relevant.

Comment 8

[Andrew McCreight [:mccr8]]

Well, it can make us leak documents.

Comment 9

[William Chen [:wchen]]

How does it make us leak documents and how does XBL deal with this?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

We have various cycle collection optimizations for Nodes which rely on IsInDoc() to not lie.
XBL doesn't have this issue, since it deals with document and elements and such, not document fragments (which so far have never been in document).

Comment 11

[William Chen [:wchen]]

Ah, OK. So the problem is ShadowRoot being in doc, and not necessarily its children (which are also just elements, text nodes, etc)? I can remove that flag on ShadowRoot as the spec doesn't mention anything about the ShadowRoot itself behaving as if in document.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Well the children should be in the doc also only in case they actually are in the doc.
I mean, there should be child->parent/insertion-parent chain from in-doc node to document.

Comment 13

[Dave Camp (:dcamp)]

This is blocking (through 777674) our ability to show the shadow dom in the devtools inspector.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Having a DocumentFragment that claims to be IsInDoc() is just broken beyond belief.  It violates all sorts of invariants.

For the actual shadow kids, they should be IsInDoc() if and only if the shadow host IsInDoc(), no?

Comment 15

[William Chen [:wchen]]

sicking has a proposal to change the spec so that we don't treat shadow tree elements as if in the document and explicitly specify "in document" features that we want to work like <style> and <script>.

I'm working on a patch right now that makes ShadowRoot and its descendants never marked in document.

Comment 16

[William Chen [:wchen]]

Attachment: Part 1: Remove "in document" flag on ShadowRoot and its descendants.

- Make ShadowRoot and its descendants no longer marked in document.
- Remove union of mSubtreeRoot and mPrimaryFrame in nsINode because nodes in a non-document subtree may have frames.
- Added GetCrossShadowCurrentDoc() to replace GetCurrentDoc() for cases where we want the current document crossing shadow root boundaries.

Comment 17

[William Chen [:wchen]]

Attachment: Make styles work when not in document, but contained by a ShadowRoot with a host in document.

Comment 18

[William Chen [:wchen]]

Attachment: Part 3: Make script run when not in document, but contained by a ShadowRoot with a host in document.

Comment 19

[William Chen [:wchen]]

Attachment: Part 4: Make PresShell events work when not in document, but contained by a ShadowRoot with a host in document.

Comment 20

[William Chen [:wchen]]

Attachment: Part 5: Bind native anonymous content to null document when node is contained by a ShadowRoot.

Comment 21

[William Chen [:wchen]]

For reference, this comment describes alternate behavior instead of treating shadow root elements as if in document:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25562#c1

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8422136 [details] [diff] [review]
Part 1: Remove "in document" flag on ShadowRoot and its descendants.

So this approach increases size of nsINode 4 or 8 bytes :/

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Having a DocumentFragment that claims to be IsInDoc() is just broken beyond
> belief.  It violates all sorts of invariants.
> 
> For the actual shadow kids, they should be IsInDoc() if and only if the
> shadow host IsInDoc(), no?

I think we should go with this approach in Gecko. (spec terminology can be something else)
Then we don't need to increase the size of nsINode, and only InDoc() nodes can have frames.
(That is also a thing which code tend to rely on)

Comment 24

[William Chen [:wchen]]

If we put ShadowRoot elements in document we have to deal with:

(In reply to Olli Pettay [:smaug] from comment #12)
> Well the children should be in the doc also only in case they actually are
> in the doc.
> I mean, there should be child->parent/insertion-parent chain from in-doc
> node to document.

(In reply to Boris Zbarsky [:bz] from comment #6)
> > Anonymous content also lie about being in the document
> 
> Anonymous content has the property the going up the GetParent() chain will
> reach the document.
> 
> There are various places where we use IsInDoc() to check whether such a walk
> makes sense and assume that it does if true is returned.
> 
> It sounds like we really need to go through and audit our
> IsInDoc()/GetDocument() callers and make sure they can deal with a change in
> the behavior here or something.

Alternatively, if we don't put the children in document we have to deal with:

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8422136 [details] [diff] [review]
> Part 1: Remove "in document" flag on ShadowRoot and its descendants.
> 
> So this approach increases size of nsINode 4 or 8 bytes :/

(In reply to Olli Pettay [:smaug] from comment #23)
> Then we don't need to increase the size of nsINode, and only InDoc() nodes
> can have frames.
> (That is also a thing which code tend to rely on)

I talked with sicking about this and he recommended that we don't put the ShadowRoot elements in doc, despite the increase in node size.

I'm now torn about what approach to take.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(In reply to William Chen [:wchen] from comment #24)
> If we put ShadowRoot elements in document we have to deal with:

ShadowRoot is not an element.

In my mind ShadowRoot, which is a DocumentFragment wouldn't ever be in document, but
its children would be in document if shadow host is in document.
I think that would be the least surprising behavior and not too far from the XBL setup.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand why it needs to take an additional 4-8 bytes?

I could see that if we don't set the "in document" flag, that we might need a "in shadowroot" flag. But that's one bit, not 4-8 bytes (unless we happen to be out of bits. But if we are, we'll have to add more bits eventually anyway).

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Currently InDoc() means document is the root of subtree, and union can be used for the primary frame.
In case !InDoc() union points to the root of the subtree (since there can't be primary frame)

Ín shadow dom case root of subtree would need to be defined anyway. I'd expect root to be
the root of shadow host, not ShadowRoot (but don't have strong opinion about that).

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What is the "root of subtree" for shadow DOM? Should it be the node you reach if you follow the parent chain (i.e. the ShadowRoot doc-fragment) or should it be the document? I'm also curious *why* we should use one or the other.

Anyhow, if it should be the document, then we could still just use the current union and do

nsINode* SubtreeRoot() {
  return (IsInDoc() || IsInShadowRootInDoc()) ? OwnerDocAsNode() : mSubtreeRoot;
}

where IsInShadowRootInDoc() is a flag we'd set if and only if a node is inserted into a shadowroot and the shadow host has the IsInDoc() flag set. This is a useful flag to have anyway in order to know when a in-shadowdom-stylesheet should be activated, and when a in-shadowdom-script should run etc.


If the "root of subtree" should be the node you reach if you follow the parent chain, then it appears that we couldn't keep the union either way?

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The reason I don't want to set the IsInDoc() flag for shadow content is that it has a very gecko-specific effect. It will cause some set of elements to behave differently, but which elements and how they behave differently is basically due to some very gecko-specific implementation details.

For example it depends on if we decide that elements should check their IsInDoc() flag and then notify their OwnerDoc(), or if we do it by having the owner document use nsIMutationObserver and walking subtrees to find elements. Currently we implement some features using one strategy and some features using the other strategy. Which one we choose is a random combination of performance, ease of coding, how often we think features are used etc.

I don't want those implementation decisions to leak out to the web.

Comment 30

[William Chen [:wchen]]

Attachment: Part 1: Remove "in document" flag on ShadowRoot and its descendants. v2

I've updated the patch to keep the union. It looks like SubtreeRoot() expects the node you get if you keep walking GetParentNode(). For elements in the shadow tree, this is the shadow root and we already keep track of that. I've made it so that we don't use the mSubtreeRoot member of the union for content in the shadow tree.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Have you pushed the patch to try?

Comment 32

[William Chen [:wchen]]

https://tbpl.mozilla.org/?tree=Try&rev=92f9ba494c20

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8423449 [details] [diff] [review]
Part 1: Remove "in document" flag on ShadowRoot and its descendants. v2

>   nsIFrame* GetPrimaryFrame() const
>   {
>-    return IsInDoc() ? mPrimaryFrame : nullptr;
>+    return (IsInDoc() || HasFlag(NODE_IS_IN_SHADOW_TREE)) ? mPrimaryFrame : nullptr;
>   }
So this would return non-null nsIFrame* pointing to an nINode for ShadowRoot.


>   void SetPrimaryFrame(nsIFrame* aFrame) {
>-    NS_ASSERTION(IsInDoc(), "This will end badly!");
>+    NS_ASSERTION(IsInDoc() || HasFlag(NODE_IS_IN_SHADOW_TREE), "This will end badly!");
Could you make this MOZ_ASSERT


>   /**
>+   * This method gets the current doc of the node hosting this content
>+   * or the current doc of this content if it is not being hosted. This
>+   * method walks through ShadowRoot boundaries until it reach the host
>+   * that is located in the root of the "tree of trees" (see Shadow DOM
>+   * spec) and returns the current doc for that host.
>+   */
>+  nsIDocument *GetCrossShadowCurrentDoc();
* goes with the type.

So this method is slower than GetCurrentDoc().
Could we perhaps take the slow path only in case of HasFlag(NODE_IS_IN_SHADOW_TREE) , and otherwise inline the call.
Something like
return HasFlag(NODE_IS_IN_SHADOW_TREE) ? GetCrossShadowCurrentDocInternal() : GetCurrentDoc();
And GetCrossShadowCurrentDocInternal() would do the shadowroot-host iteration.

>@@ -1415,16 +1417,17 @@ Element::UnbindFromTree(bool aDeep, bool
>       mParent = nullptr;
>       NS_RELEASE(p);
>     } else {
>       mParent = nullptr;
>     }
>     SetParentIsContent(false);
>   }
>   ClearInDocument();
>+  UnsetFlags(NODE_IS_IN_SHADOW_TREE);
Hmm, how does this shadow dom work... do elements in shadow dom not get UnbindFromTree call when host is unbound?
I guess so.

>+nsINode::SubtreeRoot() const
>+{
>+  // There are four cases of interest here.  nsINodes that are really:
>+  // 1. nsIDocument nodes - Are always in the document.
>+  // 2.a nsIContent nodes not in a shadow tree - Are either in the document,
>+  //     or mSubtreeRoot is updated in BindToTree/UnbindFromTree.
>+  // 2.b nsIContent nodes in a shadow tree - Are never in the document,
>+  //     ignore mSubtreeRoot and return the containing shadow root.
>+  // 4. nsIAttribute nodes - Are never in the document, and mSubtreeRoot
>+  //    is always 'this' (as set in nsINode's ctor).
>+  nsINode* node;
>+  if (IsInDoc()) {
>+    node = OwnerDocAsNode();
>+  } else if (IsNodeOfType(eCONTENT)) {
IsContent() please. That is not virtual.

>+nsINode::GetCrossShadowCurrentDoc()
>+{
>+  if (HasFlag(NODE_IS_IN_SHADOW_TREE) && IsNodeOfType(eCONTENT)) {
IsContent()

The following issues could be fixed in followup patches.

It is not clear why things like Element::NotifyStateChange/Element::UpdateState/Element::NotifyStyleStateChange or Element::ScrollIntoView would work.

Also nsIContent::GetDesiredIMEState() hints that IME handling is broken for shadow dom.

Element::CreateShadowRoot seems to rely GetCurrentDoc() to return non-null if there are frames.

Also Element::SetSMILOverrideStyleRule wants probably GetCrossShadowCurrentDoc as does
Element::PostHandleEventForLinks (though, I'm not sure about link handling in shadow dom. The sane approach is to handle them normally).

Link::LinkState()/Link::ResetLinkState() don't deal with stuff not in document.

FragmentOrElement::CanSkip* could use GetCrossShadowCurrentDoc()


nsContentUtils::IsSubDocumentTabbable should probably use GetCrossShadowCurrentDoc() so that we can tab into an iframe inside shadow dom.

nsPointerLockPermissionRequest::Run should use GetCrossShadowCurrentDoc, and the pointer lock related stuff in nsDocument::Observe

IsInvisibleBreak should use GetCrossShadowCurrentDoc, and nsFrameLoader::Create and nsFrameLoader::ShowRemoteFrame and nsINode::Traverse

CheckPluginStopEvent::Run() probably should use GetCrossShadowCurrentDoc, and probably also nsSimplePluginEvent() and nsObjectLoadingContent::UnbindFromTree
and nsObjectLoadingContent::InstantiatePluginInstance


...and that was for content/base/*

rest of the GetCurrentDoc() usage needs to be reviewed. and content/base/* re-reviewed.

But r- for the patch anyhow.

Comment 34

[William Chen [:wchen]]

Attachment: Part 1: Remove "in document" flag on ShadowRoot and its descendants. v3

(In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #33)
> Comment on attachment 8423449 [details] [diff] [review]
> Part 1: Remove "in document" flag on ShadowRoot and its descendants. v2
> 
> >   nsIFrame* GetPrimaryFrame() const
> >   {
> >-    return IsInDoc() ? mPrimaryFrame : nullptr;
> >+    return (IsInDoc() || HasFlag(NODE_IS_IN_SHADOW_TREE)) ? mPrimaryFrame : nullptr;
> >   }
> So this would return non-null nsIFrame* pointing to an nINode for ShadowRoot.
> 
I've updated ShadowRoot to clear the mSubtreeRoot pointer on construction.
> 
> >@@ -1415,16 +1417,17 @@ Element::UnbindFromTree(bool aDeep, bool
> >       mParent = nullptr;
> >       NS_RELEASE(p);
> >     } else {
> >       mParent = nullptr;
> >     }
> >     SetParentIsContent(false);
> >   }
> >   ClearInDocument();
> >+  UnsetFlags(NODE_IS_IN_SHADOW_TREE);
> Hmm, how does this shadow dom work... do elements in shadow dom not get
> UnbindFromTree call when host is unbound?
> I guess so.
> 
That's right, elements in the shadow DOM don't get the UnbindFromTree call when the host is unbound. They are still bound to the shadow tree. Unlike XBL, the shadow DOM doesn't get torn down when the host is unbound. The ShadowRoot and nodes in the shadow tree have API that still needs to work regardless of whether the host is bound or not.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

William: Is it correct to say that BindToTree and UnbindFromTree for Shadow-Dom behaves exactly like BindToTree and UnbindFromTree for content in a document fragment?

I.e. when you are inserted into the fragment you get BindToTree, when you are removed from the fragment you get UnbindFromTree. When the shadow host is removed from or inserted into the document, or anywhere else, there are no BindToTree/UnbindFromTree calls to the content in the shadow DOM.

Comment 36

[William Chen [:wchen]]

sicking: Yes, that is how it works right now.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8431029 [details] [diff] [review]
Part 1: Remove "in document" flag on ShadowRoot and its descendants. v3

> ShadowRoot::~ShadowRoot()
> {
>   if (mPoolHost) {
>     // mPoolHost may have been unlinked or a new ShadowRoot may have been
>     // creating, making this one obsolete.
>     mPoolHost->RemoveMutationObserver(this);
>   }
> 
>-  ClearInDocument();
>+  UnsetFlags(NODE_IS_IN_SHADOW_TREE);
>+
>+  SetSubtreeRootPointer(this);
This could use a comment that nsINode dtor expects mSubtreeRoot == this

>+nsINode::GetCrossShadowCurrentDocInternal() const
>+{
>+  if (HasFlag(NODE_IS_IN_SHADOW_TREE) && IsContent()) {
Just MOZ_ASSERT here that 
HasFlag(NODE_IS_IN_SHADOW_TREE) && IsContent()



>+    // Walk through ShadowRoot boundary to the host if this node is
>+    // contained in a shadow tree.
>+    const nsIContent* content = static_cast<const nsIContent*>(this);
Use AsContent() here (and elsewhere in the patch), no need for static_cast

I assume following patches will increase usage of GetCrossShadowCurrentDoc

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8422137 [details] [diff] [review]
Make styles work when not in document, but contained by a ShadowRoot with a host in document.

>+  if (mOwningNode && !mDisabled &&
>+      mOwningNode->HasFlag(NODE_IS_IN_SHADOW_TREE) &&
>+      mOwningNode->IsNodeOfType(nsINode::eCONTENT)) {
mOwningNode->IsContent();

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8422138 [details] [diff] [review]
Part 3: Make script run when not in document, but contained by a ShadowRoot with a host in document.

Looks like this is a wrong patch. Not about scripts.

Comment 40

[William Chen [:wchen]]

Attachment: Part 3: Make script run when not in document, but contained by a ShadowRoot with a host in document.

wrong patch

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8422141 [details] [diff] [review]
Part 5: Bind native anonymous content to null document when node is contained by a ShadowRoot.

This is scary, but makes sense.

Comment 42

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78af7d36d4a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8968e7307508
https://hg.mozilla.org/integration/mozilla-inbound/rev/a93e92a37640
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb404958dd81
https://hg.mozilla.org/integration/mozilla-inbound/rev/8730ce555151

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78af7d36d4a6
https://hg.mozilla.org/mozilla-central/rev/8968e7307508
https://hg.mozilla.org/mozilla-central/rev/a93e92a37640
https://hg.mozilla.org/mozilla-central/rev/cb404958dd81
https://hg.mozilla.org/mozilla-central/rev/8730ce555151

Comment 44

[Eitan Isaacson [:eeejay]]

The patches introduced here break accessibility. I'll file a bug in the a11y component, but i bet the original author of these patches (wchen?) has the best idea of how to fix this.