Bug 51914

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

Created attachment 79147 [details] Patch

(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?

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.

Created attachment 79234 [details] Patch

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.

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? :)

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

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

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)/

(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...

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.

Reverted r75995 for reason: Causes Committed r76001: <http://trac.webkit.org/changeset/76001>

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.

Created attachment 79386 [details] Patch

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.

Did you run the tests in debug and release?

Comment on attachment 79386 [details] Patch As discussed.

Created attachment 79396 [details] Patch

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.

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).

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

> 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.

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