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..2b95a79973da95105096dbc03ec9e386e63fdad4 100644
--- a/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
+++ b/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
@@ -32,23 +32,31 @@
 
 #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/WorkerGlobalScope.h"
+#include "core/workers/WorkerThread.h"
 #include "wtf/Functional.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 namespace blink {
 
+const double kDefaultIntervalInSec = 1;
+const double kMaxIntervalInSec = 30;
+
 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 +67,19 @@ 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::startPendingActivityTimer()
 {
-    getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::reportPendingActivity, crossThreadUnretained(m_messagingProxy), hasPendingActivity));
+    // Restart the timer if it's running.
+    if (m_timer->isActive())
+        m_currentIntervalInSec = kDefaultIntervalInSec;
+
+    m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE);

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

Given that startPendingActivityTimer is called at every message event, won't
this cause a timer flood?

I'd prefer not scheduling the next timer if m_timer->isActive() returns true.
(i.e., return immediately after line 79)

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

I think we could possibly restart the timer only if the current interval length
is longer than kDefault (or some threshold).  In that way we won't be flooding &
we could check the pending activity relatively timely

(I also think it might be worth restarting the timer when the worker posts a
message to the document as it often means worker has done some pending
activity... but that can be explored separately / unrelated to existing bugs)

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

On second thought, it'd be preferable just to return if the timer is active
regardless of the current interval duration. I'm considering following cases...

Case 1) If there is a long-running activity (eg. repeated timer) and a message
event is newly dispatched, the timer needs to run until completion of the long
running activity after all, so the current interval wouldn't have to be reset on
the message event.

Case 2) If a few of short-running message events are dispatched, we could expect
that all activities are done before the current interval is too lengthened and
activities would timely be checked without the reset.

Case 3) If a bunch of short-running message events are dispatched, we could
consider them as one long-running activity and handle this case like case 1.

What do you think?
Acknowledged. I'll consider it in a separate CL/issue.

[kinuko, 2016/08/17 22:15:08]

One possible scenario I considered was a message from document could be for
canceling or changing some ongoing activity, that feels plausible.  If we
consider keeping workers possibly longer is just fine I'm ok with returning
immediately when isActive is true.  After all 30 sec is not that long.

[nhiroki, 2016/08/17 23:31:31]

Acknowledged.

+    m_currentIntervalInSec = std::min(m_currentIntervalInSec * 1.5, m_maxIntervalInSec);
 }
 
 void InProcessWorkerObjectProxy::reportException(const String& errorMessage, std::unique_ptr<SourceLocation> location, int exceptionId)
@@ -86,6 +99,18 @@ void InProcessWorkerObjectProxy::postMessageToPageInspector(const String& messag
         toDocument(context)->postInspectorTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::postMessageToPageInspector, crossThreadUnretained(m_messagingProxy), message));
 }
 
+void InProcessWorkerObjectProxy::didEvaluateWorkerScript(bool)
+{
+    startPendingActivityTimer();
+}
+
+void InProcessWorkerObjectProxy::workerGlobalScopeStarted(WorkerOrWorkletGlobalScope* globalScope)
+{
+    DCHECK(!m_workerGlobalScope);
+    m_workerGlobalScope = toWorkerGlobalScope(globalScope);
+    m_timer = wrapUnique(new Timer<InProcessWorkerObjectProxy>(this, &InProcessWorkerObjectProxy::checkPendingActivity));
+}
+
 void InProcessWorkerObjectProxy::workerGlobalScopeClosed()
 {
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::terminateWorkerGlobalScope, crossThreadUnretained(m_messagingProxy)));
@@ -97,8 +122,16 @@ void InProcessWorkerObjectProxy::workerThreadTerminated()
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::workerThreadTerminated, crossThreadUnretained(m_messagingProxy)));
 }
 
+void InProcessWorkerObjectProxy::willDestroyWorkerGlobalScope()
+{
+    m_timer.reset();
+    m_workerGlobalScope = nullptr;
+}
+
 InProcessWorkerObjectProxy::InProcessWorkerObjectProxy(InProcessWorkerMessagingProxy* messagingProxy)
     : m_messagingProxy(messagingProxy)
+    , m_currentIntervalInSec(kDefaultIntervalInSec)
+    , m_maxIntervalInSec(kMaxIntervalInSec)
 {
 }
 
@@ -108,4 +141,21 @@ ExecutionContext* InProcessWorkerObjectProxy::getExecutionContext()
     return m_messagingProxy->getExecutionContext();
 }
 
+void InProcessWorkerObjectProxy::checkPendingActivity(TimerBase*)
+{

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

Shall we add DCHECK(m_workerGlobalScope) (if you want to keep using
WeakPersistent)?

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

WONTFIX because I changed WeakPersistent to Persistent.

+    bool hasPendingActivity = V8GCController::hasPendingActivity(m_workerGlobalScope->thread()->isolate(), m_workerGlobalScope);
+    if (!hasPendingActivity) {
+        // Report all activities are done.
+        getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::pendingActivityFinished, crossThreadUnretained(m_messagingProxy)));
+
+        // Don't schedule a timer. It will be started again when a message event
+        // is dispatched.
+        m_currentIntervalInSec = kDefaultIntervalInSec;
+        return;
+    }
+
+    // There is still a pending activity. Check it later.
+    startPendingActivityTimer();
+}
+
 } // namespace blink