Oilpan: move WorkerGlobalScope to oilpan's heap.

Source/core/workers/WorkerGlobalScope.h

Index: Source/core/workers/WorkerGlobalScope.h
diff --git a/Source/core/workers/WorkerGlobalScope.h b/Source/core/workers/WorkerGlobalScope.h
index 3648d24ca677ec7a9e6cfffefb88aee541d16ffb..9a1bc4d0c6433d2a6176460f965b88fcf0ba893e 100644
--- a/Source/core/workers/WorkerGlobalScope.h
+++ b/Source/core/workers/WorkerGlobalScope.h
@@ -57,8 +57,9 @@ namespace WebCore {
     class WorkerNavigator;
     class WorkerThread;
 
-    class WorkerGlobalScope : public RefCounted<WorkerGlobalScope>, public ScriptWrappable, public SecurityContext, public ExecutionContext, public ExecutionContextClient, public WorkerSupplementable, public EventTargetWithInlineData {
-        REFCOUNTED_EVENT_TARGET(WorkerGlobalScope);
+    class WorkerGlobalScope : public RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public ScriptWrappable, public SecurityContext, public ExecutionContext, public ExecutionContextClient, public WorkerSupplementable, public EventTargetWithInlineData {

[sof, 2014/02/24 22:46:24]

Just noticed: we now need to address Supplementable/Supplement, I think.

Will attend to tomorrow.

[haraken, 2014/02/25 01:30:49]

Just to help me understand: Why do we need to address Supplementable/Supplement
now? In my understanding, as long as we're using Persistent handles in
supplementing objects, it won't cause any issue even if we don't support tracing
between supplementable objects and supplementing objects.

Nit: Not related to this CL, we should remove WorkerSupplementable.h and use
Supplementable<WorkerGlobalScope>/Supplement<WorkerGlobalScope>.
WorkerSupplementable/WorkerSupplement looks like an unnecessary typedef
indirection.

[sof, 2014/02/25 08:56:35]

The problem isn't that such Persistents won't be cleared during the final GC in
ThreadState::cleanup(), but the objects that they point to won't be GCed. And we
(quite reasonably) don't run GCs until a steady state is reached.

Making the Supplements GCed would address this, I think. The alternative is to
register a 'pre-cleanup' task that clears the persistents on a case-by-case
basis. (Unless we add some macros & otherwise to make it more convenient.)

This happens with the WorkerPerformance supplement that hangs off
WorkerGlobalScope.

[haraken, 2014/02/25 11:16:28]

Let me make sure that I'm understanding correctly:

- Previously WorkerGlobalScope was destructed when WorkerThread::workerThread()
calls 'm_workerGlobalScope = nullptr'. Thus it was guaranteed that the
WorkerGlobalScope does not outlive WorkerThread.

- However, in the new code, someone holds Persistent handles to the
WorkerGlobalScope. Thus 'm_workerGlobalScope = nullptr' won't destruct the
WorkerGlobalScope. Consequently, WorkerGlobalScope will end up outliving
WorkerThread. That's the issue.

Am I understanding correctly?


If my understanding is correct, my opinions are as follows:

- Making the Supplements GCed and removing Persistent handles will solve the
issue, because it will guarantee that 'm_workerGlobalScope = nullptr' destructs
the WorkerGlobalScope.

- However, this is a fragile solution. Once we have a Persistent handle
somewhere, we will hit the same problem again. It's fragile to try to guarantee
that 'm_workerGlobalScope = nullptr' destructs the WorkerGlobalScope.

- Alternately, what we should do here would be using a Member for
WorkerGlobalScope::m_thread, by which we can guarantee that the WorkerThread and
the WorkerGlobalScope die together.

What do you think?

[sof, 2014/02/25 11:38:02]

Correct.
Essentially. No one is holding onto WorkerGlobalScope via a persistent, but
it has Supplements with Persistents. So when WorkerGlobalScope is finalized,
those Supplements will be destructed & their persistent references let go of.
Those objects won't be GCed.
Clearing m_workerGlobalScope won't destruct the object, but become garbage.
But both it and its supplements will be swept out at the next GC (which
strikes during worker thread cleanup just below.)

(I'm currently changing Supplementable to do this; dealing with some template
instantiation issues atm.)
Yes, registering pre-cleanup tasks can be used to address this, should it arise.

[sof, 2014/02/25 21:56:43]

WorkerGlobalScope is now a GCed Supplementable along with its Supplements.

+        DECLARE_GC_INFO;
+        DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>);
     public:
         virtual ~WorkerGlobalScope();
 
@@ -123,6 +124,8 @@ namespace WebCore {
         using SecurityContext::securityOrigin;
         using SecurityContext::contentSecurityPolicy;
 
+        virtual void trace(Visitor*);
+
     protected:
         WorkerGlobalScope(const KURL&, const String& userAgent, WorkerThread*, double timeOrigin, PassOwnPtr<WorkerClients>);
         void applyContentSecurityPolicyFromString(const String& contentSecurityPolicy, ContentSecurityPolicy::HeaderType);
@@ -146,9 +149,9 @@ namespace WebCore {
         KURL m_url;
         String m_userAgent;
 
-        mutable RefPtr<WorkerConsole> m_console;
-        mutable RefPtr<WorkerLocation> m_location;
-        mutable RefPtr<WorkerNavigator> m_navigator;
+        mutable RefPtrWillBeMember<WorkerConsole> m_console;
+        mutable RefPtrWillBeMember<WorkerLocation> m_location;
+        mutable RefPtrWillBeMember<WorkerNavigator> m_navigator;
 
         OwnPtr<WorkerScriptController> m_script;
         WorkerThread* m_thread;

[haraken, 2014/02/25 01:30:49]

This raw pointer will cause an issue.

- WorkerThread is off the heap.
- WorkerGlobalScope is on the heap.
- Thus WorkerGlobalScope can outlive WorkerThread.
- ~WorkerGlobalScope() touches m_thread. It can be a stale pointer.

[sof, 2014/02/25 07:46:44]

If you look at WorkerThread::workerThread(), it assumes that the global scope is
promptly deleted upon calling deref() on it.

I think that needs to be adjusted here, which should avoid the need for moving
WorkerThread into the heap in this CL (which would still be problematic given
the lack of finalization order.)

[Vyacheslav Egorov (Chromium), 2014/02/25 10:41:27]

WorkerGlobalScope can't outlive the thread because it is allocated on the thread
owned heap right now and all objects from the thread owned heap are destroyed on
ThreadState detachment (and we assert that the heap is empty).

@@ -156,8 +159,6 @@ namespace WebCore {
         OwnPtr<WorkerInspectorController> m_workerInspectorController;
         bool m_closing;
 
-        HashSet<Observer*> m_workerObservers;
-
         OwnPtr<WorkerEventQueue> m_eventQueue;
 
         OwnPtr<WorkerClients> m_workerClients;