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

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

Index: third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
diff --git a/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp b/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
index 67a8b59d6fedf0b55b9fd8a38253956a7fcace59..178b270cd7e6167909997d8ef07535468c72b043 100644
--- a/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
+++ b/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
@@ -32,23 +32,32 @@
 
 #include "bindings/core/v8/SerializedScriptValue.h"
 #include "bindings/core/v8/SourceLocation.h"
+#include "bindings/core/v8/V8GCController.h"
 #include "core/dom/CrossThreadTask.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/workers/InProcessWorkerMessagingProxy.h"
+#include "core/workers/WorkerBackingThread.h"
+#include "core/workers/WorkerGlobalScope.h"
+#include "platform/WebThreadSupportingGC.h"
 #include "wtf/Functional.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 namespace blink {
 
+const long long kDefaultDelayToCheckPendingActivityInMs = 1; // 1 sec
+const long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs

[haraken, 2016/08/05 08:20:58]

If we have the exponential-backoff timer, maybe can we entirely remove the
hasPendingActivity check from the postMessage? It looks wasteful to check
hasPendingActivity() at every postMessage -- the timer mechanism looks much
simpler and more lightweight.

[nhiroki, 2016/08/08 09:19:14]

I think it couldn't work in some cases. For example, when a message event
initiates some activity, the activity may not be reported to
InProcessWorkerMessagingProxy until the next timer is fired. As a result, the
worker object can be collected, the pending activity still exists though.

Instead, we could reduce the number of the check at postMessage. If the
exponential-backoff timer is running when a message arrives, we can skip the
check and defer to the timer. The patchset 4 adopts this way (see
InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()).

WDYT?

[haraken, 2016/08/08 11:00:55]

Are you worrying about the following case?

1) The timer reports that # of pending activities is 0.
2) A message event is dispatched. It initiates some pending activity.
3) The main thread runs a GC and collects the worker object.

But what happens if the main-thread GC runs between 1) and 2)? Won't the worker
object get collected with your CL as well?

[nhiroki, 2016/08/08 11:35:33]

Yes, that's right.
Regardless of this CL, the main-thread GC does not collect the worker between 1)
and 2) because InProcessWorkerMessagingProxy counts inflight messages and
prevents GC until the count reaches zero (see m_unconfirmedMessageCount on
InProcessWorkerMessagingProxy).

+
 std::unique_ptr<InProcessWorkerObjectProxy> InProcessWorkerObjectProxy::create(InProcessWorkerMessagingProxy* messagingProxy)
 {
     DCHECK(messagingProxy);
     return wrapUnique(new InProcessWorkerObjectProxy(messagingProxy));
 }
 
+InProcessWorkerObjectProxy::~InProcessWorkerObjectProxy() {}
+
 void InProcessWorkerObjectProxy::postMessageToWorkerObject(PassRefPtr<SerializedScriptValue> message, std::unique_ptr<MessagePortChannelArray> channels)
 {
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::postMessageToWorkerObject, crossThreadUnretained(m_messagingProxy), message, passed(std::move(channels))));
@@ -59,14 +68,31 @@ void InProcessWorkerObjectProxy::postTaskToMainExecutionContext(std::unique_ptr<
     getExecutionContext()->postTask(BLINK_FROM_HERE, std::move(task));
 }
 
-void InProcessWorkerObjectProxy::confirmMessageFromWorkerObject(bool hasPendingActivity)
+void InProcessWorkerObjectProxy::confirmMessageFromWorkerObject()
 {
-    getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject, crossThreadUnretained(m_messagingProxy), hasPendingActivity));
+    getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject, crossThreadUnretained(m_messagingProxy)));
 }
 
-void InProcessWorkerObjectProxy::reportPendingActivity(bool hasPendingActivity)
+void InProcessWorkerObjectProxy::reportPendingActivity(WorkerGlobalScope* workerGlobalScope)

[kinuko, 2016/08/05 14:55:29]

This should be probably renamed to checkPendingActivity

[nhiroki, 2016/08/08 09:19:14]

Done.

 {
+    bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope);
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::reportPendingActivity, crossThreadUnretained(m_messagingProxy), hasPendingActivity));
+
+    if (!hasPendingActivity) {
+        m_taskCanceller.cancel();
+        m_currentDelayToCheckPendingActivityInMs = kDefaultDelayToCheckPendingActivityInMs;
+        return;
+    }
+
+    // Post a delayed task to check whether there is a pending activity.
+    std::unique_ptr<WTF::Closure> task = WTF::bind(&InProcessWorkerObjectProxy::reportPendingActivity, unretained(this), wrapWeakPersistent(workerGlobalScope));
+    auto result = makeCancellable(std::move(task));
+    m_taskCanceller = std::move(result.canceller);
+    workerGlobalScope->thread()->workerBackingThread().backingThread().postDelayedTask(BLINK_FROM_HERE, std::move(result.function), m_currentDelayToCheckPendingActivityInMs);
+
+    // Update the delay.
+    long long newDelayInMs = m_currentDelayToCheckPendingActivityInMs * 1.5;
+    m_currentDelayToCheckPendingActivityInMs = std::min(newDelayInMs, m_maxDelayToCheckPendingActivityInMs);

[kinuko, 2016/08/05 14:55:29]

Hmm.. it feels what the class-level comment in the header says about this
class's role and what we do here are a bit different.

[nhiroki, 2016/08/08 09:19:14]

Slightly updated the header comment.

Checking pending activities seems to deviate from responsibilities of
InProcessWorkerObject**Proxy**. We could split this feature into a separate
class or rename InProcessWorkerObjectProxy to something later.

 }
 
 void InProcessWorkerObjectProxy::reportException(const String& errorMessage, std::unique_ptr<SourceLocation> location, int exceptionId)
@@ -97,8 +123,16 @@ void InProcessWorkerObjectProxy::workerThreadTerminated()
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::workerThreadTerminated, crossThreadUnretained(m_messagingProxy)));
 }
 
+void InProcessWorkerObjectProxy::willDestroyWorkerGlobalScope()
+{
+    if (m_taskCanceller.isActive())
+        m_taskCanceller.cancel();

[tzik, 2016/08/05 05:16:06]

nit: cancel() is noop if isActive() returns false. You can call it
unconditionally.

[nhiroki, 2016/08/08 09:19:14]

Done.

+}
+
 InProcessWorkerObjectProxy::InProcessWorkerObjectProxy(InProcessWorkerMessagingProxy* messagingProxy)
     : m_messagingProxy(messagingProxy)
+    , m_currentDelayToCheckPendingActivityInMs(kDefaultDelayToCheckPendingActivityInMs)
+    , m_maxDelayToCheckPendingActivityInMs(kMaxDelayToCheckPendingActivityInMs)
 {
 }