Bug 51914 – ElementRareData::m_shadowRoot should not be RefPtr.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Currently ElementRareData::m_shadowRoot is RefPtr. 
But I think it should be a raw pointer instead of smart one,
assuming it should be same as  ContainerNode::m_firstChild.

In general, WebKit doesn't use RefPtr for maintaining tree structure.
So we should follow that style to avoid confusion.

[Dimitri Glazkov (Google)]

You're right -- now that we use  TreeShared::parent to store the shadow host value, we can get rid of RefPtr.

[Hajime Morrita]

Created attachment 79147 [details]
Patch

[Hajime Morrita]

(In reply to comment #1)
> You're right -- now that we use  TreeShared::parent to store the shadow host value, we can get rid of RefPtr.
Hi Dimitri, so I do it. Could you take a look at this small change?

[Dimitri Glazkov (Google)]

Comment on attachment 79147 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:1090
>  void Element::setShadowRoot(PassRefPtr<Node> node)

We're not passing RefPtr anymore, since it'll be stored as a raw ptr. Should we still have PassRefPtr as an argument?

> Source/WebCore/dom/Element.cpp:1110
> +    ElementRareData* data = rareData();
> +    if (data->m_shadowRoot) {

Could be written in one line as "if (ElementRareData* data = rareData()) {"

> Source/WebCore/dom/Element.cpp:1111
> +        RefPtr<Node> oldRoot = adoptRef(data->m_shadowRoot);

adoptRef, are you sure? You actually  do want to increment the ref count here.

[Hajime Morrita]

Created attachment 79234 [details]
Patch

[Hajime Morrita]

Hi Dimitri, thank you for your quick review!
I updated the patch could you take the next round?

> > Source/WebCore/dom/Element.cpp:1090
> >  void Element::setShadowRoot(PassRefPtr<Node> node)
> 
> We're not passing RefPtr anymore, since it'll be stored as a raw ptr. Should we still have PassRefPtr as an argument?
Yes. I once tried to switch a raw pointer and notice that it is inconvenient because
SomeElement::create() returns PassRefPtr. 
It looks that arguments for new nodes are declared as PassRefPtr, and raw pointer for existing pointer.
Here is the declaration for Node::insertBefore():

    bool insertBefore(PassRefPtr<Node> newChild, Node* refChild, ExceptionCode&, bool shouldLazyAttach = false);
 
> 
> > Source/WebCore/dom/Element.cpp:1110
> > +    ElementRareData* data = rareData();
> > +    if (data->m_shadowRoot) {
> 
> Could be written in one line as "if (ElementRareData* data = rareData()) {"
Sure. Fixed.

> 
> > Source/WebCore/dom/Element.cpp:1111
> > +        RefPtr<Node> oldRoot = adoptRef(data->m_shadowRoot);
> 
> adoptRef, are you sure? You actually  do want to increment the ref count here.
You are right. Fixed.

[Dimitri Glazkov (Google)]

Comment on attachment 79234 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:1110
> +    ElementRareData* data = rareData();
> +    if (data->m_shadowRoot) {

Forgot this one? :)

[Hajime Morrita]

(In reply to comment #7)
> 
> Forgot this one? :)
Ooops! I'm fixing it and landing.
Thank you for the r+ :p

[Hajime Morrita]

Committed r75995: <http://trac.webkit.org/changeset/75995>

[Simon Fraser (smfr)]

Leopard Intel Debug is showing a bunch of new crashing tests after this change:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r75995%20(26049)/

[Hajime Morrita]

(In reply to comment #10)
> Leopard Intel Debug is showing a bunch of new crashing tests after this change:
> http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r75995%20(26049)/
Hmm. Thank you for letting me know. I'll take a look...

[David Levin]

As noted in IRC, a ref count is being leaked here due to the leakRef without a corresponding adoptRef (or deref) call.

Also, having a method take a PassRefPtr when it does take ownership (and I think that is your intention) seems really wrong and basically means anyone reading the method will think the wrong thing.

[Hajime Morrita]

Reverted r75995 for reason:

Causes

Committed r76001: <http://trac.webkit.org/changeset/76001>

[Hajime Morrita]

note from irc:
> morrita: Your patch looks wrong to me. You got rid of RefPtr but you are using leakPtr which mean you are putting a ref counted object into taht pointer.
> morrita: Then it doesn't do adoptRef (when putting out the pointer) but if you were doing that then why get rid of teh RefPtr at all because you'd be doing the same thing but all by hand.
> morrita: Also having a method take a PassRefPtr when it doesn't take ownership seems very wrong.

----
So Dimitri is right.  Element::setShadowRoot() should accept raw pointer of Node. 
The revised patch will come shortly.

[Hajime Morrita]

Created attachment 79386 [details]
Patch

[Hajime Morrita]

And fixed again... Could anyone take a look?
The difference is:
- Element::setShadowRoot() no longer accept PassRefPtr
   and accept a raw pointer instead. On caller side, RefPtr::get() is used for revealing the raw pointer.
- Removed the leakPtr() call.

[Dimitri Glazkov (Google)]

Did you run the tests in debug and release?

[David Levin]

Comment on attachment 79386 [details]
Patch

As discussed.

[Hajime Morrita]

Created attachment 79396 [details]
Patch

[Hajime Morrita]

Levin, thank you for take a look!
I updated the patch which

- changed setShadowRoot() argument type from raw pointer to PassRefPtr, and 
- removed shadow node on Element dtor.

Note that this change calls removeShadowRoot() in Element dtor,
instead of deleting node in ElementRareData dtor.
This is because the ElementRareData dtor is called *after* the Elment dtor. It is called in the Node dtor. 
I think holding that half-dead pointer is potentially dangerous so clear it early.

[David Levin]

Comment on attachment 79396 [details]
Patch

TreeShared is a little magical (the way that ref count goes to zero but it stays alive because it has a parent).

I see how this is finally deleted now.  It happens in removeShadowRoot.  The ref count goes to 1. Here:
   if (RefPtr<Node> oldRoot = data->m_shadowRoot) {

Then the parent is removed and when the "if" is exited, "oldRoot" is deref'ed so the ref count goes to zero and there is no parent so the node is destroyed.

This is a bit tricky, but it seems fine. (I wonder if there should be some comments somewhere about how lifetime is managed in here. It takes some detective work to figure it all out.)

Please make sure to run debug test before submitting (and/or use the commit queue).

[Hajime Morrita]

Thanks! I should be careful enough because this patch was rejected once ;-)

[Dimitri Glazkov (Google)]

> This is a bit tricky, but it seems fine. (I wonder if there should be some comments somewhere about how lifetime is managed in here. It takes some detective work to figure it all out.)

I highly encourage a nice ChangeLog with detailed description of what happened where and reasoning behind each change.

[Hajime Morrita]

Committed r76183: <http://trac.webkit.org/changeset/76183>