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

Reviewed by Saam Barati.

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.

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@216801 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

index 03b34e3..e964af6 100644 (file)
@@ -1,3 +1,13 @@
+2017-05-12  Mark Lam  <mark.lam@apple.com>
+
+        WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
+        https://bugs.webkit.org/show_bug.cgi?id=171775
+        <rdar://problem/30975761>
+
+        Reviewed by Saam Barati.
+
+        * TestExpectations:
+
 2017-05-12  Daniel Bates  <dabates@apple.com>
 
         Attempt to fix timeout failure of test plugins/navigator-plugin-crash.html in WebKit1
index ace13c6..c501ef3 100644 (file)
@@ -364,9 +364,6 @@ imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html [ Pass
 
 webkit.org/b/161312 imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/document-without-browsing-context.html [ Failure Pass ]
 
-# rdar://problem/30975761
-[ Debug ] http/tests/fetch/fetch-in-worker-crash.html [ Skip ]
-
 imported/w3c/web-platform-tests/XMLHttpRequest/send-conditional-cors.htm [ Failure ]
 imported/w3c/web-platform-tests/fetch/http-cache/partial.html [ Failure ]
 webkit.org/b/159724 imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-post-upload.htm [ Failure Pass ]
index de36d2b..86f04a4 100644 (file)
@@ -1,3 +1,31 @@
+2017-05-12  Mark Lam  <mark.lam@apple.com>
+
+        WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
+        https://bugs.webkit.org/show_bug.cgi?id=171775
+        <rdar://problem/30975761>
+
+        Reviewed by Saam Barati.
+
+        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):
+
 2017-05-12  Daniel Bates  <dabates@apple.com>
 
         Cleanup: Make QueueTaskToEventLoopFunctionPtr take JSGlobalObject&
index 2e64787..9544c64 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #include "ExceptionScope.h"
 
 #include "Exception.h"
+#include "ExceptionHelpers.h"
 #include <wtf/StackTrace.h>
 #include <wtf/StringPrintStream.h>
+#include <wtf/Threading.h>
 
 namespace JSC {
     
@@ -53,14 +55,14 @@ CString ExceptionScope::unexpectedExceptionMessage()
 {
     StringPrintStream out;
 
-    out.println("Unexpected exception observed at:");
-    auto currentStack = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(25, 1));
+    out.println("Unexpected exception observed on thread ", currentThread(), " at:");
+    auto currentStack = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(Options::unexpectedExceptionStackTraceLimit(), 1));
     currentStack->dump(out, "    ");
 
     if (!m_vm.nativeStackTraceOfLastThrow())
         return CString();
     
-    out.println("The exception was thrown from:");
+    out.println("The exception was thrown from thread ", m_vm.throwingThread(), " at:");
     m_vm.nativeStackTraceOfLastThrow()->dump(out, "    ");
 
     return out.toCString();
index 7df8e02..31553c8 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -62,7 +62,6 @@ class ExceptionScope {
 public:
     ALWAYS_INLINE VM& vm() const { return m_vm; }
     ALWAYS_INLINE Exception* exception() { return m_vm.exception(); }
-    ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
 
     ALWAYS_INLINE void assertNoException() { ASSERT(!exception()); }
     ALWAYS_INLINE void releaseAssertNoException() { RELEASE_ASSERT(!exception()); }
@@ -74,6 +73,8 @@ protected:
     ExceptionScope(const ExceptionScope&) = delete;
     ExceptionScope(ExceptionScope&&) = default;
 
+    ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
+
     VM& m_vm;
 };
     
index 728a273..5137af8 100644 (file)
@@ -392,6 +392,7 @@ typedef const char* optionString;
     v(bool, validateDFGExceptionHandling, false, Normal, "Causes the DFG to emit code validating exception handling for each node that can exit") /* This is true by default on Debug builds */\
     v(bool, dumpSimulatedThrows, false, Normal, "Dumps the call stack at each simulated throw for exception scope verification") \
     v(bool, validateExceptionChecks, false, Normal, "Verifies that needed exception checks are performed.") \
+    v(unsigned, unexpectedExceptionStackTraceLimit, 100, Normal, "Stack trace limit for debugging unexpected exceptions observed in the VM") \
     \
     v(bool, useExecutableAllocationFuzz, false, Normal, nullptr) \
     v(unsigned, fireExecutableAllocationFuzzAt, 0, Normal, nullptr) \
index 27a2fcd..1325059 100644 (file)
@@ -619,7 +619,8 @@ void VM::throwException(ExecState* exec, Exception* exception)
     setException(exception);
 
 #if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
-    m_nativeStackTraceOfLastThrow = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(25));
+    m_nativeStackTraceOfLastThrow = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(Options::unexpectedExceptionStackTraceLimit()));
+    m_throwingThread = currentThread();
 #endif
 }
 
index 7a4511a..25f92e8 100644 (file)
@@ -684,6 +684,7 @@ public:
 
 #if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
     StackTrace* nativeStackTraceOfLastThrow() const { return m_nativeStackTraceOfLastThrow.get(); }
+    ThreadIdentifier throwingThread() const { return m_throwingThread; }
 #endif
 
 private:
@@ -719,6 +720,7 @@ private:
 #if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
         m_needExceptionCheck = false;
         m_nativeStackTraceOfLastThrow = nullptr;
+        m_throwingThread = 0;
 #endif
         m_exception = nullptr;
     }
@@ -766,6 +768,7 @@ private:
     unsigned m_simulatedThrowPointRecursionDepth { 0 };
     mutable bool m_needExceptionCheck { false };
     std::unique_ptr<StackTrace> m_nativeStackTraceOfLastThrow;
+    ThreadIdentifier m_throwingThread;
 #endif
 
     bool m_failNextNewCodeBlock { false };
index 468b775..cc30782 100644 (file)
@@ -1,3 +1,116 @@
+2017-05-12  Mark Lam  <mark.lam@apple.com>
+
+        WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
+        https://bugs.webkit.org/show_bug.cgi?id=171775
+        <rdar://problem/30975761>
+
+        Reviewed by Saam Barati.
+
+        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.
+
+        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.
+
 2017-05-12  Zalan Bujtas  <zalan@apple.com>
 
         [iOS WK1] Do not try to layout a subframe if its document has not been constructed yet.
index 095a2d2..0a58989 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -49,6 +49,9 @@ void DeferredPromise::callFunction(ExecState& exec, JSValue function, JSValue re
     if (!canInvokeCallback())
         return;
 
+    VM& vm = exec.vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     CallData callData;
     CallType callType = getCallData(function, callData);
     ASSERT(callType != CallType::None);
@@ -58,6 +61,10 @@ void DeferredPromise::callFunction(ExecState& exec, JSValue function, JSValue re
 
     call(&exec, function, callType, callData, jsUndefined(), arguments);
 
+    // DeferredPromise should only be used by internal implementations that are well behaved.
+    // In practice, the only exception we should ever see here is the TerminatedExecutionException.
+    ASSERT_UNUSED(scope, !scope.exception() || isTerminatedExecutionException(vm, scope.exception()));
+
     clear();
 }
 
index df8250c..ac97625 100644 (file)
@@ -152,6 +152,9 @@ void WorkerScriptController::setException(JSC::Exception* exception)
 
 void WorkerScriptController::scheduleExecutionTermination()
 {
+    if (m_isTerminatingExecution)
+        return;
+
     {
         // The mutex provides a memory barrier to ensure that once
         // termination is scheduled, isTerminatingExecution() will
index 1134532..b1c205e 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
- * Copyright (C) 2016 Apple Inc.  All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc.  All rights reserved.
  * 
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -199,7 +199,7 @@ MessageQueueWaitResult WorkerRunLoop::runInMode(WorkerGlobalScope* context, cons
         break;
 
     case MessageQueueMessageReceived:
-        task->performTask(*this, context);
+        task->performTask(context);
         break;
 
     case MessageQueueTimeout:
@@ -228,7 +228,7 @@ void WorkerRunLoop::runCleanupTasks(WorkerGlobalScope* context)
         auto task = m_messageQueue.tryGetMessageIgnoringKilled();
         if (!task)
             return;
-        task->performTask(*this, context);
+        task->performTask(context);
     }
 }
 
@@ -252,9 +252,10 @@ void WorkerRunLoop::postTaskForMode(ScriptExecutionContext::Task&& task, const S
     m_messageQueue.append(std::make_unique<Task>(WTFMove(task), mode));
 }
 
-void WorkerRunLoop::Task::performTask(const WorkerRunLoop& runLoop, WorkerGlobalScope* context)
+void WorkerRunLoop::Task::performTask(WorkerGlobalScope* context)
 {
-    if ((!context->isClosing() && !runLoop.terminated()) || m_task.isCleanupTask())
+    ASSERT(context->script());
+    if ((!context->isClosing() && !context->script()->isTerminatingExecution()) || m_task.isCleanupTask())
         m_task.performTask(*context);
 }
 
index 317dfbd..e1e01e6 100644 (file)
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
- * 
+ * Copyright (C) 2017 Apple Inc.  All rights reserved.
+ *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
@@ -70,11 +71,14 @@ namespace WebCore {
         public:
             Task(ScriptExecutionContext::Task&&, const String& mode);
             const String& mode() const { return m_mode; }
-            void performTask(const WorkerRunLoop&, WorkerGlobalScope*);
 
         private:
+            void performTask(WorkerGlobalScope*);
+
             ScriptExecutionContext::Task m_task;
             String m_mode;
+
+            friend class WorkerRunLoop;
         };
 
     private:
index af55395..77a9ddc 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008 Apple Inc. All Rights Reserved.
+ * Copyright (C) 2008-2017 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -133,7 +133,7 @@ WorkerThread::~WorkerThread()
 bool WorkerThread::start()
 {
     // Mutex protection is necessary to ensure that m_thread is initialized when the thread starts.
-    LockHolder lock(m_threadCreationMutex);
+    LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
 
     if (m_thread)
         return true;
@@ -160,14 +160,21 @@ void WorkerThread::workerThread()
     g_main_context_push_thread_default(mainContext.get());
 #endif
 
+    WorkerScriptController* scriptController;
     {
-        LockHolder lock(m_threadCreationMutex);
+        // Mutex protection is necessary to ensure that we don't change m_workerGlobalScope
+        // while WorkerThread::stop() is accessing it. Note that WorkerThread::stop() can
+        // be called before we've finished creating the WorkerGlobalScope.
+        LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
         m_workerGlobalScope = createWorkerGlobalScope(m_startupData->m_scriptURL, m_startupData->m_identifier, m_startupData->m_userAgent, m_startupData->m_contentSecurityPolicyResponseHeaders, m_startupData->m_shouldBypassMainWorldContentSecurityPolicy, WTFMove(m_startupData->m_topOrigin), m_startupData->m_timeOrigin);
 
+        scriptController = m_workerGlobalScope->script();
+
         if (m_runLoop.terminated()) {
             // The worker was terminated before the thread had a chance to run. Since the context didn't exist yet,
             // forbidExecution() couldn't be called from stop().
-            m_workerGlobalScope->script()->forbidExecution();
+            scriptController->scheduleExecutionTermination();
+            scriptController->forbidExecution();
         }
     }
 
@@ -176,11 +183,10 @@ void WorkerThread::workerThread()
 
         // If the worker was somehow terminated while processing debugger commands.
         if (m_runLoop.terminated())
-            m_workerGlobalScope->script()->forbidExecution();
+            scriptController->forbidExecution();
     }
 
-    WorkerScriptController* script = m_workerGlobalScope->script();
-    script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL));
+    scriptController->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL));
     // Free the startup data to cause its member variable deref's happen on the worker's thread (since
     // all ref/derefs of these objects are happening on the thread at this point). Note that
     // WorkerThread::~WorkerThread happens on a different thread where it was created.
@@ -198,7 +204,12 @@ void WorkerThread::workerThread()
 
     // The below assignment will destroy the context, which will in turn notify messaging proxy.
     // We cannot let any objects survive past thread exit, because no other thread will run GC or otherwise destroy them.
-    m_workerGlobalScope = nullptr;
+    {
+        // Mutex protection is necessary to ensure that we don't change m_workerGlobalScope
+        // while WorkerThread::stop is accessing it.
+        LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
+        m_workerGlobalScope = nullptr;
+    }
 
     // Clean up WebCore::ThreadGlobalData before WTF::WTFThreadData goes away!
     threadGlobalData().destroy();
@@ -231,8 +242,10 @@ void WorkerThread::runEventLoop()
 
 void WorkerThread::stop()
 {
-    // Mutex protection is necessary because stop() can be called before the context is fully created.
-    LockHolder lock(m_threadCreationMutex);
+    // Mutex protection is necessary to ensure that m_workerGlobalScope isn't changed by
+    // WorkerThread::workerThread() while we're accessing it. Note also that stop() can
+    // be called before m_workerGlobalScope is fully created.
+    LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
 
     // Ensure that tasks are being handled by thread event loop. If script execution weren't forbidden, a while(1) loop in JS could keep the thread alive forever.
     if (m_workerGlobalScope) {
index 2cd4372..29f4d98 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, 2016 Apple Inc. All Rights Reserved.
+ * Copyright (C) 2008-2017 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -106,7 +106,7 @@ private:
     bool m_pausedForDebugger { false };
 
     RefPtr<WorkerGlobalScope> m_workerGlobalScope;
-    Lock m_threadCreationMutex;
+    Lock m_threadCreationAndWorkerGlobalScopeMutex;
 
     std::unique_ptr<WorkerThreadStartupData> m_startupData;