Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set

third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h

Index: third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
diff --git a/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h b/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
index 84705fe742dce77e1449b17726e602e3163bad4f..a2c21756ca0bdeca43d30576fec2e2238aa71adc 100644
--- a/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
+++ b/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
@@ -34,6 +34,7 @@
 #include "core/CoreExport.h"
 #include "core/dom/MessagePort.h"
 #include "core/workers/WorkerReportingProxy.h"
+#include "platform/Timer.h"
 #include "platform/heap/Handle.h"
 #include "wtf/PassRefPtr.h"
 #include <memory>
@@ -44,43 +45,62 @@ class ConsoleMessage;
 class ExecutionContext;
 class ExecutionContextTask;
 class InProcessWorkerMessagingProxy;
+class WorkerGlobalScope;
 class WorkerOrWorkletGlobalScope;
 
 // A proxy to talk to the worker object. This object is created on the
 // parent context thread (i.e. usually the main thread), passed on to
-// the worker thread, and used just to proxy messages to the
+// the worker thread, and used to proxy messages to the
 // InProcessWorkerMessagingProxy on the parent context thread.
 //
+// This also checks pending activities on WorkerGlobalScope and reports a result
+// to the message proxy when an exponential backoff timer is fired.
+//
 // Used only by in-process workers (DedicatedWorker and CompositorWorker.)
 class CORE_EXPORT InProcessWorkerObjectProxy : public WorkerReportingProxy {
     USING_FAST_MALLOC(InProcessWorkerObjectProxy);
     WTF_MAKE_NONCOPYABLE(InProcessWorkerObjectProxy);
 public:
     static std::unique_ptr<InProcessWorkerObjectProxy> create(InProcessWorkerMessagingProxy*);
-    ~InProcessWorkerObjectProxy() override { }
+    ~InProcessWorkerObjectProxy() override;
 
     void postMessageToWorkerObject(PassRefPtr<SerializedScriptValue>, std::unique_ptr<MessagePortChannelArray>);
     void postTaskToMainExecutionContext(std::unique_ptr<ExecutionContextTask>);
-    void confirmMessageFromWorkerObject(bool hasPendingActivity);
-    void reportPendingActivity(bool hasPendingActivity);
+    void confirmMessageFromWorkerObject();
+    void startPendingActivityTimer();
 
     // WorkerReportingProxy overrides.
     void reportException(const String& errorMessage, std::unique_ptr<SourceLocation>, int exceptionId) override;
     void reportConsoleMessage(MessageSource, MessageLevel, const String& message, SourceLocation*) override;
     void postMessageToPageInspector(const String&) override;
-    void didEvaluateWorkerScript(bool success) override { }
-    void workerGlobalScopeStarted(WorkerOrWorkletGlobalScope*) override { }
+    void didEvaluateWorkerScript(bool success) override;
+    void workerGlobalScopeStarted(WorkerOrWorkletGlobalScope*) override;
     void workerGlobalScopeClosed() override;
     void workerThreadTerminated() override;
-    void willDestroyWorkerGlobalScope() override { }
+    void willDestroyWorkerGlobalScope() override;
 
 protected:
     InProcessWorkerObjectProxy(InProcessWorkerMessagingProxy*);
     virtual ExecutionContext* getExecutionContext();
 
 private:
+    friend class InProcessWorkerMessagingProxyForTest;
+
+    void checkPendingActivity(TimerBase*);
+
     // This object always outlives this proxy.
     InProcessWorkerMessagingProxy* m_messagingProxy;
+
+    // Used for scheduling a timer to check pending activities on the worker
+    // global scope. The interval of the timer is initially set to

[kinuko, 2016/08/16 21:37:38]

nit: the current interval duration is m_currentIntervalInSec and it's initially
set to kDefaultIntervalInSec ?

[nhiroki, 2016/08/17 04:45:33]

Done.

+    // |m_currentIntervalInSec| and exponentially increased up to
+    // |m_maxIntervalInSec| until all activities are done. The task is cancelled
+    // when the worker global scoped is destroyed.

[kinuko, 2016/08/16 21:37:38]

maybe comment that m_maxIntervalInSec is usually kMaxIntervalInSec but made as a
member variable for testing

[nhiroki, 2016/08/17 04:45:32]

Done.

+    std::unique_ptr<Timer<InProcessWorkerObjectProxy>> m_timer;
+    double m_currentIntervalInSec;
+    double m_maxIntervalInSec;
+
+    WeakPersistent<WorkerGlobalScope> m_workerGlobalScope;

[haraken, 2016/08/16 12:17:40]

I'm wondering why you can just use Persistent<WorkerGlobalScope>. I don't think
the strong Persistent will cause memory leak because
InProcessWorkerObjectProxy::willDestroyWorkerGlobalScope explicitly clears out
m_workerGlobalScope.

[nhiroki, 2016/08/17 04:45:32]

<TL;DR> Changed WeakPersistent to Persistent.

<Long braindump-ish reply>

I'm a bit unsure the usage policy of strong/weak Persistent. As you commented,
there is no risk of memory leak by strong Persistent, but, on the other hand,
using WeakPersistent seems to more clarify the ownership/lifecycle of the
object. Is there any guide about it?

... ah, I just noticed that BlinkGCAPIReference.md mentions it:

"The use of `WeakMember<T>` incurs some overhead in garbage collector's
performance. Use it sparingly. Usually, weak members are not necessary at all,
because reference cycles with members are allowed. More specifically,
`WeakMember<T>` should be used only if the owner of a weak member can outlive
the pointed object. Otherwise, `Member<T>` should be used."

OK, using strong Persistent for avoiding overhead sounds reasonable. I changed
WeakPersistent to Persistent.

 };
 
 } // namespace blink