VMTraps has some races
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Jun 2017 17:34:57 +0000 (17:34 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Jun 2017 17:34:57 +0000 (17:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173941

Reviewed by Michael Saboff.

Source/JavaScriptCore:

This patch refactors much of the VMTraps API.

On the message sending side:

1) No longer uses the Yarr JIT check to determine if we are in
RegExp code. That was unsound because RegExp JIT code can be run
on compilation threads.  Instead it looks at the current frame's
code block slot and checks if it is valid, which is the same as
what it did for JIT code previously.

2) Only have one signal sender thread, previously, there could be
many at once, which caused some data races. Additionally, the
signal sender thread is an automatic thread so it will deallocate
itself when not in use.

On the VMTraps breakpoint side:

1) We now have a true mapping of if we hit a breakpoint instead of
a JIT assertion. So the exception handler won't eat JIT assertions
anymore.

2) It jettisons all CodeBlocks that have VMTraps breakpoints on
them instead of every CodeBlock on the stack. This both prevents
us from hitting stale VMTraps breakpoints and also doesn't OSR
codeblocks that otherwise don't need to be jettisoned.

3) The old exception handler could theoretically fail for a couple
of reasons then resume execution with a clobbered instruction
set. This patch will kill the program if the exception handler
would fail.

This patch also refactors some of the jsc.cpp functions to take the
CommandLine options object instead of individual options. Also, there
is a new command line option that makes exceptions due to watchdog
timeouts an acceptable result.

* API/tests/testapi.c:
(main):
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::installVMTrapBreakpoints):
* dfg/DFGCommonData.cpp:
(JSC::DFG::pcCodeBlockMap):
(JSC::DFG::CommonData::invalidate):
(JSC::DFG::CommonData::~CommonData):
(JSC::DFG::CommonData::installVMTrapBreakpoints):
(JSC::DFG::codeBlockForVMTrapPC):
* dfg/DFGCommonData.h:
* jsc.cpp:
(functionDollarAgentStart):
(checkUncaughtException):
(checkException):
(runWithOptions):
(printUsageStatement):
(CommandLine::parseArguments):
(jscmain):
(runWithScripts): Deleted.
* runtime/JSLock.cpp:
(JSC::JSLock::didAcquireLock):
* runtime/VMTraps.cpp:
(JSC::sanitizedTopCallFrame):
(JSC::VMTraps::tryInstallTrapBreakpoints):
(JSC::VMTraps::willDestroyVM):
(JSC::VMTraps::fireTrap):
(JSC::VMTraps::handleTraps):
(JSC::VMTraps::VMTraps):
(JSC::VMTraps::~VMTraps):
(JSC::findActiveVMAndStackBounds): Deleted.
(JSC::installSignalHandler): Deleted.
(JSC::VMTraps::addSignalSender): Deleted.
(JSC::VMTraps::removeSignalSender): Deleted.
(JSC::VMTraps::SignalSender::willDestroyVM): Deleted.
(JSC::VMTraps::SignalSender::send): Deleted.
* runtime/VMTraps.h:
(JSC::VMTraps::~VMTraps): Deleted.
(JSC::VMTraps::SignalSender::SignalSender): Deleted.

Tools:

Add new testing mode for testing the Watchdog with our stress
tests.

* Scripts/run-jsc-stress-tests:

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

Source/JavaScriptCore/API/tests/testapi.c
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/dfg/DFGCommonData.cpp
Source/JavaScriptCore/dfg/DFGCommonData.h
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/runtime/JSLock.cpp
Source/JavaScriptCore/runtime/VMTraps.cpp
Source/JavaScriptCore/runtime/VMTraps.h
Tools/ChangeLog
Tools/Scripts/run-jsc-stress-tests

index 9d3b456..f12cc2e 100644 (file)
@@ -1348,7 +1348,6 @@ int main(int argc, char* argv[])
     ::SetErrorMode(0);
 #endif
 
-    testExecutionTimeLimit();
     testCompareAndSwap();
     startMultithreadedMultiVMExecutionTest();
 
index 606623b..2ee8371 100644 (file)
@@ -1,3 +1,86 @@
+2017-06-28  Keith Miller  <keith_miller@apple.com>
+
+        VMTraps has some races
+        https://bugs.webkit.org/show_bug.cgi?id=173941
+
+        Reviewed by Michael Saboff.
+
+        This patch refactors much of the VMTraps API.
+
+        On the message sending side:
+
+        1) No longer uses the Yarr JIT check to determine if we are in
+        RegExp code. That was unsound because RegExp JIT code can be run
+        on compilation threads.  Instead it looks at the current frame's
+        code block slot and checks if it is valid, which is the same as
+        what it did for JIT code previously.
+
+        2) Only have one signal sender thread, previously, there could be
+        many at once, which caused some data races. Additionally, the
+        signal sender thread is an automatic thread so it will deallocate
+        itself when not in use.
+
+        On the VMTraps breakpoint side:
+
+        1) We now have a true mapping of if we hit a breakpoint instead of
+        a JIT assertion. So the exception handler won't eat JIT assertions
+        anymore.
+
+        2) It jettisons all CodeBlocks that have VMTraps breakpoints on
+        them instead of every CodeBlock on the stack. This both prevents
+        us from hitting stale VMTraps breakpoints and also doesn't OSR
+        codeblocks that otherwise don't need to be jettisoned.
+
+        3) The old exception handler could theoretically fail for a couple
+        of reasons then resume execution with a clobbered instruction
+        set. This patch will kill the program if the exception handler
+        would fail.
+
+        This patch also refactors some of the jsc.cpp functions to take the
+        CommandLine options object instead of individual options. Also, there
+        is a new command line option that makes exceptions due to watchdog
+        timeouts an acceptable result.
+
+        * API/tests/testapi.c:
+        (main):
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::installVMTrapBreakpoints):
+        * dfg/DFGCommonData.cpp:
+        (JSC::DFG::pcCodeBlockMap):
+        (JSC::DFG::CommonData::invalidate):
+        (JSC::DFG::CommonData::~CommonData):
+        (JSC::DFG::CommonData::installVMTrapBreakpoints):
+        (JSC::DFG::codeBlockForVMTrapPC):
+        * dfg/DFGCommonData.h:
+        * jsc.cpp:
+        (functionDollarAgentStart):
+        (checkUncaughtException):
+        (checkException):
+        (runWithOptions):
+        (printUsageStatement):
+        (CommandLine::parseArguments):
+        (jscmain):
+        (runWithScripts): Deleted.
+        * runtime/JSLock.cpp:
+        (JSC::JSLock::didAcquireLock):
+        * runtime/VMTraps.cpp:
+        (JSC::sanitizedTopCallFrame):
+        (JSC::VMTraps::tryInstallTrapBreakpoints):
+        (JSC::VMTraps::willDestroyVM):
+        (JSC::VMTraps::fireTrap):
+        (JSC::VMTraps::handleTraps):
+        (JSC::VMTraps::VMTraps):
+        (JSC::VMTraps::~VMTraps):
+        (JSC::findActiveVMAndStackBounds): Deleted.
+        (JSC::installSignalHandler): Deleted.
+        (JSC::VMTraps::addSignalSender): Deleted.
+        (JSC::VMTraps::removeSignalSender): Deleted.
+        (JSC::VMTraps::SignalSender::willDestroyVM): Deleted.
+        (JSC::VMTraps::SignalSender::send): Deleted.
+        * runtime/VMTraps.h:
+        (JSC::VMTraps::~VMTraps): Deleted.
+        (JSC::VMTraps::SignalSender::SignalSender): Deleted.
+
 2017-06-28  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Instrument active pixel memory used by canvases
index 259e64d..2f8e18c 100644 (file)
@@ -3036,9 +3036,10 @@ bool CodeBlock::installVMTrapBreakpoints()
     // we should not perturb the refCount of m_jitCode.
     if (!JITCode::isOptimizingJIT(jitType()))
         return false;
-    m_jitCode->dfgCommon()->installVMTrapBreakpoints();
+    m_jitCode->dfgCommon()->installVMTrapBreakpoints(this);
     return true;
 #else
+    UNREACHABLE_FOR_PLATFORM();
     return false;
 #endif
 }
index cd1e5ef..961475c 100644 (file)
@@ -36,6 +36,8 @@
 #include "TrackedReferences.h"
 #include "VM.h"
 
+#include <wtf/NeverDestroyed.h>
+
 namespace JSC { namespace DFG {
 
 void CommonData::notifyCompilingStructureTransition(Plan& plan, CodeBlock* codeBlock, Node* node)
@@ -87,23 +89,76 @@ void CommonData::shrinkToFit()
     transitions.shrinkToFit();
 }
 
+static StaticLock pcCodeBlockMapLock;
+inline HashMap<void*, CodeBlock*>& pcCodeBlockMap(AbstractLocker&)
+{
+    static NeverDestroyed<HashMap<void*, CodeBlock*>> pcCodeBlockMap;
+    return pcCodeBlockMap;
+}
+
 bool CommonData::invalidate()
 {
     if (!isStillValid)
         return false;
+
+    if (UNLIKELY(hasVMTrapsBreakpointsInstalled)) {
+        LockHolder locker(pcCodeBlockMapLock);
+        auto& map = pcCodeBlockMap(locker);
+        for (auto& jumpReplacement : jumpReplacements)
+            map.remove(jumpReplacement.dataLocation());
+        hasVMTrapsBreakpointsInstalled = false;
+    }
+
     for (unsigned i = jumpReplacements.size(); i--;)
         jumpReplacements[i].fire();
     isStillValid = false;
     return true;
 }
 
-void CommonData::installVMTrapBreakpoints()
+CommonData::~CommonData()
 {
+    if (UNLIKELY(hasVMTrapsBreakpointsInstalled)) {
+        LockHolder locker(pcCodeBlockMapLock);
+        auto& map = pcCodeBlockMap(locker);
+        for (auto& jumpReplacement : jumpReplacements)
+            map.remove(jumpReplacement.dataLocation());
+    }
+}
+
+void CommonData::installVMTrapBreakpoints(CodeBlock* owner)
+{
+    LockHolder locker(pcCodeBlockMapLock);
     if (!isStillValid || hasVMTrapsBreakpointsInstalled)
         return;
     hasVMTrapsBreakpointsInstalled = true;
-    for (unsigned i = jumpReplacements.size(); i--;)
-        jumpReplacements[i].installVMTrapBreakpoint();
+
+    auto& map = pcCodeBlockMap(locker);
+#if !defined(NDEBUG)
+    // We need to be able to handle more than one invalidation point at the same pc
+    // but we want to make sure we don't forget to remove a pc from the map.
+    HashSet<void*> newReplacements;
+#endif
+    for (auto& jumpReplacement : jumpReplacements) {
+        jumpReplacement.installVMTrapBreakpoint();
+        void* source = jumpReplacement.dataLocation();
+        auto result = map.add(source, owner);
+        UNUSED_PARAM(result);
+#if !defined(NDEBUG)
+        ASSERT(result.isNewEntry || newReplacements.contains(source));
+        newReplacements.add(source);
+#endif
+    }
+}
+
+CodeBlock* codeBlockForVMTrapPC(void* pc)
+{
+    ASSERT(isJITPC(pc));
+    LockHolder locker(pcCodeBlockMapLock);
+    auto& map = pcCodeBlockMap(locker);
+    auto result = map.find(pc);
+    if (result == map.end())
+        return nullptr;
+    return result->value;
 }
 
 bool CommonData::isVMTrapBreakpoint(void* address)
index fde0747..294c11f 100644 (file)
@@ -75,6 +75,7 @@ public:
         , frameRegisterCount(std::numeric_limits<unsigned>::max())
         , requiredRegisterCountForExit(std::numeric_limits<unsigned>::max())
     { }
+    ~CommonData();
     
     void notifyCompilingStructureTransition(Plan&, CodeBlock*, Node*);
     CallSiteIndex addCodeOrigin(CodeOrigin);
@@ -86,7 +87,7 @@ public:
     
     bool invalidate(); // Returns true if we did invalidate, or false if the code block was already invalidated.
     bool hasInstalledVMTrapsBreakpoints() const { return isStillValid && hasVMTrapsBreakpointsInstalled; }
-    void installVMTrapBreakpoints();
+    void installVMTrapBreakpoints(CodeBlock* owner);
     bool isVMTrapBreakpoint(void* address);
 
     unsigned requiredRegisterCountForExecutionAndExit() const
@@ -128,6 +129,8 @@ private:
 
 };
 
+CodeBlock* codeBlockForVMTrapPC(void* pc);
+
 } } // namespace JSC::DFG
 
 #endif // ENABLE(DFG_JIT)
index a71fe76..aadc142 100644 (file)
@@ -986,7 +986,7 @@ class Workers;
 
 template<typename Func>
 int runJSC(CommandLine, bool isWorker, const Func&);
-static void checkException(GlobalObject*, bool isLastFile, bool hasException, JSValue, const String& uncaughtExceptionName, bool alwaysDumpUncaughtException, bool dump, bool& success);
+static void checkException(GlobalObject*, bool isLastFile, bool hasException, JSValue, CommandLine&, bool& success);
 
 class Message : public ThreadSafeRefCounted<Message> {
 public:
@@ -1210,6 +1210,7 @@ public:
     bool m_profile { false };
     String m_profilerOutput;
     String m_uncaughtExceptionName;
+    bool m_treatWatchdogExceptionAsSuccess { false };
     bool m_alwaysDumpUncaughtException { false };
     bool m_dumpSamplingProfilerData { false };
     bool m_enableRemoteDebugging { false };
@@ -2617,7 +2618,7 @@ EncodedJSValue JSC_HOST_CALL functionDollarAgentStart(ExecState* exec)
                     result = evaluate(globalObject->globalExec(), makeSource(sourceCode, SourceOrigin(ASCIILiteral("worker"))), JSValue(), evaluationException);
                     if (evaluationException)
                         result = evaluationException->value();
-                    checkException(globalObject, true, evaluationException, result, String(), false, false, success);
+                    checkException(globalObject, true, evaluationException, result, commandLine, success);
                     if (!success)
                         exit(1);
                     return success;
@@ -3292,8 +3293,9 @@ static void dumpException(GlobalObject* globalObject, JSValue exception)
 #undef CHECK_EXCEPTION
 }
 
-static bool checkUncaughtException(VM& vm, GlobalObject* globalObject, JSValue exception, const String& expectedExceptionName, bool alwaysDumpException)
+static bool checkUncaughtException(VM& vm, GlobalObject* globalObject, JSValue exception, CommandLine& options)
 {
+    const String& expectedExceptionName = options.m_uncaughtExceptionName;
     auto scope = DECLARE_CATCH_SCOPE(vm);
     scope.clearException();
     if (!exception) {
@@ -3314,7 +3316,7 @@ static bool checkUncaughtException(VM& vm, GlobalObject* globalObject, JSValue e
         return false;
     }
     if (isInstanceOfExpectedException) {
-        if (alwaysDumpException)
+        if (options.m_alwaysDumpUncaughtException)
             dumpException(globalObject, exception);
         return true;
     }
@@ -3324,25 +3326,32 @@ static bool checkUncaughtException(VM& vm, GlobalObject* globalObject, JSValue e
     return false;
 }
 
-static void checkException(GlobalObject* globalObject, bool isLastFile, bool hasException, JSValue value, const String& uncaughtExceptionName, bool alwaysDumpUncaughtException, bool dump, bool& success)
+static void checkException(GlobalObject* globalObject, bool isLastFile, bool hasException, JSValue value, CommandLine& options, bool& success)
 {
     VM& vm = globalObject->vm();
-    if (!uncaughtExceptionName || !isLastFile) {
+
+    if (options.m_treatWatchdogExceptionAsSuccess && value.inherits(vm, TerminatedExecutionError::info())) {
+        ASSERT(hasException);
+        return;
+    }
+
+    if (!options.m_uncaughtExceptionName || !isLastFile) {
         success = success && !hasException;
-        if (dump && !hasException)
+        if (options.m_dump && !hasException)
             printf("End: %s\n", value.toWTFString(globalObject->globalExec()).utf8().data());
         if (hasException)
             dumpException(globalObject, value);
     } else
-        success = success && checkUncaughtException(vm, globalObject, (hasException) ? value : JSValue(), uncaughtExceptionName, alwaysDumpUncaughtException);
+        success = success && checkUncaughtException(vm, globalObject, (hasException) ? value : JSValue(), options);
 }
 
-static bool runWithScripts(GlobalObject* globalObject, const Vector<Script>& scripts, const String& uncaughtExceptionName, bool alwaysDumpUncaughtException, bool dump, bool module)
+static bool runWithOptions(GlobalObject* globalObject, CommandLine& options)
 {
+    Vector<Script>& scripts = options.m_scripts;
     String fileName;
     Vector<char> scriptBuffer;
 
-    if (dump)
+    if (options.m_dump)
         JSC::Options::dumpGeneratedBytecodes() = true;
 
     VM& vm = globalObject->vm();
@@ -3355,7 +3364,7 @@ static bool runWithScripts(GlobalObject* globalObject, const Vector<Script>& scr
 
     for (size_t i = 0; i < scripts.size(); i++) {
         JSInternalPromise* promise = nullptr;
-        bool isModule = module || scripts[i].scriptType == Script::ScriptType::Module;
+        bool isModule = options.m_module || scripts[i].scriptType == Script::ScriptType::Module;
         if (scripts[i].codeSource == Script::CodeSource::File) {
             fileName = scripts[i].argument;
             if (scripts[i].strictMode == Script::StrictMode::Strict)
@@ -3382,12 +3391,12 @@ static bool runWithScripts(GlobalObject* globalObject, const Vector<Script>& scr
             scope.clearException();
 
             JSFunction* fulfillHandler = JSNativeStdFunction::create(vm, globalObject, 1, String(), [&, isLastFile](ExecState* exec) {
-                checkException(globalObject, isLastFile, false, exec->argument(0), uncaughtExceptionName, alwaysDumpUncaughtException, dump, success);
+                checkException(globalObject, isLastFile, false, exec->argument(0), options, success);
                 return JSValue::encode(jsUndefined());
             });
 
             JSFunction* rejectHandler = JSNativeStdFunction::create(vm, globalObject, 1, String(), [&, isLastFile](ExecState* exec) {
-                checkException(globalObject, isLastFile, true, exec->argument(0), uncaughtExceptionName, alwaysDumpUncaughtException, dump, success);
+                checkException(globalObject, isLastFile, true, exec->argument(0), options, success);
                 return JSValue::encode(jsUndefined());
             });
 
@@ -3400,7 +3409,7 @@ static bool runWithScripts(GlobalObject* globalObject, const Vector<Script>& scr
             scope.assertNoException();
             if (evaluationException)
                 returnValue = evaluationException->value();
-            checkException(globalObject, isLastFile, evaluationException, returnValue, uncaughtExceptionName, alwaysDumpUncaughtException, dump, success);
+            checkException(globalObject, isLastFile, evaluationException, returnValue, options, success);
         }
 
         scriptBuffer.clear();
@@ -3502,6 +3511,7 @@ static NO_RETURN void printUsageStatement(bool help = false)
     fprintf(stderr, "  --strict-file=<file>       Parse the given file as if it were in strict mode (this option may be passed more than once)\n");
     fprintf(stderr, "  --module-file=<file>       Parse and evaluate the given file as module (this option may be passed more than once)\n");
     fprintf(stderr, "  --exception=<name>         Check the last script exits with an uncaught exception with the specified name\n");
+    fprintf(stderr, "  --watchdog-exception-ok    Uncaught watchdog exceptions exit with success\n");
     fprintf(stderr, "  --dumpException            Dump uncaught exception text\n");
     fprintf(stderr, "  --options                  Dumps all JSC VM options and exits\n");
     fprintf(stderr, "  --dumpOptions              Dumps all non-default JSC VM options before continuing\n");
@@ -3631,6 +3641,11 @@ void CommandLine::parseArguments(int argc, char** argv)
             continue;
         }
 
+        if (!strcmp(arg, "--watchdog-exception-ok")) {
+            m_treatWatchdogExceptionAsSuccess = true;
+            continue;
+        }
+
         // See if the -- option is a JSC VM option.
         if (strstr(arg, "--") == arg) {
             if (!JSC::Options::setOption(&arg[2])) {
@@ -3778,7 +3793,7 @@ int jscmain(int argc, char** argv)
     result = runJSC(
         options, false,
         [&] (VM&, GlobalObject* globalObject) {
-            return runWithScripts(globalObject, options.m_scripts, options.m_uncaughtExceptionName, options.m_alwaysDumpUncaughtException, options.m_dump, options.m_module);
+            return runWithOptions(globalObject, options);
         });
 
     printSuperSamplerState();
index 31e2db4..f83512f 100644 (file)
@@ -154,10 +154,10 @@ void JSLock::didAcquireLock()
     registerThreadForMachExceptionHandling(&Thread::current());
 #endif
 
+    // Note: everything below must come after addCurrentThread().
     m_vm->traps().notifyGrabAllLocks();
 
 #if ENABLE(SAMPLING_PROFILER)
-    // Note: this must come after addCurrentThread().
     if (SamplingProfiler* samplingProfiler = m_vm->samplingProfiler())
         samplingProfiler->noticeJSLockAcquisition();
 #endif
index 44d53ea..02fa4fd 100644 (file)
@@ -72,83 +72,7 @@ inline static bool vmIsInactive(VM& vm)
     return !vm.entryScope && !vm.ownerThread();
 }
 
-struct VMAndStackBounds {
-    VM* vm;
-    StackBounds stackBounds;
-};
-
-static Expected<VMAndStackBounds, VMTraps::Error> findActiveVMAndStackBounds(SignalContext& context)
-{
-    VMInspector& inspector = VMInspector::instance();
-    auto locker = tryHoldLock(inspector.getLock());
-    if (UNLIKELY(!locker))
-        return makeUnexpected(VMTraps::Error::LockUnavailable);
-    
-    VM* activeVM = nullptr;
-    StackBounds stackBounds = StackBounds::emptyBounds();
-    void* stackPointer = context.stackPointer;
-    bool unableToAcquireMachineThreadsLock = false;
-    inspector.iterate(locker, [&] (VM& vm) {
-        if (vmIsInactive(vm))
-            return VMInspector::FunctorStatus::Continue;
-
-        auto& machineThreads = vm.heap.machineThreads();
-        auto machineThreadsLocker = tryHoldLock(machineThreads.getLock());
-        if (UNLIKELY(!machineThreadsLocker)) {
-            unableToAcquireMachineThreadsLock = true;
-            return VMInspector::FunctorStatus::Continue; // Try next VM.
-        }
-
-        const auto& threadList = machineThreads.threadsListHead(machineThreadsLocker);
-        for (MachineThreads::MachineThread* thread = threadList.head(); thread; thread = thread->next()) {
-            RELEASE_ASSERT(thread->stackBase());
-            RELEASE_ASSERT(thread->stackEnd());
-            if (stackPointer <= thread->stackBase() && stackPointer >= thread->stackEnd()) {
-                activeVM = &vm;
-                stackBounds = StackBounds(thread->stackBase(), thread->stackEnd());
-                return VMInspector::FunctorStatus::Done;
-            }
-        }
-        return VMInspector::FunctorStatus::Continue;
-    });
-
-    if (!activeVM && unableToAcquireMachineThreadsLock)
-        return makeUnexpected(VMTraps::Error::LockUnavailable);
-    return VMAndStackBounds { activeVM, stackBounds };
-}
-
-static void installSignalHandler()
-{
-    installSignalHandler(Signal::BadAccess, [] (Signal, SigInfo&, PlatformRegisters& registers) -> SignalAction {
-        SignalContext context(registers);
-
-        if (!isJITPC(context.trapPC))
-            return SignalAction::NotHandled;
-
-        // FIXME: This currently eats all traps including jit asserts we should make sure this
-        // always works. https://bugs.webkit.org/show_bug.cgi?id=171039
-        auto activeVMAndStackBounds = findActiveVMAndStackBounds(context);
-        if (!activeVMAndStackBounds)
-            return SignalAction::Handled; // Let the SignalSender try again later.
-
-        VM* vm = activeVMAndStackBounds.value().vm;
-        if (vm) {
-            VMTraps& traps = vm->traps();
-            if (!traps.needTrapHandling())
-                return SignalAction::Handled; // The polling code beat us to handling the trap already.
-
-            auto expectedSuccess = traps.tryJettisonCodeBlocksOnStack(context);
-            if (!expectedSuccess)
-                return SignalAction::Handled; // Let the SignalSender try again later.
-            if (expectedSuccess.value())
-                return SignalAction::Handled; // We've success jettison the codeBlocks.
-        }
-
-        return SignalAction::Handled;
-    });
-}
-
-ALWAYS_INLINE static CallFrame* sanitizedTopCallFrame(CallFrame* topCallFrame)
+inline CallFrame* sanitizedTopCallFrame(CallFrame* topCallFrame)
 {
 #if !defined(NDEBUG) && !CPU(ARM) && !CPU(MIPS)
     // prepareForExternalCall() in DFGSpeculativeJIT.h may set topCallFrame to a bad word
@@ -183,28 +107,17 @@ void VMTraps::tryInstallTrapBreakpoints(SignalContext& context, StackBounds stac
 
     CallFrame* callFrame = reinterpret_cast<CallFrame*>(context.framePointer);
 
-    auto codeBlockSetLocker = tryHoldLock(vm.heap.codeBlockSet().getLock());
+    auto& lock = vm.heap.codeBlockSet().getLock();
+    // If the target thread is in C++ code it might be holding the codeBlockSet lock.
+    // if it's in JIT code then it cannot be holding that lock but the GC might be.
+    auto codeBlockSetLocker = isJITPC(trapPC) ? holdLock(lock) : tryHoldLock(lock);
     if (!codeBlockSetLocker)
         return; // Let the SignalSender try again later.
 
-    {
-        auto& allocator = ExecutableAllocator::singleton();
-        auto allocatorLocker = tryHoldLock(allocator.getLock());
-        if (!allocatorLocker)
-            return; // Let the SignalSender try again later.
-
-        if (allocator.isValidExecutableMemory(allocatorLocker, trapPC)) {
-            if (vm.isExecutingInRegExpJIT) {
-                // We need to do this because a regExpJIT frame isn't a JS frame.
-                callFrame = sanitizedTopCallFrame(vm.topCallFrame);
-            }
-        } else if (LLInt::isLLIntPC(trapPC)) {
-            // The framePointer probably has the callFrame. We're good to go.
-        } else {
-            // We resort to topCallFrame to see if we can get anything
-            // useful. We usually get here when we're executing C code.
-            callFrame = sanitizedTopCallFrame(vm.topCallFrame);
-        }
+    if (!isJITPC(trapPC) && !LLInt::isLLIntPC(trapPC)) {
+        // We resort to topCallFrame to see if we can get anything
+        // useful. We usually get here when we're executing C code.
+        callFrame = sanitizedTopCallFrame(vm.topCallFrame);
     }
 
     CodeBlock* foundCodeBlock = nullptr;
@@ -240,7 +153,7 @@ void VMTraps::tryInstallTrapBreakpoints(SignalContext& context, StackBounds stac
     }
 
     if (JITCode::isOptimizingJIT(foundCodeBlock->jitType())) {
-        auto locker = tryHoldLock(m_lock);
+        auto locker = tryHoldLock(*m_lock);
         if (!locker)
             return; // Let the SignalSender try again later.
 
@@ -250,45 +163,6 @@ void VMTraps::tryInstallTrapBreakpoints(SignalContext& context, StackBounds stac
     }
 }
 
-auto VMTraps::tryJettisonCodeBlocksOnStack(SignalContext& context) -> Expected<bool, Error>
-{
-    VM& vm = this->vm();
-    auto codeBlockSetLocker = tryHoldLock(vm.heap.codeBlockSet().getLock());
-    if (!codeBlockSetLocker)
-        return makeUnexpected(Error::LockUnavailable);
-
-    CallFrame* topCallFrame = reinterpret_cast<CallFrame*>(context.framePointer);
-    void* trapPC = context.trapPC;
-    bool trapPCIsVMTrap = false;
-    
-    vm.heap.forEachCodeBlockIgnoringJITPlans(codeBlockSetLocker, [&] (CodeBlock* codeBlock) {
-        if (!codeBlock->hasInstalledVMTrapBreakpoints())
-            return false; // Not found yet.
-
-        JITCode* jitCode = codeBlock->jitCode().get();
-        ASSERT(JITCode::isOptimizingJIT(jitCode->jitType()));
-        if (jitCode->dfgCommon()->isVMTrapBreakpoint(trapPC)) {
-            trapPCIsVMTrap = true;
-            // At the codeBlock trap point, we're guaranteed that:
-            // 1. the pc is not in the middle of any range of JIT code which invalidation points
-            //    may write over. Hence, it's now safe to patch those invalidation points and
-            //    jettison the codeBlocks.
-            // 2. The top frame must be an optimized JS frame.
-            ASSERT(codeBlock == topCallFrame->codeBlock());
-            codeBlock->jettison(Profiler::JettisonDueToVMTraps);
-            return true;
-        }
-
-        return false; // Not found yet.
-    });
-
-    if (!trapPCIsVMTrap)
-        return false;
-
-    invalidateCodeBlocksOnStack(codeBlockSetLocker, topCallFrame);
-    return true;
-}
-
 void VMTraps::invalidateCodeBlocksOnStack()
 {
     invalidateCodeBlocksOnStack(vm().topCallFrame);
@@ -321,105 +195,136 @@ void VMTraps::invalidateCodeBlocksOnStack(Locker<Lock>&, ExecState* topCallFrame
     }
 }
 
-#endif // ENABLE(SIGNAL_BASED_VM_TRAPS)
-
-void VMTraps::willDestroyVM()
-{
-    m_isShuttingDown = true;
-    WTF::storeStoreFence();
-#if ENABLE(SIGNAL_BASED_VM_TRAPS)
-    while (!m_signalSenders.isEmpty()) {
-        RefPtr<SignalSender> sender;
-        {
-            // We don't want to be holding the VMTraps lock when calling
-            // SignalSender::willDestroyVM() because SignalSender::willDestroyVM()
-            // will acquire the SignalSender lock, and SignalSender::send() needs
-            // to acquire these locks in the opposite order.
-            auto locker = holdLock(m_lock);
-            sender = m_signalSenders.takeAny();
-            if (!sender)
-                break;
-        }
-        sender->willDestroyVM();
+class VMTraps::SignalSender final : public AutomaticThread {
+public:
+    using Base = AutomaticThread;
+    SignalSender(const AbstractLocker& locker, VM& vm)
+        : Base(locker, vm.traps().m_lock, vm.traps().m_trapSet)
+        , m_vm(vm)
+    {
+        static std::once_flag once;
+        std::call_once(once, [] {
+            installSignalHandler(Signal::BadAccess, [] (Signal, SigInfo&, PlatformRegisters& registers) -> SignalAction {
+                SignalContext context(registers);
+
+                if (!isJITPC(context.trapPC))
+                    return SignalAction::NotHandled;
+
+                CodeBlock* currentCodeBlock = DFG::codeBlockForVMTrapPC(context.trapPC);
+                ASSERT(currentCodeBlock->hasInstalledVMTrapBreakpoints());
+                VM& vm = *currentCodeBlock->vm();
+                ASSERT(vm.traps().needTrapHandling()); // We should have already jettisoned this code block when we handled the trap.
+
+                // We are in JIT code so it's safe to aquire this lock.
+                auto codeBlockSetLocker = holdLock(vm.heap.codeBlockSet().getLock());
+                bool sawCurrentCodeBlock = false;
+                vm.heap.forEachCodeBlockIgnoringJITPlans(codeBlockSetLocker, [&] (CodeBlock* codeBlock) {
+                    // We want to jettison all code blocks that have vm traps breakpoints, otherwise we could hit them later.
+                    if (codeBlock->hasInstalledVMTrapBreakpoints()) {
+                        if (currentCodeBlock == codeBlock)
+                            sawCurrentCodeBlock = true;
+
+                        codeBlock->jettison(Profiler::JettisonDueToVMTraps);
+                    }
+                    return false;
+                });
+                RELEASE_ASSERT(sawCurrentCodeBlock);
+                
+                return SignalAction::Handled; // We've successfully jettisoned the codeBlocks.
+            });
+        });
     }
-    ASSERT(m_signalSenders.isEmpty());
-#endif
-}
 
-#if ENABLE(SIGNAL_BASED_VM_TRAPS)
-void VMTraps::addSignalSender(VMTraps::SignalSender* sender)
-{
-    auto locker = holdLock(m_lock);
-    m_signalSenders.add(sender);
-}
+    VMTraps& traps() { return m_vm.traps(); }
 
-void VMTraps::removeSignalSender(VMTraps::SignalSender* sender)
-{
-    auto locker = holdLock(m_lock);
-    m_signalSenders.remove(sender);
-}
+protected:
+    PollResult poll(const AbstractLocker&) override
+    {
+        if (traps().m_isShuttingDown)
+            return PollResult::Stop;
 
-void VMTraps::SignalSender::willDestroyVM()
-{
-    auto locker = holdLock(m_lock);
-    m_vm = nullptr;
-}
+        if (!traps().needTrapHandling())
+            return PollResult::Wait;
 
-void VMTraps::SignalSender::send()
-{
-    static std::once_flag once;
-    std::call_once(once, [] {
-        installSignalHandler();
-    });
+        // We know that no trap could have been processed and re-added because we are holding the lock.
+        if (vmIsInactive(m_vm))
+            return PollResult::Wait;
+        return PollResult::Work;
+    }
+
+    WorkResult work() override
+    {
 
-    while (true) {
         // We need a nested scope so that we'll release the lock before we sleep below.
-        {
-            auto locker = holdLock(m_lock);
-            if (!m_vm)
-                break;
-
-            VM& vm = *m_vm;
-            auto optionalOwnerThread = vm.ownerThread();
-            if (optionalOwnerThread) {
-                sendMessage(*optionalOwnerThread.value().get(), [] (PlatformRegisters& registers) -> void {
-                    SignalContext context(registers);
-                    auto activeVMAndStackBounds = findActiveVMAndStackBounds(context);
-                    if (activeVMAndStackBounds) {
-                        VM* vm = activeVMAndStackBounds.value().vm;
-                        if (vm) {
-                            StackBounds stackBounds = activeVMAndStackBounds.value().stackBounds;
-                            VMTraps& traps = vm->traps();
-                            if (traps.needTrapHandling())
-                                traps.tryInstallTrapBreakpoints(context, stackBounds);
-                        }
+        VM& vm = m_vm;
+
+        auto optionalOwnerThread = vm.ownerThread();
+        if (optionalOwnerThread) {
+            sendMessage(*optionalOwnerThread.value().get(), [&] (PlatformRegisters& registers) -> void {
+                SignalContext context(registers);
+
+                auto ownerThread = vm.apiLock().ownerThread();
+                // We can't mess with a thread unless it's the one we suspended.
+                if (!ownerThread || ownerThread != optionalOwnerThread)
+                    return;
+
+                Thread& thread = *ownerThread->get();
+                StackBounds stackBounds = StackBounds::emptyBounds();
+                {
+                    // FIXME: We need to use the machine threads because it is the only non-TLS source
+                    // for the stack bounds of this thread. We should keep in on the WTF::Thread instead.
+                    // see: https://bugs.webkit.org/show_bug.cgi?id=173975
+                    MachineThreads& machineThreads = vm.heap.machineThreads();
+                    auto machineThreadsLock = tryHoldLock(machineThreads.getLock());
+                    if (!machineThreadsLock)
+                        return; // Try again later.
+
+                    auto& threadList = machineThreads.threadsListHead(machineThreadsLock);
+                    for (MachineThreads::MachineThread* machineThread = threadList.head(); machineThread; machineThread = machineThread->next()) {
+                        if (machineThread->m_thread.get() == thread)
+                            stackBounds = StackBounds(machineThread->stackBase(), machineThread->stackEnd());
                     }
-                });
-                break;
-            }
-
-            if (vmIsInactive(vm))
-                break;
+                    RELEASE_ASSERT(!stackBounds.isEmpty());
+                }
 
-            VMTraps::Mask mask(m_eventType);
-            if (!vm.needTrapHandling(mask))
-                break;
+                vm.traps().tryInstallTrapBreakpoints(context, stackBounds);
+            });
         }
 
         sleepMS(1);
+        return WorkResult::Continue;
     }
+    
+private:
+
+    VM& m_vm;
+};
 
-    auto locker = holdLock(m_lock);
-    if (m_vm)
-        m_vm->traps().removeSignalSender(this);
-}
 #endif // ENABLE(SIGNAL_BASED_VM_TRAPS)
 
+void VMTraps::willDestroyVM()
+{
+    m_isShuttingDown = true;
+    WTF::storeStoreFence();
+#if ENABLE(SIGNAL_BASED_VM_TRAPS)
+    if (m_signalSender) {
+        {
+            auto locker = holdLock(*m_lock);
+            if (!m_signalSender->tryStop(locker))
+                m_trapSet->notifyAll(locker);
+        }
+        if (!ASSERT_DISABLED)
+            m_signalSender->join();
+        m_signalSender = nullptr;
+    }
+#endif
+}
+
 void VMTraps::fireTrap(VMTraps::EventType eventType)
 {
     ASSERT(!vm().currentThreadIsHoldingAPILock());
     {
-        auto locker = holdLock(m_lock);
+        auto locker = holdLock(*m_lock);
         ASSERT(!m_isShuttingDown);
         setTrapForEvent(locker, eventType);
         m_needToInvalidatedCodeBlocks = true;
@@ -430,11 +335,10 @@ void VMTraps::fireTrap(VMTraps::EventType eventType)
         // sendSignal() can loop until it has confirmation that the mutator thread
         // has received the trap request. We'll call it from another trap so that
         // fireTrap() does not block.
-        RefPtr<SignalSender> sender = adoptRef(new SignalSender(vm(), eventType));
-        addSignalSender(sender.get());
-        Thread::create("jsc.vmtraps.signalling.thread", [sender] {
-            sender->send();
-        });
+        auto locker = holdLock(*m_lock);
+        if (!m_signalSender)
+            m_signalSender = adoptRef(new SignalSender(locker, vm()));
+        m_trapSet->notifyAll(locker);
     }
 #endif
 }
@@ -444,6 +348,16 @@ void VMTraps::handleTraps(ExecState* exec, VMTraps::Mask mask)
     VM& vm = this->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
+    {
+        auto codeBlockSetLocker = holdLock(vm.heap.codeBlockSet().getLock());
+        vm.heap.forEachCodeBlockIgnoringJITPlans(codeBlockSetLocker, [&] (CodeBlock* codeBlock) {
+            // We want to jettison all code blocks that have vm traps breakpoints, otherwise we could hit them later.
+            if (codeBlock->hasInstalledVMTrapBreakpoints())
+                codeBlock->jettison(Profiler::JettisonDueToVMTraps);
+            return false;
+        });
+    }
+
     ASSERT(needTrapHandling(mask));
     while (needTrapHandling(mask)) {
         auto eventType = takeTopPriorityTrap(mask);
@@ -460,7 +374,6 @@ void VMTraps::handleTraps(ExecState* exec, VMTraps::Mask mask)
             FALLTHROUGH;
 
         case NeedTermination:
-            invalidateCodeBlocksOnStack(exec);
             throwException(exec, scope, createTerminatedExecutionException(&vm));
             return;
 
@@ -472,7 +385,7 @@ void VMTraps::handleTraps(ExecState* exec, VMTraps::Mask mask)
 
 auto VMTraps::takeTopPriorityTrap(VMTraps::Mask mask) -> EventType
 {
-    auto locker = holdLock(m_lock);
+    auto locker = holdLock(*m_lock);
     for (int i = 0; i < NumberOfEventTypes; ++i) {
         EventType eventType = static_cast<EventType>(i);
         if (hasTrapForEvent(locker, eventType, mask)) {
@@ -483,4 +396,17 @@ auto VMTraps::takeTopPriorityTrap(VMTraps::Mask mask) -> EventType
     return Invalid;
 }
 
+VMTraps::VMTraps()
+    : m_lock(Box<Lock>::create())
+    , m_trapSet(AutomaticThreadCondition::create())
+{
+}
+
+VMTraps::~VMTraps()
+{
+#if ENABLE(SIGNAL_BASED_VM_TRAPS)
+    ASSERT(!m_signalSender);
+#endif
+}
+
 } // namespace JSC
index 9b72647..9b06cdd 100644 (file)
@@ -25,6 +25,8 @@
 
 #pragma once
 
+#include <wtf/AutomaticThread.h>
+#include <wtf/Box.h>
 #include <wtf/Expected.h>
 #include <wtf/HashSet.h>
 #include <wtf/Lock.h>
@@ -86,12 +88,8 @@ public:
         BitField m_mask;
     };
 
-    ~VMTraps()
-    {
-#if ENABLE(SIGNAL_BASED_VM_TRAPS)
-        ASSERT(m_signalSenders.isEmpty());
-#endif
-    }
+    ~VMTraps();
+    VMTraps();
 
     void willDestroyVM();
 
@@ -110,7 +108,6 @@ public:
     void handleTraps(ExecState*, VMTraps::Mask);
 
     void tryInstallTrapBreakpoints(struct SignalContext&, StackBounds);
-    Expected<bool, Error> tryJettisonCodeBlocksOnStack(struct SignalContext&);
 
 private:
     VM& vm() const;
@@ -134,21 +131,8 @@ private:
     EventType takeTopPriorityTrap(Mask);
 
 #if ENABLE(SIGNAL_BASED_VM_TRAPS)
-    class SignalSender : public ThreadSafeRefCounted<SignalSender> {
-    public:
-        SignalSender(VM& vm, EventType eventType)
-            : m_vm(&vm)
-            , m_eventType(eventType)
-        { }
-
-        void willDestroyVM();
-        void send();
-
-    private:
-        Lock m_lock;
-        VM* m_vm;
-        EventType m_eventType;
-    };
+    class SignalSender;
+    friend class SignalSender;
 
     void invalidateCodeBlocksOnStack();
     void invalidateCodeBlocksOnStack(ExecState* topCallFrame);
@@ -161,7 +145,8 @@ private:
     void invalidateCodeBlocksOnStack(ExecState*) { }
 #endif
 
-    Lock m_lock;
+    Box<Lock> m_lock;
+    RefPtr<AutomaticThreadCondition> m_trapSet;
     union {
         BitField m_needTrapHandling { 0 };
         BitField m_trapsBitField;
@@ -170,7 +155,7 @@ private:
     bool m_isShuttingDown { false };
 
 #if ENABLE(SIGNAL_BASED_VM_TRAPS)
-    HashSet<RefPtr<SignalSender>> m_signalSenders;
+    RefPtr<SignalSender> m_signalSender;
 #endif
 
     friend class LLIntOffsetsExtractor;
index 5e7a7fe..c6744bf 100644 (file)
@@ -1,3 +1,15 @@
+2017-06-28  Keith Miller  <keith_miller@apple.com>
+
+        VMTraps has some races
+        https://bugs.webkit.org/show_bug.cgi?id=173941
+
+        Reviewed by Michael Saboff.
+
+        Add new testing mode for testing the Watchdog with our stress
+        tests.
+
+        * Scripts/run-jsc-stress-tests:
+
 2017-06-29  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK][WPE] Implement API::IconLoadingClient and rework WebKitFaviconDatabase to use IconDatabase directly
index d142d3d..8ef2ddc 100755 (executable)
@@ -890,6 +890,11 @@ def runFTLEager(*optionalTestSpecificOptions)
     run("ftl-eager", "--airForceBriggsAllocator=true", *(FTL_OPTIONS + EAGER_OPTIONS + COLLECT_CONTINUOUSLY_OPTIONS + optionalTestSpecificOptions))
 end
 
+def runFTLEagerWatchdog(*optionalTestSpecificOptions)
+    timeout = rand(100)
+    run("ftl-eager-watchdog-#{timeout}", "--watchdog=#{timeout}", "--watchdog-exception-ok", *(FTL_OPTIONS + EAGER_OPTIONS + COLLECT_CONTINUOUSLY_OPTIONS + optionalTestSpecificOptions))
+end
+
 def runFTLEagerNoCJITValidate(*optionalTestSpecificOptions)
     run("ftl-eager-no-cjit", "--validateGraph=true", "--airForceIRCAllocator=true", *(FTL_OPTIONS + NO_CJIT_OPTIONS + EAGER_OPTIONS + COLLECT_CONTINUOUSLY_OPTIONS + optionalTestSpecificOptions))
 end