WorkerThread::stop() should call scheduleExecutionTermination() last.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 May 2017 23:22:33 +0000 (23:22 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 May 2017 23:22:33 +0000 (23:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171775
<rdar://problem/30975761>

Reviewed by Geoffrey Garen.

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):
(JSC::ExceptionScope::releaseAssertIsTerminatedExecutionException):
* runtime/ExceptionScope.h:
(JSC::ExceptionScope::exception):
(JSC::ExceptionScope::unexpectedExceptionMessage):
* 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 run loop is terminate, 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 ensuring that having WorkerThread::stop() terminate
the run loop before it scheduleExecutionTermination() which throws the
TerminatedExecutionException.  This way, by the time JS code unwinds out of the
VM due to the TerminatedExecutionException, runLoop.terminated() is guaranteed
to be true and thereby prevents re-entry into the VM.

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

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

LayoutTests:

* TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@216635 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ExceptionScope.cpp
Source/JavaScriptCore/runtime/ExceptionScope.h
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp
Source/WebCore/workers/WorkerThread.cpp

index 03e2db1..a1daef0 100644 (file)
@@ -1,3 +1,13 @@
+2017-05-10  Mark Lam  <mark.lam@apple.com>
+
+        WorkerThread::stop() should call scheduleExecutionTermination() last.
+        https://bugs.webkit.org/show_bug.cgi?id=171775
+        <rdar://problem/30975761>
+
+        Reviewed by Geoffrey Garen.
+
+        * TestExpectations:
+
 2017-05-10  Tim Horton  <timothy_horton@apple.com>
 
         Add an experimental feature flag for viewport-fit
index 15ae457..9d23372 100644 (file)
@@ -365,9 +365,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 c0bba6c..b471898 100644 (file)
@@ -1,5 +1,32 @@
 2017-05-10  Mark Lam  <mark.lam@apple.com>
 
+        WorkerThread::stop() should call scheduleExecutionTermination() last.
+        https://bugs.webkit.org/show_bug.cgi?id=171775
+        <rdar://problem/30975761>
+
+        Reviewed by Geoffrey Garen.
+
+        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):
+        (JSC::ExceptionScope::releaseAssertIsTerminatedExecutionException):
+        * runtime/ExceptionScope.h:
+        (JSC::ExceptionScope::exception):
+        (JSC::ExceptionScope::unexpectedExceptionMessage):
+        * runtime/VM.cpp:
+        (JSC::VM::throwException):
+        * runtime/VM.h:
+        (JSC::VM::throwingThread):
+        (JSC::VM::clearException):
+
+2017-05-10  Mark Lam  <mark.lam@apple.com>
+
         Crash in JavaScriptCore GC when using JSC on dispatch queues (thread_get_state returns NULL stack pointer).
         https://bugs.webkit.org/show_bug.cgi?id=160337
         <rdar://problem/27611733>
index 2e64787..39da9dc 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(100, 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();
@@ -68,4 +70,9 @@ CString ExceptionScope::unexpectedExceptionMessage()
 
 #endif // ENABLE(EXCEPTION_SCOPE_VERIFICATION)
     
+void ExceptionScope::releaseAssertIsTerminatedExecutionException()
+{
+    RELEASE_ASSERT_WITH_MESSAGE(isTerminatedExecutionException(m_vm, exception()), "%s", unexpectedExceptionMessage().data());
+}
+
 } // namespace JSC
index 7df8e02..8251bb7 100644 (file)
@@ -42,6 +42,8 @@ public:
     ALWAYS_INLINE void assertNoException() { ASSERT_WITH_MESSAGE(!exception(), "%s", unexpectedExceptionMessage().data()); }
     ALWAYS_INLINE void releaseAssertNoException() { RELEASE_ASSERT_WITH_MESSAGE(!exception(), "%s", unexpectedExceptionMessage().data()); }
 
+    void releaseAssertIsTerminatedExecutionException();
+
 protected:
     ExceptionScope(VM&, ExceptionEventLocation);
     ExceptionScope(const ExceptionScope&) = delete;
@@ -62,11 +64,12 @@ 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()); }
 
+    void releaseAssertIsTerminatedExecutionException();
+
 protected:
     ALWAYS_INLINE ExceptionScope(VM& vm)
         : m_vm(vm)
@@ -74,6 +77,8 @@ protected:
     ExceptionScope(const ExceptionScope&) = delete;
     ExceptionScope(ExceptionScope&&) = default;
 
+    ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
+
     VM& m_vm;
 };
     
index c3fc541..91ecfc1 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(100));
+    m_throwingThread = currentThread();
 #endif
 }
 
index 178a3e2..997e6ce 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 9b6c44f..491f851 100644 (file)
@@ -1,3 +1,35 @@
+2017-05-10  Mark Lam  <mark.lam@apple.com>
+
+        WorkerThread::stop() should call scheduleExecutionTermination() last.
+        https://bugs.webkit.org/show_bug.cgi?id=171775
+        <rdar://problem/30975761>
+
+        Reviewed by Geoffrey Garen.
+
+        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 run loop is terminate, 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 ensuring that having WorkerThread::stop() terminate
+        the run loop before it scheduleExecutionTermination() which throws the
+        TerminatedExecutionException.  This way, by the time JS code unwinds out of the
+        VM due to the TerminatedExecutionException, runLoop.terminated() is guaranteed
+        to be true and thereby prevents re-entry into the VM.
+
+        This issue is covered by an existing test that I just unskipped in TestExpectations.
+
+        * bindings/js/JSDOMPromiseDeferred.cpp:
+        (WebCore::DeferredPromise::callFunction):
+        * workers/WorkerThread.cpp:
+        (WebCore::WorkerThread::stop):
+
 2017-05-10  Tim Horton  <timothy_horton@apple.com>
 
         Add an experimental feature flag for viewport-fit
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 af55395..ec02e04 100644 (file)
@@ -234,10 +234,7 @@ void WorkerThread::stop()
     // Mutex protection is necessary because stop() can be called before the context is fully created.
     LockHolder lock(m_threadCreationMutex);
 
-    // 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) {
-        m_workerGlobalScope->script()->scheduleExecutionTermination();
-
         m_runLoop.postTaskAndTerminate({ ScriptExecutionContext::Task::CleanupTask, [] (ScriptExecutionContext& context ) {
             WorkerGlobalScope& workerGlobalScope = downcast<WorkerGlobalScope>(context);
 
@@ -265,6 +262,10 @@ void WorkerThread::stop()
         return;
     }
     m_runLoop.terminate();
+
+    // 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)
+        m_workerGlobalScope->script()->scheduleExecutionTermination();
 }
 
 void WorkerThread::releaseFastMallocFreeMemoryInAllThreads()