New issue
Advanced search Search tips

Issue 639153 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 640843



Sign in to add a comment

Remove WorkerThread::terminated() for cleanup

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

Issue description

We should remove WorkerThread::terminated() because it returns an internal state of WorkerThread. External classes should not depend on such the internal state.

* External classes that call it from main thread should use WorkerThreadLifecycleObserver instead.
* External classes that call it from worker thread should observe WorkerReportingProxy::willDestroyWorkerGlobalScope() or WorkerReportingProxy::workerThreadTerminated() instead.

(I just came up with this cleanup idea and don’t know how feasible this is yet.)
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 23 2016

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

commit 4c022860cc2564271b19289696b4e675e73cc4bc
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Aug 23 04:43:29 2016

Worker: Stop WorkerLoaderProxy::postTaskToWorkerGlobalScope from returning bool

No one takes care of the return value.

BUG= 639153 

Review-Url: https://codereview.chromium.org/2254303004
Cr-Commit-Position: refs/heads/master@{#413665}

[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/core/workers/WorkerLoaderProxy.cpp
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/core/workers/WorkerLoaderProxy.h
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/4c022860cc2564271b19289696b4e675e73cc4bc/third_party/WebKit/Source/web/WebSharedWorkerImpl.h

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 23 2016

Labels: M-54 OS-All
Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 23 2016

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

commit 0aeb592d32403fe36924953d37b6a0626a0f7e09
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Aug 23 08:18:37 2016

ServiceWorker: Detect forcible worker thread termination in a correct way

WorkerThread::terminated() is not a correct way to detect forcible worker thread
termination during an event dispatch because it just represents that the worker
thread is requested to terminate on the main thread, and it can return true
even if a dispatched event is successfully finished without an interruption.

BUG= 639153 ,  640078 

Review-Url: https://codereview.chromium.org/2268933002
Cr-Commit-Position: refs/heads/master@{#413692}

[modify] https://crrev.com/0aeb592d32403fe36924953d37b6a0626a0f7e09/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/0aeb592d32403fe36924953d37b6a0626a0f7e09/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/0aeb592d32403fe36924953d37b6a0626a0f7e09/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 24 2016

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

commit 70a476ad4428cd7ca9bfa0fc69fda3deff7268fb
Author: nhiroki <nhiroki@chromium.org>
Date: Wed Aug 24 10:29:50 2016

WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete

Bridge::disconnect() on a worker thread waited until Peer::disconnect() is
complete on the main thread to make sure that Peer never dereferences a pointer
to Bridge after disconnection. However, in the current implementation, the
pointer is CrossThreadWeakPersistent and its existence is appropriately checked
before access[1], so Bridge::disconnect() no longer have to wait until it's
nullified on Peer::disconnect().

[1] https://codereview.chromium.org/2123703002

BUG= 639153 ,  640088 

Review-Url: https://codereview.chromium.org/2271133002
Cr-Commit-Position: refs/heads/master@{#414040}

[modify] https://crrev.com/70a476ad4428cd7ca9bfa0fc69fda3deff7268fb/third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp
[modify] https://crrev.com/70a476ad4428cd7ca9bfa0fc69fda3deff7268fb/third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h

Blocking: 640843
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 8 2016

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

commit 2276d72486293b7b1c755a1b960da2c2503bf6bc
Author: nhiroki <nhiroki@chromium.org>
Date: Thu Sep 08 11:16:39 2016

Worker: Remove WorkerThread::terminated()

Checking WorkerThread::m_threadState is a proper way to confirm whether a worker
thread is initialized.

BUG= 639153 

Review-Url: https://codereview.chromium.org/2324763002
Cr-Commit-Position: refs/heads/master@{#417252}

[modify] https://crrev.com/2276d72486293b7b1c755a1b960da2c2503bf6bc/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/2276d72486293b7b1c755a1b960da2c2503bf6bc/third_party/WebKit/Source/core/workers/WorkerThread.h

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 8 2016

Labels: merge-merged-2854
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2276d72486293b7b1c755a1b960da2c2503bf6bc

commit 2276d72486293b7b1c755a1b960da2c2503bf6bc
Author: nhiroki <nhiroki@chromium.org>
Date: Thu Sep 08 11:16:39 2016

Worker: Remove WorkerThread::terminated()

Checking WorkerThread::m_threadState is a proper way to confirm whether a worker
thread is initialized.

BUG= 639153 

Review-Url: https://codereview.chromium.org/2324763002
Cr-Commit-Position: refs/heads/master@{#417252}

[modify] https://crrev.com/2276d72486293b7b1c755a1b960da2c2503bf6bc/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/2276d72486293b7b1c755a1b960da2c2503bf6bc/third_party/WebKit/Source/core/workers/WorkerThread.h

Sign in to add a comment