WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecu...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 May 2017 20:34:13 +0000 (20:34 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 May 2017 20:34:13 +0000 (20:34 +0000)
commit4a52d805e6304f9deb7c1be5c3143b1d54636b50
tree9933738d1b632ff7aa7100581e02e3871b413e43
parent784d31e088abe3d1bbad8f8c5aa16640c00f487e
WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
https://bugs.webkit.org/show_bug.cgi?id=171775
<rdar://problem/30975761>

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

Increased the number of frames captured in VM::nativeStackTraceOfLastThrow()
from 25 to 100.  From experience, I found that 25 is sometimes not sufficient
for our debugging needs.

Also added VM::throwingThread() to track which thread an exception was thrown in.
This may be useful if the client is entering the VM from different threads.

* runtime/ExceptionScope.cpp:
(JSC::ExceptionScope::unexpectedExceptionMessage):
* runtime/ExceptionScope.h:
(JSC::ExceptionScope::exception):
(JSC::ExceptionScope::unexpectedExceptionMessage):
* runtime/Options.h:
- Added the unexpectedExceptionStackTraceLimit option.
* runtime/VM.cpp:
(JSC::VM::throwException):
* runtime/VM.h:
(JSC::VM::throwingThread):
(JSC::VM::clearException):

Source/WebCore:

Currently, WorkerThread::stop() calls scheduleExecutionTermination() to terminate
JS execution first, followed by posting a cleanup task to the worker, and lastly,
it invokes terminate() on the WorkerRunLoop.

As a result, before the run loop is terminated, the worker thread may observe the
TerminatedExecutionException in JS code, bail out, see another JS task to run,
re-enters the VM to run said JS code, and fails with an assertion due to the
TerminatedExecutionException still being pending on VM entry.

WorkerRunLoop::Task::performTask() already has a check to only allow a task to
run if and only if !runLoop.terminated() and the task is not a clean up task.
We'll fix the above race by changing WorkerRunLoop::Task::performTask() to check
!context->script()->isTerminatingExecution() instead of !runLoop.terminated().
Since WorkerThread::stop() always scheduleExecutionTermination() before it
terminates the run loop, !context->script()->isTerminatingExecution() implies
!runLoop.terminated().

The only time that runLoop is terminated without scheduleExecutionTermination()
being called is when WorkerThread::stop() is called before the WorkerThread has
finished creating its WorkerGlobalScope.  In this scenario, WorkerThread::stop()
will still terminate the run loop.  Hence, after the WorkerGlobalScope is created
(in WorkerThread::workerThread()), we will check if the run loop has been
terminated (i.e. stop() was called).  If so, we'll scheduleExecutionTermination()
there, and guarantee that if runloop.terminated() is true, then
context->script()->isTerminatingExecution() is also true.

Solutions that were considered but did not work (recorded for future reference):

1. In WorkerThread::stop(), call scheduleExecutionTermination() only after it
   posts the cleanup task and terminate the run loop.

   This did not work because this creates a race where the worker thread may run
   the cleanup task before WorkerThread::stop() finishes.  As a result, the
   scriptController may be deleted before we get to invoke scheduleExecutionTermination()
   on it, thereby resulting in a use after free.

   To make this work, we would have to change the life cycle management strategy
   of the WorkerScriptController.  This is a more risky change that we would
   want to take on at this time, and may also not be worth the gain.

2. Break scheduleExecutionTermination() up into 2 parts i.e. WorkerThread::stop()
   will:
   1. set the scriptControllers m_isTerminatingExecution flag before
      posting the cleanup task and terminating the run loop, and
   2. invoke VM::notifyNeedsTermination() after posting the cleanup task and
      terminating the run loop.

   This requires that we protect the liveness of the VM until we can invoke
   notifyNeedsTermination() on it.

   This did not work because:
   1. We may end up destructing the VM in WorkerThread::stop() i.e. in the main
      web frame, but only the worker thread holds the JS lock for the VM.

      We can make the WorkerThread::stop() acquire the JS lock just before it
      releases the protected VM's RefPtr, but that would mean the main thread
      may be stuck waiting a bit for the worker thread to release its JSLock.
      This is not desirable.

   2. In practice, changing the liveness period of the Worker VM relative to its
      WorkerScriptController and WorkerGlobalScope also has unexpected
      ramifications.  We observed many worker tests failing with assertion
      failures and crashes due to this change.

   Hence, this approach is also a more risky change than it appears on the
   surface, and is not worth exploring at this time.

In the end, changing WorkerRunLoop::Task::performTask() to check for
!scriptController->isTerminatingExecution() is the most straight forward solution
that is easy to prove correct.

Also fixed a race in WorkerThread::workerThread() where it can delete the
WorkerGlobalScope while WorkerThread::stop() is in the midst of accessing it.
We now guard the the nullifying of m_workerGlobalScope with the
m_threadCreationAndWorkerGlobalScopeMutex as well.

UPDATE: the only new thing in this patch for re-landing (vs one previously landed)
is that instead of nullifying m_workerGlobalScope directly (thereby deleting the
WorkerGlobalScope context), we'll swap it out and delete it only after we've
unlocked the m_threadCreationAndWorkerGlobalScopeMutex.  This is needed because
the destruction of the WorkerGlobalScope will cause the main thread to race against
the worker thread to delete the WorkerThread object, and the WorkerThread object
owns the mutex that we need to unlock after nullifying the m_workerGlobalScope
field.

This issue is covered by an existing test that I just unskipped in TestExpectations.

* bindings/js/JSDOMPromiseDeferred.cpp:
(WebCore::DeferredPromise::callFunction):

* bindings/js/WorkerScriptController.cpp:
(WebCore::WorkerScriptController::scheduleExecutionTermination):
- Added a check to do nothing and return early if the scriptController is already
  terminating execution.

* workers/WorkerRunLoop.cpp:
(WebCore::WorkerRunLoop::runInMode):
(WebCore::WorkerRunLoop::runCleanupTasks):
(WebCore::WorkerRunLoop::Task::performTask):

* workers/WorkerRunLoop.h:
- Made Task::performTask() private and make Task befriend the WorkerRunLoop class.
  This ensures that only the WorkerRunLoop may call performTask().
  Note: this change only formalizes and hardens a relationship that was already
  in place before this.

* workers/WorkerThread.cpp:
(WebCore::WorkerThread::start):
(WebCore::WorkerThread::workerThread):
(WebCore::WorkerThread::stop):
* workers/WorkerThread.h:
- Renamed m_threadCreationMutex to m_threadCreationAndWorkerGlobalScopeMutex so
  that it more accurately describes what it guards.

LayoutTests:

* TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@216876 268f45cc-cd09-0410-ab3c-d52691b4dbfc
15 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ExceptionScope.cpp
Source/JavaScriptCore/runtime/ExceptionScope.h
Source/JavaScriptCore/runtime/Options.h
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp
Source/WebCore/bindings/js/WorkerScriptController.cpp
Source/WebCore/workers/WorkerRunLoop.cpp
Source/WebCore/workers/WorkerRunLoop.h
Source/WebCore/workers/WorkerThread.cpp
Source/WebCore/workers/WorkerThread.h