Changeset 216635 in webkit

Timestamp:

May 10, 2017, 4:22:33 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

WorkerThread::stop() should call scheduleExecutionTermination() last.
​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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ExceptionScope.cpp (modified) (5 diffs)
• Source/JavaScriptCore/runtime/ExceptionScope.h (modified) (3 diffs)
• Source/JavaScriptCore/runtime/VM.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/VM.h (modified) (3 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp (modified) (3 diffs)
• Source/WebCore/workers/WorkerThread.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/TestExpectations

@@ -365 +365 @@
 
 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 ]

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/runtime/ExceptionScope.cpp

@@ -1 +1 @@
 /*
- * 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
@@ -28 +28 @@
 
 #include "Exception.h"
+#include "ExceptionHelpers.h"
 #include <wtf/StackTrace.h>
 #include <wtf/StringPrintStream.h>
+#include <wtf/Threading.h>
 
 namespace JSC {
@@ -54 +56 @@
     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, "    ");
 
@@ -61 +63 @@
         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, "    ");
 
@@ -69 +71 @@
 #endif // ENABLE(EXCEPTION_SCOPE_VERIFICATION)
     
+void ExceptionScope::releaseAssertIsTerminatedExecutionException()
+{
+    RELEASE_ASSERT_WITH_MESSAGE(isTerminatedExecutionException(m_vm, exception()), "%s", unexpectedExceptionMessage().data());
+}
+
 } // namespace JSC

trunk/Source/JavaScriptCore/runtime/ExceptionScope.h

@@ -43 +43 @@
     ALWAYS_INLINE void releaseAssertNoException() { RELEASE_ASSERT_WITH_MESSAGE(!exception(), "%s", unexpectedExceptionMessage().data()); }
 
+    void releaseAssertIsTerminatedExecutionException();
+
 protected:
     ExceptionScope(VM&, ExceptionEventLocation);
@@ -63 +65 @@
     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:
@@ -74 +77 @@
     ExceptionScope(const ExceptionScope&) = delete;
     ExceptionScope(ExceptionScope&&) = default;
+
+    ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
 
     VM& m_vm;

trunk/Source/JavaScriptCore/runtime/VM.cpp

@@ -620 +620 @@
 
 #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
 }

trunk/Source/JavaScriptCore/runtime/VM.h

@@ -685 +685 @@
 #if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
     StackTrace* nativeStackTraceOfLastThrow() const { return m_nativeStackTraceOfLastThrow.get(); }
+    ThreadIdentifier throwingThread() const { return m_throwingThread; }
 #endif
 
@@ -720 +721 @@
         m_needExceptionCheck = false;
         m_nativeStackTraceOfLastThrow = nullptr;
+        m_throwingThread = 0;
 #endif
         m_exception = nullptr;
@@ -767 +769 @@
     mutable bool m_needExceptionCheck { false };
     std::unique_ptr<StackTrace> m_nativeStackTraceOfLastThrow;
+    ThreadIdentifier m_throwingThread;
 #endif
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp

@@ -1 +1 @@
 /*
- * 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
@@ -50 +50 @@
         return;
 
+    VM& vm = exec.vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     CallData callData;
     CallType callType = getCallData(function, callData);
@@ -58 +61 @@
 
     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();

trunk/Source/WebCore/workers/WorkerThread.cpp

@@ -235 +235 @@
     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);
@@ -266 +263 @@
     }
     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();
 }