Bug 110064 – Let JS wrapper manage Attr node lifetime instead of Element.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: DOM

[Andreas Kling]

Let JS wrapper manage Attr node lifetime instead of Element.

[Andreas Kling]

Created attachment 188781 [details]
Proposed patch

[Andreas Kling]

Created attachment 188782 [details]
Proposed patch

Oops, wrong file.

[Alexey Proskuryakov]

Comment on attachment 188782 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188782&action=review

> Source/WebCore/ChangeLog:10
> +        In practice, this means that Attr nodes disappear when the last JS wrapper is GC'd.

What about XPath? It creates Attr nodes without wrappers.

[Geoffrey Garen]

Comment on attachment 188782 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188782&action=review

> Source/WebCore/dom/Attr.h:50
> -    Element* ownerElement() const { return m_element; }
> +    Element* ownerElement() const { return m_element.get(); }

This is no longer a good name, since the Element does not own the Attr.

[Andreas Kling]

(In reply to comment #4)
> (From update of attachment 188782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188782&action=review
> 
> > Source/WebCore/dom/Attr.h:50
> > -    Element* ownerElement() const { return m_element; }
> > +    Element* ownerElement() const { return m_element.get(); }
> 
> This is no longer a good name, since the Element does not own the Attr.

Alas, it's DOM API. I can add a comment about how it's not really owned by the Element.

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 188782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188782&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        In practice, this means that Attr nodes disappear when the last JS wrapper is GC'd.
> 
> What about XPath? It creates Attr nodes without wrappers.

That ChangeLog comment was probably a little too general. Attr nodes are still like any other nodes, in that you stash them in a RefPtr and go about your business. AFAICT the XPath code doesn't rely on Element retaining instantiated Attr nodes forever. A better comment might be:

In practice, this means that Attr nodes are not kept alive indefinitely by their "owner" Element.

[Andreas Kling]

Created attachment 188944 [details]
Proposed patch

- Bit more fleshy ChangeLog.
- Removed detaching of Attr nodes in ~Element() since Element won't be destroyed as long as Attr nodes are attached.
- Added a comment about Attr::ownerElement() being named that way because of DOM API.

[Adam Barth]

Comment on attachment 188944 [details]
Proposed patch

Would you be willing to update V8GCController::opaqueRootForGC appropriately for this change?

[Adam Barth]

Comment on attachment 188944 [details]
Proposed patch

Doesn't this mess up the DOM semantics?  Support JavaScript has a reference to the Attr and no other references to Nodes.  Now a GC will delete the wrapper from the Attr's ownerElement, which will be observable.

[Adam Barth]

s/Support/Suppose/

[Adam Barth]

Worse, won't GC now delete all the nodes in the tree except the nodes including-and-below the ownerElement?  That's observable because attr.ownerElement.parentNode will be null incorrectly.