Have Element::ensureUserAgentShadowRoot() return a reference

Have Element::ensureUserAgentShadowRoot() return a reference

Have Element::ensureUserAgentShadowRoot() return a reference instead of
a pointer as it can never return null.

R=adamk

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161631

[adamk, 2013-11-06 17:15:03]

This overlaps almost completely with https://codereview.chromium.org/59113007/,
which eseidel expressed concerns about that I was hoping to soothe this morning
in in-person conversation. Will update when I talk to him.

[eseidel, 2013-11-06 20:18:39]

Discussed with adamk in person.

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
Source/core/dom/shadow/ElementShadow.cpp:152: return *shadowRoot;
It's generally not safe to return a reference from a RefPtr.  It happens to be
in this case.  But that's the core confusion of this code.  The RefPtr in this
function should only exist as long as needed and then teh rest of the function
should use raw pointers, making clear that something else is responsible for the
lifetime of this shadowRoot and that it's OK for us to return a raw ptr to it.

[adamk, 2013-11-06 20:24:05]

lgtm

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
Source/core/dom/shadow/ElementShadow.cpp:152: return *shadowRoot;
On 2013/11/06 20:18:39, eseidel wrote:
> It's generally not safe to return a reference from a RefPtr.  It happens to be
> in this case.  But that's the core confusion of this code.  The RefPtr in this
> function should only exist as long as needed and then teh rest of the function
> should use raw pointers, making clear that something else is responsible for
the
> lifetime of this shadowRoot and that it's OK for us to return a raw ptr to it.

The easiest way to make this look safer would be to make m_shadowRoots store
RefPtrs and then return *m_shadowRoots.head() here. I think it's fine to leave
that for later, maybe put a comment here for now saying why it's safe to return
what appears to reference a RefPtr that's about to go out of scope.

[Inactive, 2013-11-07 16:54:27]

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
Source/core/dom/shadow/ElementShadow.cpp:152: return *shadowRoot;
On 2013/11/06 20:24:05, adamk wrote:
> On 2013/11/06 20:18:39, eseidel wrote:
> > It's generally not safe to return a reference from a RefPtr.  It happens to
be
> > in this case.  But that's the core confusion of this code.  The RefPtr in
this
> > function should only exist as long as needed and then teh rest of the
function
> > should use raw pointers, making clear that something else is responsible for
> the
> > lifetime of this shadowRoot and that it's OK for us to return a raw ptr to
it.
> 
> The easiest way to make this look safer would be to make m_shadowRoots store
> RefPtrs and then return *m_shadowRoots.head() here. I think it's fine to leave
> that for later, maybe put a comment here for now saying why it's safe to
return
> what appears to reference a RefPtr that's about to go out of scope.

I took another look at the code and the ownership model is still not clear to
me. m_shadowRoots is a simple doubly-linked list of raw pointers so it does not
own the new ShadowRoot object. To me it looks that ownership is passed to
addShadowRoot()'s caller. If so, why don't we return a PassRefPtr instead of a
raw pointer?

The code in Element::createShadowRoot() already constructs a PassRefPtr from the
return value.

However, the code in Element::ensureUserAgentShadowRoot() does not and uses the
ShadowRoot object after the call to addShadowRoot(). I don't understand why this
is safe so I must be misunderstanding something. At that point, the RefPtr has
gone out of scope and the doubly-linked list only keeps a raw pointer, so what
prevents the ShadowRoot object from getting destroyed?

[adamk, 2013-11-07 19:43:44]

On 2013/11/07 16:54:27, Chris Dumez wrote:
>
https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
> File Source/core/dom/shadow/ElementShadow.cpp (right):
> 
>
https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
> Source/core/dom/shadow/ElementShadow.cpp:152: return *shadowRoot;
> On 2013/11/06 20:24:05, adamk wrote:
> > On 2013/11/06 20:18:39, eseidel wrote:
> > > It's generally not safe to return a reference from a RefPtr.  It happens
to
> be
> > > in this case.  But that's the core confusion of this code.  The RefPtr in
> this
> > > function should only exist as long as needed and then teh rest of the
> function
> > > should use raw pointers, making clear that something else is responsible
for
> > the
> > > lifetime of this shadowRoot and that it's OK for us to return a raw ptr to
> it.
> > 
> > The easiest way to make this look safer would be to make m_shadowRoots store
> > RefPtrs and then return *m_shadowRoots.head() here. I think it's fine to
leave
> > that for later, maybe put a comment here for now saying why it's safe to
> return
> > what appears to reference a RefPtr that's about to go out of scope.
> 
> I took another look at the code and the ownership model is still not clear to
> me. m_shadowRoots is a simple doubly-linked list of raw pointers so it does
not
> own the new ShadowRoot object. To me it looks that ownership is passed to
> addShadowRoot()'s caller. If so, why don't we return a PassRefPtr instead of a
> raw pointer?
> 
> The code in Element::createShadowRoot() already constructs a PassRefPtr from
the
> return value.
> 
> However, the code in Element::ensureUserAgentShadowRoot() does not and uses
the
> ShadowRoot object after the call to addShadowRoot(). I don't understand why
this
> is safe so I must be misunderstanding something. At that point, the RefPtr has
> gone out of scope and the doubly-linked list only keeps a raw pointer, so what
> prevents the ShadowRoot object from getting destroyed?

Congratulations, you have stumbled upon perhaps the most confusing piece of
memory management in Blink (and WebKit): TreeShared (see core/dom/TreeShared.h).
All Nodes get their RefCounted behavior not from wtf/RefCounted.h, but from
TreeShared. TreeShared objects are alive as long as

1. Their refcount is >0
AND
2. They have a parent (....or shadow host)

So the thing that's actually keeping the shadow root alive is the
shadowRoot->setParentOrShadowHostNode() call. Fun, yes?

[Inactive, 2013-11-07 19:48:35]

On 2013/11/07 19:43:44, adamk wrote:
> On 2013/11/07 16:54:27, Chris Dumez wrote:
> >
>
https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
> > File Source/core/dom/shadow/ElementShadow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
> > Source/core/dom/shadow/ElementShadow.cpp:152: return *shadowRoot;
> > On 2013/11/06 20:24:05, adamk wrote:
> > > On 2013/11/06 20:18:39, eseidel wrote:
> > > > It's generally not safe to return a reference from a RefPtr.  It happens
> to
> > be
> > > > in this case.  But that's the core confusion of this code.  The RefPtr
in
> > this
> > > > function should only exist as long as needed and then teh rest of the
> > function
> > > > should use raw pointers, making clear that something else is responsible
> for
> > > the
> > > > lifetime of this shadowRoot and that it's OK for us to return a raw ptr
to
> > it.
> > > 
> > > The easiest way to make this look safer would be to make m_shadowRoots
store
> > > RefPtrs and then return *m_shadowRoots.head() here. I think it's fine to
> leave
> > > that for later, maybe put a comment here for now saying why it's safe to
> > return
> > > what appears to reference a RefPtr that's about to go out of scope.
> > 
> > I took another look at the code and the ownership model is still not clear
to
> > me. m_shadowRoots is a simple doubly-linked list of raw pointers so it does
> not
> > own the new ShadowRoot object. To me it looks that ownership is passed to
> > addShadowRoot()'s caller. If so, why don't we return a PassRefPtr instead of
a
> > raw pointer?
> > 
> > The code in Element::createShadowRoot() already constructs a PassRefPtr from
> the
> > return value.
> > 
> > However, the code in Element::ensureUserAgentShadowRoot() does not and uses
> the
> > ShadowRoot object after the call to addShadowRoot(). I don't understand why
> this
> > is safe so I must be misunderstanding something. At that point, the RefPtr
has
> > gone out of scope and the doubly-linked list only keeps a raw pointer, so
what
> > prevents the ShadowRoot object from getting destroyed?
> 
> Congratulations, you have stumbled upon perhaps the most confusing piece of
> memory management in Blink (and WebKit): TreeShared (see
core/dom/TreeShared.h).
> All Nodes get their RefCounted behavior not from wtf/RefCounted.h, but from
> TreeShared. TreeShared objects are alive as long as
> 
> 1. Their refcount is >0
> AND
> 2. They have a parent (....or shadow host)
> 
> So the thing that's actually keeping the shadow root alive is the
> shadowRoot->setParentOrShadowHostNode() call. Fun, yes?

Thanks for the explanation Adam. Fun indeed :)

[Inactive, 2013-11-07 20:03:27]

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/1/Source/core/dom/shadow/Elemen...
Source/core/dom/shadow/ElementShadow.cpp:152: return *shadowRoot;
On 2013/11/06 20:24:05, adamk wrote:
> On 2013/11/06 20:18:39, eseidel wrote:
> > It's generally not safe to return a reference from a RefPtr.  It happens to
be
> > in this case.  But that's the core confusion of this code.  The RefPtr in
this
> > function should only exist as long as needed and then teh rest of the
function
> > should use raw pointers, making clear that something else is responsible for
> the
> > lifetime of this shadowRoot and that it's OK for us to return a raw ptr to
it.
> 
> The easiest way to make this look safer would be to make m_shadowRoots store
> RefPtrs and then return *m_shadowRoots.head() here. I think it's fine to leave
> that for later, maybe put a comment here for now saying why it's safe to
return
> what appears to reference a RefPtr that's about to go out of scope.

Done.

[adamk, 2013-11-07 20:08:18]

lgtm

[Inactive, 2013-11-07 20:10:17]

On 2013/11/07 20:08:18, adamk wrote:
> lgtm

Thanks Adam.

Eric, any objection to landing this now?

[eseidel, 2013-11-07 23:28:19]

I think ADam and I ended up on the same page after our pow-wow.  If he likes it,
I'm sure I do too.  Thanks!

[eseidel, 2013-11-07 23:30:06]

Please feel free to ignore my comments.

https://codereview.chromium.org/59463007/diff/260001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/260001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:152: ASSERT(m_shadowRoots.head());
Did you want to ASSERT that m_shadowRoots.head() == shadowRoot.get()?
I guess we could also ASSERT(m_shadowRoots->refCount() > 1) if we really wanted
to.  The ownership in thsi function is just so screwy.

[adamk, 2013-11-08 00:01:50]

https://codereview.chromium.org/59463007/diff/260001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/260001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:152: ASSERT(m_shadowRoots.head());
On 2013/11/07 23:30:07, eseidel wrote:
> Did you want to ASSERT that m_shadowRoots.head() == shadowRoot.get()?
> I guess we could also ASSERT(m_shadowRoots->refCount() > 1) if we really
wanted
> to.  The ownership in thsi function is just so screwy.

The ASSERT you suggest will fail, so it can't be added without making some
actual functional changes to this code.

[Inactive, 2013-11-08 15:09:37]

https://codereview.chromium.org/59463007/diff/260001/Source/core/dom/shadow/E...
File Source/core/dom/shadow/ElementShadow.cpp (right):

https://codereview.chromium.org/59463007/diff/260001/Source/core/dom/shadow/E...
Source/core/dom/shadow/ElementShadow.cpp:152: ASSERT(m_shadowRoots.head());
On 2013/11/07 23:30:07, eseidel wrote:
> Did you want to ASSERT that m_shadowRoots.head() == shadowRoot.get()?
> I guess we could also ASSERT(m_shadowRoots->refCount() > 1) if we really
wanted
> to.  The ownership in thsi function is just so screwy.

Well, I really wanted this assertion because I want to make sure I never
dereference a null pointer. However, adding an assertion to make sure
m_shadowRoots.head() == shadowRoot.get() makes a lot of sense as well. I will
add it before landing, thanks.

[commit-bot: I haz the power, 2013-11-08 15:20:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/59463007/350001

[commit-bot: I haz the power, 2013-11-08 16:47:41]

Change committed as 161631