New issue
Advanced search Search tips

Issue 641846 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 639244



Sign in to add a comment

Remove WorkerThread::terminateAndWait()

Project Member Reported by nhiroki@chromium.org, Aug 29 2016

Issue description

Currently, in production code, WorkerThread::terminateAndWait() is called only from WebEmbeddedWorkerImpl::~WebEmbeddedWorkerImpl(). It'd be nice if we can remove it or make it test-only function because it sometime causes crash bugs (see  issue 487050  for details)
 
Components: Blink>ServiceWorker
Blockedon: 639244
WIP CL is here: https://codereview.chromium.org/2284303002/

This seems to be blocked by issue 639244 that abandons graceful renderer process shutdown (see also the CL description)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/15e3d6427580e9418b98aa765a4a2600acec5807

commit 15e3d6427580e9418b98aa765a4a2600acec5807
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Jun 30 12:31:15 2017

ServiceWorker: Remove WorkerThread::TerminateAndWait() call from WebEmbeddedWorkerImpl

This CL removes WorkerThread::TerminateAndWait() call from the dtor of
WebEmbeddedWorkerImpl. WorkerThread should already be terminated or should not
be started yet at the point as follows:

Note that WebEmbeddedWorkerImpl::TerminateWorkerContext() is the entry point to
terminate WorkerThread and must always be called before the dtor.

Case 1) TerminateWorkerContext() is called while WorkerThread is running.
WebEmbeddedWorkerImpl::TerminateWorkerContext
  WorkerThread::Terminate
  (thread temination runs)
    WorkerThread::PerformShutdownOnWorkerThread
      ServiceWorkerGlobalScopeProxy::DidTerminateWorkerThread
        ServiceWorkerContextClient::WorkerContextDestroyed
          EmbeddedWorkerInstanceClientImpl::WorkerContextDestroyed
            EmbeddedWorkerInstanceClientImpl::WorkerWrapper::~WorkerWrapper
              -> WebEmbeddedWorkerImpl is destroyed here.

Case 2) TerminateWorkerContext() is called before WorkerThread starts.
WebEmbeddedWorkerImpl::TerminateWorkerContext
  ServiceWorkerContextClient::WorkerContextFailedToStart
    EmbeddedWorkerInstanceClientImpl::WorkerContextDestroyed
      EmbeddedWorkerInstanceClientImpl::WorkerWrapper::~WorkerWrapper
        -> WebEmbeddedWorkerImpl is destroyed here.

Bug:  641846 
Change-Id: I1df721bff395449f1da20c247e1e24063c4dc836
Reviewed-on: https://chromium-review.googlesource.com/554442
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483684}
[modify] https://crrev.com/15e3d6427580e9418b98aa765a4a2600acec5807/third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/15e3d6427580e9418b98aa765a4a2600acec5807/third_party/WebKit/Source/modules/serviceworkers/WebEmbeddedWorkerImplTest.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/426f46f231759c646bf2357be299f87d2c966260

commit 426f46f231759c646bf2357be299f87d2c966260
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Jul 03 03:09:58 2017

Worker: Remove WorkerThread::TerminateAndWait()

WorkerThread::TerminateAndWait() is used only in unit tests and can be replaced
with Terminate() and WaitForShutdownForTesting().

Bug:  641846 
Change-Id: I763256be20d1aa76c0c7d9448e2b854c7ce9602c
Reviewed-on: https://chromium-review.googlesource.com/558324
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483928}
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/core/workers/ThreadedWorkletTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScopeTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThreadTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp
[modify] https://crrev.com/426f46f231759c646bf2357be299f87d2c966260/third_party/WebKit/Source/modules/webaudio/AudioWorkletThreadTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2dcd677d1b401a3e9b4d1af21410764ba971b7cb

commit 2dcd677d1b401a3e9b4d1af21410764ba971b7cb
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Jul 03 03:55:18 2017

Worker: Avoid duplicate thread state checks

This is a cleanup and should not change existing behavior.

Bug:  641846 
Change-Id: Ieeae827f6ea506f939236c894ef3ecd57e812a06
Reviewed-on: https://chromium-review.googlesource.com/558844
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483934}
[modify] https://crrev.com/2dcd677d1b401a3e9b4d1af21410764ba971b7cb/third_party/WebKit/Source/core/workers/WorkerThread.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d76ff5f3e0dd3177c72c5e848ff40349375c41e

commit 4d76ff5f3e0dd3177c72c5e848ff40349375c41e
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Jul 05 01:36:55 2017

Worker: Separate synchronous worker termination from production code

Synchronous termination is now used only for the leak detector via
TerminateAndWaitForAllWorkers(), that is, production code no longer depends on
it. To simplify the production code, this CL pushes the code into
TerminateAndWaitForAllWorkers(), and renames it to
TerminateAllWorkersForTesting().

Bug:  641846 
Change-Id: Ia2c5372394cf0ddfdf038c8ceb81a6655d7c564f
Reviewed-on: https://chromium-review.googlesource.com/558596
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484173}
[modify] https://crrev.com/4d76ff5f3e0dd3177c72c5e848ff40349375c41e/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/4d76ff5f3e0dd3177c72c5e848ff40349375c41e/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/4d76ff5f3e0dd3177c72c5e848ff40349375c41e/third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp
[modify] https://crrev.com/4d76ff5f3e0dd3177c72c5e848ff40349375c41e/third_party/WebKit/Source/modules/exported/WebLeakDetector.cpp

Labels: M-61
Status: Fixed (was: Assigned)

Sign in to add a comment