Changeset 260165 in webkit

Timestamp:

Apr 15, 2020 6:40:42 PM (4 years ago)

Author:

rmorisset@apple.com

Message:

Flaky Test: fetch/fetch-worker-crash.html
​https://bugs.webkit.org/show_bug.cgi?id=187257
<rdar://problem/48527526>

Reviewed by Yusuke Suzuki.

Source/JavaScriptCore:

The crash is coming from setExceptionPorts which is inlined in WTF::registerThreadForMachExceptionHandling.
From the error message we know that the problem is an "invalid port right".
​http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/thread_set_exception_ports.html tells us that the "port right" is the third parameter to thread_set_exception_ports, which is exceptionPort in our case.
exceptionPort is a global variable defined at the top of Signals.cpp:

static mach_port_t exceptionPort;

It is set in exactly one place:

kern_return_t kr = mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_RECEIVE, &exceptionPort);

in a std::call_once, in startMachExceptionHandlerThread().
Note that startMachExceptionHandlerThread() is called from the main thread just before the point where we are stuck.. and there is no synchronization to make sure it completed and its effect is visible to the worker thread before it uses exceptionPort.

So I think the crash is due to this race between allocating exceptionPort and using it, resulting in an invalid exceptionPort being sometimes passed to the kernel.
So this patch is a simple speculative fix, by running startMachExceptionHandlerThread() in initializeThreading(), before JSLock()::lock() can be run.

• runtime/InitializeThreading.cpp:

(JSC::initializeThreading):

Source/WTF:

Make startMachExceptionHandlerThread visible so that we can make sure it is called whenever initializing JSC.

• wtf/threads/Signals.cpp:

(WTF::startMachExceptionHandlerThread):

• wtf/threads/Signals.h:

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/runtime/InitializeThreading.cpp (modified) (2 diffs)
• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/threads/Signals.cpp (modified) (1 diff)
• WTF/wtf/threads/Signals.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-04-15  Robin Morisset  <rmorisset@apple.com>
+
+        Flaky Test: fetch/fetch-worker-crash.html
+        https://bugs.webkit.org/show_bug.cgi?id=187257
+        <rdar://problem/48527526>
+
+        Reviewed by Yusuke Suzuki.
+
+        The crash is coming from setExceptionPorts which is inlined in WTF::registerThreadForMachExceptionHandling.
+        From the error message we know that the problem is an "invalid port right".
+        http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/thread_set_exception_ports.html tells us that the "port right" is the third parameter to thread_set_exception_ports, which is exceptionPort in our case.
+        exceptionPort is a global variable defined at the top of Signals.cpp:
+          static mach_port_t exceptionPort;
+        It is set in exactly one place:
+          kern_return_t kr = mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_RECEIVE, &exceptionPort);
+        in a std::call_once, in startMachExceptionHandlerThread().
+        Note that startMachExceptionHandlerThread() is called from the main thread just before the point where we are stuck.. and there is no synchronization to make sure it completed and its effect is visible to the worker thread before it uses exceptionPort.
+
+        So I think the crash is due to this race between allocating exceptionPort and using it, resulting in an invalid exceptionPort being sometimes passed to the kernel.
+        So this patch is a simple speculative fix, by running startMachExceptionHandlerThread() in initializeThreading(), before JSLock()::lock() can be run.
+
+        * runtime/InitializeThreading.cpp:
+        (JSC::initializeThreading):
+
 2020-04-15  Ross Kirsling  <ross.kirsling@sony.com>
 

trunk/Source/JavaScriptCore/runtime/InitializeThreading.cpp

@@ -54 +54 @@
 #include <wtf/dtoa.h>
 #include <wtf/dtoa/cached-powers.h>
+#include <wtf/threads/Signals.h>
 
 namespace JSC {
@@ -100 +101 @@
         if (VM::isInMiniMode())
             WTF::fastEnableMiniMode();
+
+#if HAVE(MACH_EXCEPTIONS)
+        // JSLock::lock() can call registerThreadForMachExceptionHandling() which crashes if this has not been called first.
+        WTF::startMachExceptionHandlerThread();
+#endif
     });
 }

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-04-15  Robin Morisset  <rmorisset@apple.com>
+
+        Flaky Test: fetch/fetch-worker-crash.html
+        https://bugs.webkit.org/show_bug.cgi?id=187257
+        <rdar://problem/48527526>
+
+        Reviewed by Yusuke Suzuki.
+
+        Make startMachExceptionHandlerThread visible so that we can make sure it is called whenever initializing JSC.
+
+        * wtf/threads/Signals.cpp:
+        (WTF::startMachExceptionHandlerThread):
+        * wtf/threads/Signals.h:
+
 2020-04-14  Peng Liu  <peng.liu6@apple.com>
 

trunk/Source/WTF/wtf/threads/Signals.cpp

@@ -70 +70 @@
 static constexpr size_t maxMessageSize = 1 * KB;
 
-static void startMachExceptionHandlerThread()
+void startMachExceptionHandlerThread()
 {
     static std::once_flag once;

trunk/Source/WTF/wtf/threads/Signals.h

@@ -94 +94 @@
 class Thread;
 void registerThreadForMachExceptionHandling(Thread&);
+void startMachExceptionHandlerThread();
 
 void handleSignalsWithMach();