Oilpan: move WorkerGlobalScope to oilpan's heap.

Source/core/workers/WorkerGlobalScope.h

 /*
  * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 39 matching lines @@
     class Blob;
     class ExceptionState;
     class ScheduledAction;
     class WorkerClients;
     class WorkerConsole;
     class WorkerInspectorController;
     class WorkerLocation;
     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();
 
         virtual bool isWorkerGlobalScope() const OVERRIDE FINAL { return true; }
 
         virtual ExecutionContext* executionContext() const OVERRIDE FINAL;
 
         virtual bool isSharedWorkerGlobalScope() const { return false; }
         virtual bool isDedicatedWorkerGlobalScope() const { return false; }
         virtual bool isServiceWorkerGlobalScope() const { return false; }
@@ skipping 44 matching lines @@
 
         bool idleNotification();
 
         double timeOrigin() const { return m_timeOrigin; }
 
         WorkerClients* clients() { return m_workerClients.get(); }
 
         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);
 
         virtual void logExceptionToConsole(const String& errorMessage, const String& sourceURL, int lineNumber, int columnNumber, PassRefPtr<ScriptCallStack>) OVERRIDE;
         void addMessageToWorkerConsole(MessageSource, MessageLevel, const String& message, const String& sourceURL, unsigned lineNumber, PassRefPtr<ScriptCallStack>, ScriptState*);
 
     private:
         virtual void refExecutionContext() OVERRIDE FINAL { ref(); }
         virtual void derefExecutionContext() OVERRIDE FINAL { deref(); }
 
         virtual const KURL& virtualURL() const OVERRIDE FINAL;
         virtual KURL virtualCompleteURL(const String&) const OVERRIDE FINAL;
 
         virtual void reportBlockedScriptExecutionToInspector(const String& directiveText) OVERRIDE FINAL;
         virtual void addMessage(MessageSource, MessageLevel, const String& message, const String& sourceURL, unsigned lineNumber, ScriptState* = 0) OVERRIDE FINAL;
 
         virtual EventTarget* errorEventTarget() OVERRIDE FINAL;
         virtual void didUpdateSecurityOrigin() OVERRIDE FINAL { }
 
         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).

 
         OwnPtr<WorkerInspectorController> m_workerInspectorController;
         bool m_closing;
 
-        HashSet<Observer*> m_workerObservers;
-
         OwnPtr<WorkerEventQueue> m_eventQueue;
 
         OwnPtr<WorkerClients> m_workerClients;
 
         double m_timeOrigin;
     };
 
 DEFINE_TYPE_CASTS(WorkerGlobalScope, ExecutionContext, context, context->isWorkerGlobalScope(), context.isWorkerGlobalScope());
 
 } // namespace WebCore
 
 #endif // WorkerGlobalScope_h