Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268933002:
ServiceWorker: Detect forcible worker thread termination in a correct way (Closed)
|
Created:
4 years, 4 months ago by nhiroki Modified:
4 years, 4 months ago Reviewers:
horo CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+worker_chromium.org, haraken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: 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
Committed: https://crrev.com/0aeb592d32403fe36924953d37b6a0626a0f7e09
Cr-Commit-Position: refs/heads/master@{#413692}
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #Patch Set 3 : revert moving getExitCode() in favor of https://codereview.chromium.org/2274453002/ #
Messages
Total messages: 27 (18 generated)
Description was changed from ========== ServiceWorker: Check forcible worker thread termination in a correct way BUG=639153 ========== to ========== ServiceWorker: Check 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 while the worker thread is still running. BUG=639153 ==========
Description was changed from ========== ServiceWorker: Check 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 while the worker thread is still running. BUG=639153 ========== to ========== ServiceWorker: Check 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 ==========
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ServiceWorker: Check 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 ========== to ========== ServiceWorker: Check whether forcible worker thread termination takes place 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 ==========
Description was changed from ========== ServiceWorker: Check whether forcible worker thread termination takes place 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 ========== to ========== ServiceWorker: Check whether forcible worker thread termination took place 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 ==========
Description was changed from ========== ServiceWorker: Check whether forcible worker thread termination took place 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
nhiroki@chromium.org changed reviewers: + horo@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2268933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2268933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerThread.cpp:342: MutexLocker lock(m_threadStateMutex); you can use getExitCode()
https://codereview.chromium.org/2268933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2268933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerThread.cpp:342: MutexLocker lock(m_threadStateMutex); On 2016/08/23 05:33:20, horo wrote: > you can use getExitCode() I'd prefer to explicitly acquire the lock at the beginning of top-level functions of WorkerThread rather than its internal so that we can more defensively avoid deadlock by recursive lock acquisitions, IMO.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2268933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2268933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerThread.cpp:342: MutexLocker lock(m_threadStateMutex); On 2016/08/23 05:48:42, nhiroki wrote: > On 2016/08/23 05:33:20, horo wrote: > > you can use getExitCode() > > I'd prefer to explicitly acquire the lock at the beginning of top-level > functions of WorkerThread rather than its internal so that we can more > defensively avoid deadlock by recursive lock acquisitions, IMO. I got it.
The CQ bit was unchecked by nhiroki@chromium.org
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/0aeb592d32403fe36924953d37b6a0626a0f7e09 Cr-Commit-Position: refs/heads/master@{#413692} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0aeb592d32403fe36924953d37b6a0626a0f7e09 Cr-Commit-Position: refs/heads/master@{#413692} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
