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

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 14 matching lines @@
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "core/workers/InProcessWorkerObjectProxy.h"
 
 #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))));
 }
 
 void InProcessWorkerObjectProxy::postTaskToMainExecutionContext(std::unique_ptr<ExecutionContextTask> task)
 {
     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)
 {
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::reportException, crossThreadUnretained(m_messagingProxy), errorMessage, passed(location->clone()), exceptionId));
 }
 
 void InProcessWorkerObjectProxy::reportConsoleMessage(MessageSource source, MessageLevel level, const String& message, SourceLocation* location)
 {
     getExecutionContext()->postTask(BLINK_FROM_HERE, createCrossThreadTask(&InProcessWorkerMessagingProxy::reportConsoleMessage, crossThreadUnretained(m_messagingProxy), source, level, message, passed(location->clone())));
 }
 
 void InProcessWorkerObjectProxy::postMessageToPageInspector(const String& message)
 {
     ExecutionContext* context = getExecutionContext();
     if (context->isDocument())
         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)));
 }
 
 void InProcessWorkerObjectProxy::workerThreadTerminated()
 {
     // This will terminate the MessagingProxy.
     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)
 {
 }
 
 ExecutionContext* InProcessWorkerObjectProxy::getExecutionContext()
 {
     DCHECK(m_messagingProxy);
     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