Oilpan: have PendingScripts trace their script elements.

Source/core/dom/PendingScript.h

Index: Source/core/dom/PendingScript.h
diff --git a/Source/core/dom/PendingScript.h b/Source/core/dom/PendingScript.h
index a5c5855819c11fa1bd04f3e38f9388af6b5d1fb9..95c8b1d003cf96d6a85ad01732775d00ead6fe50 100644
--- a/Source/core/dom/PendingScript.h
+++ b/Source/core/dom/PendingScript.h
@@ -29,6 +29,7 @@
 #include "core/fetch/ResourceClient.h"
 #include "core/fetch/ResourceOwner.h"
 #include "core/fetch/ScriptResource.h"
+#include "platform/heap/Handle.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/RefPtr.h"
 #include "wtf/text/TextPosition.h"
@@ -43,7 +44,7 @@ class ScriptResource;
 // A ResourcePtr alone does not prevent the underlying Resource
 // from purging its data buffer. This class holds a dummy client open for its
 // lifetime in order to guarantee that the data buffer will not be purged.
-class PendingScript : public ResourceOwner<ScriptResource> {
+class PendingScript FINAL : public ResourceOwner<ScriptResource> {
 public:
     PendingScript()
         : m_watchingForLoad(false)
@@ -90,15 +91,31 @@ public:
 
     Element* element() const { return m_element.get(); }
     void setElement(Element* element) { m_element = element; }
-    PassRefPtr<Element> releaseElementAndClear();
+    PassRefPtrWillBeRawPtr<Element> releaseElementAndClear();
 
     void setScriptResource(ScriptResource*);
 
     virtual void notifyFinished(Resource*);
 
+    void trace(Visitor*);
+
 private:
     bool m_watchingForLoad;
-    RefPtr<Element> m_element;
+    // FIXME: Oilpan: PendingScript must trace its script element, but
+    // turning the object into a GCed object is problematic. Due to
+    // external ownership arrangements: a PendingScript is
+    // conceptually owned by a script runner object, which when
+    // finalized will have to coordinate the release of the pending
+    // script resource with that of its owner/host
+    // (cf. HTMLScriptRunner finalization.) Doing that when host +
+    // owner + PendingScript are all heap allocated and independently
+    // finalized is not easily achieveable, hence we currently keep
+    // PendingScript off heap, along with its immediate owner.

[haraken, 2014/05/26 01:08:25]

Hmm, can we probably address the issue using weak processing?

By calling registerWeakMembers(this, weakCallback), the weak processing is
called back when the |this| object is going to die. In the weak processing,
you're allowed to touch other on-heap objects that might die in the same GC
cycle. (However, you're not allowed to allocate anything.)

[sof, 2014/05/26 09:41:34]

If HTMLScriptRunner and HTMLDocumentParser both die at the same time, then no
weak callback gets to run, right? (as that's dependent on the trace() of either
gets to run & register a weak callback.)

If so, then I don't see how this could work in this context.

[Mads Ager (chromium), 2014/05/26 13:55:36]

Yeah, I see the issue. For now I think we need to keep the OwnPtrs here to make
sure that these owned objects are basically part of the DocumentParser so it can
unregister itself from the pending resources when it dies.

I guess one way of dealing with this longer term would be to get Resource onto
our heap and make the set of clients a weak collection.

The current code is pretty nasty:

DocumentParser
    OwnPtr<HTMLScriptRunner>

~HTMLScriptRunner
    stopWatchingForLoad(...);

void HTMLScriptRunner::stopWatchingForLoad(PendingScript& pendingScript)
{
    ASSERT(pendingScript.watchingForLoad());
    m_host->stopWatchingForLoad(pendingScript.resource());
    pendingScript.setWatchingForLoad(false);
}

We are at the end of the destruction of m_host at this point, but let's call a
method on it! :-)

[sof, 2014/05/26 21:10:18]

That gave me an idea -- if HTMLScriptRunner is made the ResourceClient instead,
and it is able to handle the finalization without relying on the 'host object',
then we can simplify. The HTMLScriptRunner would then have to redirect the
notifications of finished resource loads to its host, but I think that's an
acceptable arrangement.

+    //
+    // Do want to trace() PendingScripts, so we do annotate the
+    // element as being a Member, but disable GC plugin checks for it.
+    GC_PLUGIN_IGNORE("357163")
+    RefPtrWillBeMember<Element> m_element;
     TextPosition m_startingPosition; // Only used for inline script tags.
 };