WorkerThread::stop() should call scheduleExecutionTermination() last.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 May 2017 15:26:03 +0000 (15:26 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 May 2017 15:26:03 +0000 (15:26 +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@216677 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 a0afdb0..cd0ecf8 100644 (file)
@@ -1,3 +1,13 @@
+2017-05-11  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-11  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed GTK+ gardening. Update expectations of tests failing after r216450.
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 b0d25ab..9c229a4 100644 (file)
@@ -1,3 +1,30 @@
+2017-05-11  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-11  JF Bastien  <jfbastien@apple.com>
 
         WebAssembly: stop supporting 0xD
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 3af9af0..6e903b8 100644 (file)
@@ -1,3 +1,35 @@
+2017-05-11  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-11  Chris Dumez  <cdumez@apple.com>
 
         Drop custom bindings code for HTMLFormControlsCollection's named property getter
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..79a5000 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
@@ -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);
 
@@ -262,6 +259,15 @@ void WorkerThread::stop()
             } });
 
         } });
+        
+        // Prevent a while (1) { } loop from keeping the worker alive forever by requesting that the worker thread
+        // throw an uncatchable TerminatedExecutionException.
+        //
+        // We should only do this after terminating the runloop via the call to m_runLoop.postTaskAndTerminate()
+        // above. This ensures that when the worker bails out of JS code due to the TerminatedExecutionException,
+        // the m_runLoop.terminated() flag will already be set, and WorkerRunLoop::Task::performTask() won't try to
+        // re-enter the VM with the TerminatedExecutionException still pending thereafter.
+        m_workerGlobalScope->script()->scheduleExecutionTermination();
         return;
     }
     m_runLoop.terminate();