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 4016c1c4c505534b26ff5e82b7d953bb00264cf8..f1c505c706a122833f0ceef4af1ba1ddf1dcc0c3 100644
--- a/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
+++ b/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
@@ -32,12 +32,15 @@
 
 #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/ParentFrameTaskRunners.h"
+#include "core/workers/WorkerGlobalScope.h"
+#include "core/workers/WorkerThread.h"
 #include "platform/CrossThreadFunctional.h"
 #include "public/platform/WebTaskRunner.h"
 #include "wtf/Functional.h"
@@ -46,12 +49,17 @@
 
 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)
 {
     getParentFrameTaskRunners()->get(TaskType::PostedMessage)->postTask(BLINK_FROM_HERE, crossThreadBind(&InProcessWorkerMessagingProxy::postMessageToWorkerObject, crossThreadUnretained(m_messagingProxy), message, passed(std::move(channels))));
@@ -64,14 +72,26 @@ void InProcessWorkerObjectProxy::postTaskToMainExecutionContext(std::unique_ptr<
     getExecutionContext()->postTask(BLINK_FROM_HERE, std::move(task));
 }
 
-void InProcessWorkerObjectProxy::confirmMessageFromWorkerObject(bool hasPendingActivity)
+void InProcessWorkerObjectProxy::confirmMessageFromWorkerObject()
 {
-    getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE, crossThreadBind(&InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject, crossThreadUnretained(m_messagingProxy), hasPendingActivity));
+    getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE, crossThreadBind(&InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject, crossThreadUnretained(m_messagingProxy)));
 }
 
-void InProcessWorkerObjectProxy::reportPendingActivity(bool hasPendingActivity)
+void InProcessWorkerObjectProxy::startPendingActivityTimer()
 {
-    getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE, crossThreadBind(&InProcessWorkerMessagingProxy::reportPendingActivity, crossThreadUnretained(m_messagingProxy), hasPendingActivity));
+    if (m_timer->isActive()) {
+        // Avoid a timer flood, for example, caused by a burst of posted
+        // messages.
+        if (m_currentIntervalInSec == kDefaultIntervalInSec)

[haraken, 2016/08/17 06:30:47]

Does this branch really work? You're updating m_currentIntervalInSec to
m_currentIntervalInSec*1.5 at line 94. Hence, this branch wouldn't be helpful to
avoid the timer flood.

Given the overhead of creating cancellable tasks (which we're discussing in
another thread), I want to take a conservative approach here; i.e., if it's not
easy to avoid the timer flood, just return immediately when m_timer->isActive
returns true. It may cause a 30 sec of delay in the worst case, but it would be
better than causing the timer flood.

Maybe we could introduce another timer and do something like this:

void startPendingActivityTimer() {
  if (!m_anotherTimer.isActive())
    m_anotherTimer.startOneShot(1000);
}

void anotherTimerFired() {
  m_currentIntervalInSec = kDefaultIntervalInSec;
  m_timer->startOneShot(m_currentIntervalInSec);
  m_currentIntervalInSec = std::min(...);
}

[kinuko, 2016/08/17 22:50:10]

m_currentIntervalInSec is actually a next interval sec so it won't work.  We
could expose something similar to base::Timer::GetCurrentDelay() in Timer.h
(having that tends to be useful anyways) and check it to make this pattern
saner, but as I wrote I'm fine just returning immediately too.

(Separately-- reseting an ongoing timer feels relatively a common pattern, if
it's costly it's probably worth commenting that in Timer.h until we fix it)

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

Acknowledged.

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

Ooops... yes, this doesn't work.
Agreed. Changed this function to immediately return if isActive() returns true.

+            return;
+
+        // Restart the timer with the default interval duration to check newly
+        // initiated pending activities timely.
+        m_currentIntervalInSec = kDefaultIntervalInSec;
+    }
+
+    m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE);
+    m_currentIntervalInSec = std::min(m_currentIntervalInSec * 1.5, m_maxIntervalInSec);
 }
 
 void InProcessWorkerObjectProxy::reportException(const String& errorMessage, std::unique_ptr<SourceLocation> location, int exceptionId)
@@ -94,6 +114,18 @@ void InProcessWorkerObjectProxy::postMessageToPageInspector(const String& messag
     }
 }
 
+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()
 {
     getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE, crossThreadBind(&InProcessWorkerMessagingProxy::terminateWorkerGlobalScope, crossThreadUnretained(m_messagingProxy)));
@@ -105,8 +137,16 @@ void InProcessWorkerObjectProxy::workerThreadTerminated()
     getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE, crossThreadBind(&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)
 {
 }
 
@@ -122,4 +162,21 @@ ExecutionContext* InProcessWorkerObjectProxy::getExecutionContext()
     return m_messagingProxy->getExecutionContext();
 }
 
+void InProcessWorkerObjectProxy::checkPendingActivity(TimerBase*)
+{
+    bool hasPendingActivity = V8GCController::hasPendingActivity(m_workerGlobalScope->thread()->isolate(), m_workerGlobalScope);
+    if (!hasPendingActivity) {
+        // Report all activities are done.
+        getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE, crossThreadBind(&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