Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011763002:
Worker: Attempt to gracefully terminate WorkerThread as much as possible (Closed)
|
Created:
4 years, 7 months ago by nhiroki Modified:
4 years, 6 months ago CC:
blink-reviews, blink-worker-reviews_chromium.org, chromium-reviews, falken, horo+watch_chromium.org, kinuko+worker_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@reorder_functions Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Attempt to gracefully terminate WorkerThread as much as possible
Before this CL, WorkerThread::terminate() called on the main thread may forcibly
terminate V8 Isolate in order to ensure that the worker thread is not blocked by
JS execution and a shutdown task starts soon. This is risky because, after the
forced termination, V8 APIs suddenly start to return empty handles and it may
cause crashes if there are no treatments for the case.
After this CL, terminate() avoids the force termination as much as possible.
Instead, it posts a delayed task to terminate the isolate in case that the
shutdown sequence does not start on the worker thread in a certain time period.
BUG=487050
Committed: https://crrev.com/1ad17d042c96e4ca924fbd11e88813da8d5241d6
Cr-Commit-Position: refs/heads/master@{#397343}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : address review comments and fix failing tests #Patch Set 3 : fix dead lock when terminateAndWait() is called before start() or after terminate() and add more te… #Patch Set 4 : add more test expectations and clean up #
Total comments: 4
Patch Set 5 : address review comments #
Total comments: 15
Patch Set 6 : address review comments #Patch Set 7 : fix test expectations #
Total comments: 2
Patch Set 8 : remove termination-before-initialization cases #
Total comments: 2
Patch Set 9 : EXPECT_EQ #
Messages
Total messages: 40 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Description was changed from ========== WIP: Delayed Task BUG= ========== to ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in some cases. This is risky because, after the forced termination, V8 APIs may suddenly start to return empty handles and it may cause crashes if these are no treatments for the case. After this CL, terminate() stop the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ==========
Description was changed from ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in some cases. This is risky because, after the forced termination, V8 APIs may suddenly start to return empty handles and it may cause crashes if these are no treatments for the case. After this CL, terminate() stop the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ========== to ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ==========
Patchset #1 (id:200001) has been deleted
Description was changed from ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ========== to ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ==========
nhiroki@chromium.org changed reviewers: + kinuko@chromium.org, yhirano@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:62: class WorkerThread::ForceTerminationTask { +final https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:73: m_cancellableTaskFactory->cancel(); Is this needed? Doesn't m_cancellableTaskFactory cancel tasks when destructed? https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:100: // (https://crbug.com/413518) Is this comment still needed? https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:439: m_scheduledForceTerminationTask = ForceTerminationTask::create(this); I'm not sure if we should avoid scheduling the task on compositor workers. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:146: Force, [optional] Is "Forcible" better? https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:165: void setForceTerminationDelayInMs(long long forceTerminationDelayInMs) { m_forceTerminationDelayInMs = forceTerminationDelayInMs; } Please add "ForTest[ing]" to its name. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:140: TEST_F(WorkerThreadTest, StartAndStopOnScriptLoaded_AsyncForciblyTerminate) Can you add a test case for the case that a thread is terminated forcibly while waiting for ForceTerminationTask to run?
Patchset #2 (id:240001) has been deleted
Thank you for reviewing! Updated. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:62: class WorkerThread::ForceTerminationTask { On 2016/05/27 11:04:43, yhirano wrote: > +final Done. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:73: m_cancellableTaskFactory->cancel(); On 2016/05/27 11:04:43, yhirano wrote: > Is this needed? Doesn't m_cancellableTaskFactory cancel tasks when destructed? Ah, it's not necessary. Removed. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:100: // (https://crbug.com/413518) On 2016/05/27 11:04:43, yhirano wrote: > Is this comment still needed? Removed. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:439: m_scheduledForceTerminationTask = ForceTerminationTask::create(this); On 2016/05/27 11:04:43, yhirano wrote: > I'm not sure if we should avoid scheduling the task on compositor workers. If we should avoid scheduling it on CW, we could add |virtual bool WorkerThread::needsGracefulTermination()|, make CompositorWorkerThread return false and check it here. If the design of CW lifecycle hasn't clearly been determined yet, I'd like to keep this CL as is for now and add the check function later if necessary. flackr@, any thoughts? https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:146: Force, On 2016/05/27 11:04:43, yhirano wrote: > [optional] Is "Forcible" better? Done. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:165: void setForceTerminationDelayInMs(long long forceTerminationDelayInMs) { m_forceTerminationDelayInMs = forceTerminationDelayInMs; } On 2016/05/27 11:04:43, yhirano wrote: > Please add "ForTest[ing]" to its name. Done. https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:140: TEST_F(WorkerThreadTest, StartAndStopOnScriptLoaded_AsyncForciblyTerminate) On 2016/05/27 11:04:43, yhirano wrote: > Can you add a test case for the case that a thread is terminated forcibly while > waiting for ForceTerminationTask to run? Good point. The patchset 1 couldn't handle this case as follows: 1) terminateInternal() called from terminateAndWait() immediately returns because |m_terminated| was already set by the previous terminate() call. 2) terminateAndWait() blocks the main thread and it prevents the scheduled task from running. 3) DEAD LOCK. The patchset 2 made terminateAndWait() overtake the scheduled termination task.
nhiroki@chromium.org changed reviewers: + flackr@chromium.org
+flackr, could you take a look at https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... ? Thanks.
https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:439: m_scheduledForceTerminationTask = ForceTerminationTask::create(this); On 2016/05/30 at 06:18:47, nhiroki wrote: > On 2016/05/27 11:04:43, yhirano wrote: > > I'm not sure if we should avoid scheduling the task on compositor workers. > > If we should avoid scheduling it on CW, we could add |virtual bool WorkerThread::needsGracefulTermination()|, make CompositorWorkerThread return false and check it here. > > If the design of CW lifecycle hasn't clearly been determined yet, I'd like to keep this CL as is for now and add the check function later if necessary. > > flackr@, any thoughts? It seems like compositor worker currently isn't ever going to trigger this because we manually attach without a worker thread on the BackingThreadHolder and manually detach last (i.e. shouldScheduleToTerminateIsolate will be false because workerScriptCount > 1) and we manually call TerminateExecution. That being said, if we did change this, this scheduled forced termination task would never get a chance to run on compositor worker since at shutdown it waits on the termination of the backing thread. All in all, I think it's fine to keep as is for now from the perspective of compositor worker.
https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:439: m_scheduledForceTerminationTask = ForceTerminationTask::create(this); On 2016/05/30 14:41:38, flackr wrote: > On 2016/05/30 at 06:18:47, nhiroki wrote: > > On 2016/05/27 11:04:43, yhirano wrote: > > > I'm not sure if we should avoid scheduling the task on compositor workers. > > > > If we should avoid scheduling it on CW, we could add |virtual bool > WorkerThread::needsGracefulTermination()|, make CompositorWorkerThread return > false and check it here. > > > > If the design of CW lifecycle hasn't clearly been determined yet, I'd like to > keep this CL as is for now and add the check function later if necessary. > > > > flackr@, any thoughts? > > It seems like compositor worker currently isn't ever going to trigger this > because we manually attach without a worker thread on the BackingThreadHolder > and manually detach last (i.e. shouldScheduleToTerminateIsolate will be false > because workerScriptCount > 1) and we manually call TerminateExecution. That > being said, if we did change this, this scheduled forced termination task would > never get a chance to run on compositor worker since at shutdown it waits on the > termination of the backing thread. > > All in all, I think it's fine to keep as is for now from the perspective of > compositor worker. I understand the current CW behavior. Thank you for checking this.
lgtm https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:93: m_workerThread->m_workerGlobalScope->scriptController()->willScheduleExecutionTermination(); [optional] It might be good to make a function for these two statements because they are called together. https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:155: m_workerThread->terminateAndWaitForAllWorkers(); [optional] I prefer calling static member functions explicitly (in this case, WorkerThread::terminateAndWaitForAllWorkers)
Patchset #5 (id:320001) has been deleted
Thank you! Updated. https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:93: m_workerThread->m_workerGlobalScope->scriptController()->willScheduleExecutionTermination(); On 2016/05/31 04:48:07, yhirano wrote: > [optional] It might be good to make a function for these two statements because > they are called together. Done. https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2011763002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:155: m_workerThread->terminateAndWaitForAllWorkers(); On 2016/05/31 04:48:07, yhirano wrote: > [optional] I prefer calling static member functions explicitly (in this case, > WorkerThread::terminateAndWaitForAllWorkers) Done.
https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:56: const long long kForceTerminationDelayInMs = 3000; // 3 secs nit: we used to have 2 secs timer to forcibly kill worker process, we might be able to start with that https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:414: // completed on initializeOnWorkerThread(). The comment looks stale now? (We're no longer waiting in this case) https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:414: // completed on initializeOnWorkerThread(). Hmm, if we're not calling start() initializeOnWorkerThread isn't scheduled so it looks it's fine not to wait, but if start() is called but initializeOnWorkerThread has not yet run it'll eventually run and try to signal m_shutdownEvent->signal? https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:449: DCHECK_EQ(TerminationMode::Graceful, mode); nit: DCHECK(!m_scheduledForceTreminationTask) ? https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:125: ExitCode exitCode(); nit: getExitCode() c.f. "Please use names that differ by more than case" (https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/%22Please... ) https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:144: // Synchronously terminate the isolate. Please be careful to use this nit: "terminate the isolate" -> "terminate the v8 isolate" or maybe just "terminate the worker execution" (here and below) https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:159: void forciblyTerminateIsolate(); nit: forciblyTerminateV8Isolate or forciblyTerminateExecution https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:159: void forciblyTerminateIsolate(); nit: why are having these methods between initializeOnWorkerThread() and prepareForShutdownOnWorkerThread()? It feels a bit misplaced?
Thanks for your comments. Updated. https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:56: const long long kForceTerminationDelayInMs = 3000; // 3 secs On 2016/05/31 10:13:26, kinuko wrote: > nit: we used to have 2 secs timer to forcibly kill worker process, we might be > able to start with that Done. https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:414: // completed on initializeOnWorkerThread(). On 2016/05/31 10:13:26, kinuko wrote: > Hmm, if we're not calling start() initializeOnWorkerThread isn't scheduled so it > looks it's fine not to wait, but if start() is called but > initializeOnWorkerThread has not yet run it'll eventually run and try to signal > m_shutdownEvent->signal? Good point. I'm now thinking that the main thread should wait for the shutdown signal (WorkerReportingProxy::workerThreadTerminated call) even if WorkerThread hasn't actually started yet so that each worker class can have a chance to delete a WorkerThread instance via workerThreadTerminated() call (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) The patchset6+ ensures that workerThreadTerminate() is called on any termination paths. WDYT? https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:449: DCHECK_EQ(TerminationMode::Graceful, mode); On 2016/05/31 10:13:26, kinuko wrote: > nit: DCHECK(!m_scheduledForceTreminationTask) ? Done. https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:125: ExitCode exitCode(); On 2016/05/31 10:13:26, kinuko wrote: > nit: getExitCode() > > c.f. "Please use names that differ by more than case" > (https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/%22Please... > ) Thank you for sharing the pointer. I missed the discussion. Done. https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:144: // Synchronously terminate the isolate. Please be careful to use this On 2016/05/31 10:13:26, kinuko wrote: > nit: "terminate the isolate" -> "terminate the v8 isolate" or maybe just > "terminate the worker execution" (here and below) Replaced all. https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:159: void forciblyTerminateIsolate(); On 2016/05/31 10:13:26, kinuko wrote: > nit: forciblyTerminateV8Isolate or forciblyTerminateExecution Done. https://codereview.chromium.org/2011763002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:159: void forciblyTerminateIsolate(); On 2016/05/31 10:13:26, kinuko wrote: > nit: why are having these methods between initializeOnWorkerThread() and > prepareForShutdownOnWorkerThread()? It feels a bit misplaced? In common cases, these are called after initializeOnWorkerThread and before prepareForShutdownOnWorkerThread. That's the reason why I placed them here. However, as you commented, these can be called on the main thread and placing them among *OnWorkerThread functions looks a bit strange, so I moved them to before this *OnWorkerThread block.
https://codereview.chromium.org/2011763002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:346: if (!m_started) I wonder if we should just DCHECK(m_started) in terminate, it doesn't look calling terminate() without calling start() should happen in normal sequence?
Patchset #8 (id:400001) has been deleted
Thank you! Updated. https://codereview.chromium.org/2011763002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2011763002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:346: if (!m_started) On 2016/06/02 02:31:28, kinuko wrote: > I wonder if we should just DCHECK(m_started) in terminate, it doesn't look > calling terminate() without calling start() should happen in normal sequence? It looks like the worker thread is always supposed to start immediately after creation for any worker types, so just having DCHECK would be sufficient. Updated.
lgtm https://codereview.chromium.org/2011763002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2011763002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:128: EXPECT_TRUE(WorkerThread::ExitCode::GracefullyTerminated == exitCode); nit: EXPECT_EQ
https://codereview.chromium.org/2011763002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2011763002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:128: EXPECT_TRUE(WorkerThread::ExitCode::GracefullyTerminated == exitCode); On 2016/06/02 07:25:08, kinuko wrote: > nit: EXPECT_EQ Done.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2011763002/#ps440001 (title: "EXPECT_EQ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011763002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011763002/440001
Message was sent while issue was closed.
Description was changed from ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ========== to ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 ========== to ========== Worker: Attempt to gracefully terminate WorkerThread as much as possible Before this CL, WorkerThread::terminate() called on the main thread may forcibly terminate V8 Isolate in order to ensure that the worker thread is not blocked by JS execution and a shutdown task starts soon. This is risky because, after the forced termination, V8 APIs suddenly start to return empty handles and it may cause crashes if there are no treatments for the case. After this CL, terminate() avoids the force termination as much as possible. Instead, it posts a delayed task to terminate the isolate in case that the shutdown sequence does not start on the worker thread in a certain time period. BUG=487050 Committed: https://crrev.com/1ad17d042c96e4ca924fbd11e88813da8d5241d6 Cr-Commit-Position: refs/heads/master@{#397343} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1ad17d042c96e4ca924fbd11e88813da8d5241d6 Cr-Commit-Position: refs/heads/master@{#397343} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
