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

third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp

Index: third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
diff --git a/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp b/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
index 8ccb106ab96a23b1832a052f6984c7f504d5c69c..a52ec8125ec7bcb70af807386bd33062ea7e5d79 100644
--- a/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
+++ b/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
@@ -28,7 +28,6 @@
 
 #include "core/workers/InProcessWorkerMessagingProxy.h"
 
-#include "bindings/core/v8/V8GCController.h"
 #include "core/dom/CrossThreadTask.h"
 #include "core/dom/Document.h"
 #include "core/dom/SecurityContext.h"
@@ -65,7 +64,8 @@ void processMessageOnWorkerGlobalScope(PassRefPtr<SerializedScriptValue> message
     WorkerGlobalScope* globalScope = toWorkerGlobalScope(scriptContext);
     MessagePortArray* ports = MessagePort::entanglePorts(*scriptContext, std::move(channels));
     globalScope->dispatchEvent(MessageEvent::create(ports, message));
-    workerObjectProxy->confirmMessageFromWorkerObject(V8GCController::hasPendingActivity(globalScope->thread()->isolate(), scriptContext));
+    workerObjectProxy->confirmMessageFromWorkerObject();
+    workerObjectProxy->startPendingActivityTimer();
 }
 
 static int s_liveMessagingProxyCount = 0;
@@ -73,19 +73,9 @@ static int s_liveMessagingProxyCount = 0;
 } // namespace
 
 InProcessWorkerMessagingProxy::InProcessWorkerMessagingProxy(InProcessWorkerBase* workerObject, WorkerClients* workerClients)
-    : m_executionContext(workerObject->getExecutionContext())
-    , m_workerObjectProxy(InProcessWorkerObjectProxy::create(this))
-    , m_workerObject(workerObject)
-    , m_mayBeDestroyed(false)
-    , m_unconfirmedMessageCount(0)
-    , m_workerThreadHadPendingActivity(false)
-    , m_askedToTerminate(false)
-    , m_workerInspectorProxy(WorkerInspectorProxy::create())
-    , m_workerClients(workerClients)
+    : InProcessWorkerMessagingProxy(workerObject->getExecutionContext(), workerObject, workerClients)
 {
-    DCHECK(isParentContextThread());
     DCHECK(m_workerObject);
-    s_liveMessagingProxyCount++;
 }
 
 InProcessWorkerMessagingProxy::~InProcessWorkerMessagingProxy()
@@ -148,6 +138,8 @@ void InProcessWorkerMessagingProxy::postMessageToWorkerGlobalScope(PassRefPtr<Se
 
     std::unique_ptr<ExecutionContextTask> task = createCrossThreadTask(&processMessageOnWorkerGlobalScope, message, passed(std::move(channels)), crossThreadUnretained(&workerObjectProxy()));
     if (m_workerThread) {
+        // A message event is an activity and may initiate another activity.
+        m_workerGlobalScopeMayHavePendingActivity = true;
         ++m_unconfirmedMessageCount;
         m_workerThread->postTask(BLINK_FROM_HERE, std::move(task));
     } else {
@@ -207,13 +199,28 @@ void InProcessWorkerMessagingProxy::workerThreadCreated()
     m_unconfirmedMessageCount = m_queuedEarlyTasks.size();
 
     // Worker initialization means a pending activity.
-    m_workerThreadHadPendingActivity = true;
+    m_workerGlobalScopeMayHavePendingActivity = true;
 
     for (auto& earlyTasks : m_queuedEarlyTasks)
         m_workerThread->postTask(BLINK_FROM_HERE, std::move(earlyTasks));
     m_queuedEarlyTasks.clear();
 }
 
+InProcessWorkerMessagingProxy::InProcessWorkerMessagingProxy(ExecutionContext* executionContext, InProcessWorkerBase* workerObject, WorkerClients* workerClients)
+    : m_executionContext(executionContext)
+    , m_workerObjectProxy(InProcessWorkerObjectProxy::create(this))
+    , m_workerObject(workerObject)
+    , m_mayBeDestroyed(false)
+    , m_unconfirmedMessageCount(0)
+    , m_workerGlobalScopeMayHavePendingActivity(false)
+    , m_askedToTerminate(false)
+    , m_workerInspectorProxy(WorkerInspectorProxy::create())
+    , m_workerClients(workerClients)
+{
+    DCHECK(isParentContextThread());
+    s_liveMessagingProxyCount++;
+}
+
 void InProcessWorkerMessagingProxy::workerObjectDestroyed()
 {
     DCHECK(isParentContextThread());
@@ -270,26 +277,33 @@ void InProcessWorkerMessagingProxy::postMessageToPageInspector(const String& mes
         m_workerInspectorProxy->dispatchMessageFromWorker(message);
 }
 
-void InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject(bool hasPendingActivity)
+void InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject()
 {
     DCHECK(isParentContextThread());
-    if (!m_askedToTerminate) {
-        DCHECK(m_unconfirmedMessageCount);
-        --m_unconfirmedMessageCount;
-    }
-    reportPendingActivity(hasPendingActivity);
+    if (m_askedToTerminate)
+        return;
+    DCHECK(m_unconfirmedMessageCount);
+    --m_unconfirmedMessageCount;
 }
 
-void InProcessWorkerMessagingProxy::reportPendingActivity(bool hasPendingActivity)
+void InProcessWorkerMessagingProxy::pendingActivityFinished()
 {
     DCHECK(isParentContextThread());
-    m_workerThreadHadPendingActivity = hasPendingActivity;
+    DCHECK(m_workerGlobalScopeMayHavePendingActivity);
+    if (m_unconfirmedMessageCount > 0) {
+        // Ignore the report because an inflight message event may initiate a
+        // new activity.
+        return;
+    }
+    m_workerGlobalScopeMayHavePendingActivity = false;

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

I'm wondering why you need both m_workerGlobalScopeMayHavePendingActivity and
m_unconfirmedMessageCount. I guess m_unconfirmedMessageCount would be enough.

Either way, you can work on it in a follow up.

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

I'm not sure how m_unconfirmedMessageCount works without
m_workerGlobalScopeMayHavePendingActivity. In that way, when does ObjectProxy
send an ack for a posted message to MessagingProxy?

If we naively remove m_workerGlobalScopeMayHavePendingActivity from the current
CL, it would fail as follow:

  1) MessageProxy posts a message and increments the count (0->1).
  2) ObjectProxy dispatches a message event, which schedules a DOMTimer, and
sends an ack.
  3) MessageProxy receives the ack and decrements the count (1->0).
  4) The worker object is GC'ed.
  5) The DOMTimer is never fired.

If ObjectProxy sends an ack when the exponential-backoff timer is fired instead
of when the message event is dispatched, memory leak could happen:

  1) MessageProxy posts a message and increments the count (0->1).
  2) ObjectProxy dispatches a message event, which schedules a DOMTimer.
  3) MessageProxy posts another message and increments the count (1->2).
  4) ObjectProxy dispatches another message event.
  5) DOMTimer is fired. All activities are done at this point.
  6) The exponential-backoff timer is fired.
  7) ObjectProxy makes sure there are no pending activities and sends an ack.
  8) The exponential-backoff timer stops.
  9) MessageProxy receives the ack and decrements the count (2->1).
 10) MessageProxy never receives the second ack and the worker is leaked.

 }
 
 bool InProcessWorkerMessagingProxy::hasPendingActivity() const
 {
     DCHECK(isParentContextThread());
-    return (m_unconfirmedMessageCount || m_workerThreadHadPendingActivity) && !m_askedToTerminate;
+    if (m_askedToTerminate)
+        return false;
+    return m_unconfirmedMessageCount || m_workerGlobalScopeMayHavePendingActivity;
 }
 
 bool InProcessWorkerMessagingProxy::isParentContextThread() const