Have Element::ensureUserAgentShadowRoot() return a reference

Source/core/dom/shadow/ElementShadow.cpp

Index: Source/core/dom/shadow/ElementShadow.cpp
diff --git a/Source/core/dom/shadow/ElementShadow.cpp b/Source/core/dom/shadow/ElementShadow.cpp
index 158addd3d0da4180463f8863a020789d291d6a10..8d4379001531ae8cf7a9f36fe4318c890a6819a8 100644
--- a/Source/core/dom/shadow/ElementShadow.cpp
+++ b/Source/core/dom/shadow/ElementShadow.cpp
@@ -131,7 +131,7 @@ ElementShadow::~ElementShadow()
     removeAllShadowRoots();
 }
 
-ShadowRoot* ElementShadow::addShadowRoot(Element& shadowHost, ShadowRoot::ShadowRootType type)
+ShadowRoot& ElementShadow::addShadowRoot(Element& shadowHost, ShadowRoot::ShadowRootType type)
 {
     RefPtr<ShadowRoot> shadowRoot = ShadowRoot::create(&shadowHost.document(), type);
 
@@ -149,7 +149,7 @@ ShadowRoot* ElementShadow::addShadowRoot(Element& shadowHost, ShadowRoot::Shadow
     shadowHost.didAddShadowRoot(*shadowRoot);
     InspectorInstrumentation::didPushShadowRoot(&shadowHost, shadowRoot.get());
 
-    return shadowRoot.get();
+    return *shadowRoot;

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

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]

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:28]

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?

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

Done.

 }
 
 void ElementShadow::removeAllShadowRoots()