If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

GetCurrentDoc fixes in content/base

RESOLVED FIXED in mozilla35

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

34 Branch
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8497391 [details] [diff] [review]
content_base_getcurrentdoc.diff

https://tbpl.mozilla.org/?tree=Try&rev=14543724ba2a

There are few unclear cases, but obviously the spec isn't quite clear either.
Attachment #8497391 - Flags: review?(wchen)
(Assignee)

Comment 1

3 years ago
Hmm, that breaks something :/
(Assignee)

Comment 2

3 years ago
Created attachment 8497579 [details] [diff] [review]
v2

One silly mistake in the previous patch, and one existing issue fixed, where
nsGenericDOMDataNode::UnbindFromTree behaves differently to Element::UnbindFromTree

https://tbpl.mozilla.org/?tree=Try&rev=75b97d83ccd9
Assignee: nobody → bugs
Attachment #8497391 - Attachment is obsolete: true
Attachment #8497391 - Flags: review?(wchen)
Attachment #8497579 - Flags: review?(wchen)
(Assignee)

Comment 3

3 years ago
Created attachment 8497755 [details] [diff] [review]
additional patch

I somehow missed these.
Attachment #8497755 - Flags: review?(wchen)
Comment on attachment 8497579 [details] [diff] [review]
v2

Review of attachment 8497579 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/Element.cpp
@@ +1431,5 @@
> +  // tear down the binding. Only tear down the binding if the element
> +  // is still no longer in the DOM. nsXBLService::LoadBinding tears
> +  // down the old binding if the element is inserted back into the
> +  // DOM and loads a different binding.
> +  if (!mContent->IsInDoc()) {

It seems to me this should be IsInComposedDoc() since we also run this when the content is detatched from a shadow tree.

::: content/base/src/Link.cpp
@@ +76,5 @@
>    Element *element = self->mElement;
>  
>    // If we have not yet registered for notifications and need to,
>    // due to our href changing, register now!
> +  if (!mRegistered && mNeedsRegistration && element->IsInComposedDoc()) {

The "link" element is specified to be inert inside the shadow tree [1]. We need to also ensure that the current element is not a HTMLLinkElement, or make sure all the callers in HTMLLinkElement don't call when in a shadow tree. This goes for the other method changed in this class.

[1] http://w3c.github.io/webcomponents/spec/shadow/#inert-html-elements

::: content/base/src/nsDocument.cpp
@@ +6051,5 @@
>          }
>        }
>  
>        EnqueueLifecycleCallback(nsIDocument::eCreated, elem, nullptr, definition);
> +      //XXXsmaug It is unclear if we should use GetComposedDoc() here.

According to the spec [2], we should be using the uncomposed document here. Although, it feels like it should be spec'ed to use the composed document once it's defined.

[2] http://w3c.github.io/webcomponents/spec/custom/#types-of-callbacks

::: content/base/src/nsGenericDOMDataNode.cpp
@@ +591,5 @@
> +    // anonymous content that the document is changing.
> +    if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +      nsContentUtils::AddScriptRunner(
> +        new RemoveFromBindingManagerRunnable(document->BindingManager(), this,
> +                                             document));

I'm not sure it's a good idea to use a runnable here if we don't have to. From what I understand the reason why we use a runnable for elements is because they may have a binding and removing the binding may run destructors. I don't think there is a way to attach bindings to data nodes so all we are doing here is nulling the insertion parent and this makes it potentially run asyncronously, so we could end up with a period of time when the node is detached but still have an insertion parent.
Attachment #8497579 - Flags: review?(wchen) → review-

Updated

3 years ago
Attachment #8497755 - Flags: review?(wchen) → review+
(Assignee)

Comment 5

3 years ago
(In reply to William Chen [:wchen] from comment #4)
> Comment on attachment 8497579 [details] [diff] [review]
> v2
> 
> Review of attachment 8497579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/Element.cpp
> @@ +1431,5 @@
> > +  // tear down the binding. Only tear down the binding if the element
> > +  // is still no longer in the DOM. nsXBLService::LoadBinding tears
> > +  // down the old binding if the element is inserted back into the
> > +  // DOM and loads a different binding.
> > +  if (!mContent->IsInDoc()) {
> 
> It seems to me this should be IsInComposedDoc() since we also run this when
> the content is detatched from a shadow tree.
Indeed.


> The "link" element is specified to be inert inside the shadow tree [1]. We
> need to also ensure that the current element is not a HTMLLinkElement, or
> make sure all the callers in HTMLLinkElement don't call when in a shadow
> tree. This goes for the other method changed in this class.
> 
> [1] http://w3c.github.io/webcomponents/spec/shadow/#inert-html-elements
> 
I disagree. These Get/IsIn*Doc calls are about link state and if the link is visible we should update their state.



> ::: content/base/src/nsDocument.cpp
> @@ +6051,5 @@
> >          }
> >        }
> >  
> >        EnqueueLifecycleCallback(nsIDocument::eCreated, elem, nullptr, definition);
> > +      //XXXsmaug It is unclear if we should use GetComposedDoc() here.
> 
> According to the spec [2], we should be using the uncomposed document here.
> Although, it feels like it should be spec'ed to use the composed document
> once it's defined.
> 
> [2] http://w3c.github.io/webcomponents/spec/custom/#types-of-callbacks
Yeah, looks like custom element spec is behaving only with composed tree.
We need spec changes there.



> ::: content/base/src/nsGenericDOMDataNode.cpp
> @@ +591,5 @@
> > +    // anonymous content that the document is changing.
> > +    if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> > +      nsContentUtils::AddScriptRunner(
> > +        new RemoveFromBindingManagerRunnable(document->BindingManager(), this,
> > +                                             document));
> 
> I'm not sure it's a good idea to use a runnable here if we don't have to.
We really should be consistent with Element::UnbindFromTree.
Consistency as such is already a good enough reason to use runnable.


r+ with Element.cpp changed to use IsInComposedTree() ?
Flags: needinfo?(wchen)
Comment on attachment 8497579 [details] [diff] [review]
v2

Review of attachment 8497579 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Olli Pettay [:smaug] from comment #5)
> > The "link" element is specified to be inert inside the shadow tree [1]. We
> > need to also ensure that the current element is not a HTMLLinkElement, or
> > make sure all the callers in HTMLLinkElement don't call when in a shadow
> > tree. This goes for the other method changed in this class.
> > 
> > [1] http://w3c.github.io/webcomponents/spec/shadow/#inert-html-elements
> > 
> I disagree. These Get/IsIn*Doc calls are about link state and if the link is
> visible we should update their state.

<link> elements generally don't get a frame and aren't visible. I'm OK with this if it doesn't affect the "inertness" of <link> in the shadow DOM, but it seems like we should be using the uncomposed document in the case of <link> and other elements like <a> should use the composed document as the spec says that <link> should behave as if not part of the document tree (although, that part of the spec is one of the sections that needs some work).

> r+ with Element.cpp changed to use IsInComposedTree() ?

OK
Attachment #8497579 - Flags: review- → review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(wchen)
(Assignee)

Comment 7

3 years ago
Created attachment 8499063 [details] [diff] [review]
combined
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/929d78de0f8a
https://hg.mozilla.org/mozilla-central/rev/929d78de0f8a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.