watchdog m_didFire state erroneously retained.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Aug 2015 02:49:52 +0000 (02:49 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Aug 2015 02:49:52 +0000 (02:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=131082

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

The watchdog can fire for 2 reasons:
1. an external controlling entity (i.e. another thread) has scheduled termination
   of the script thread via watchdog::terminateSoon().
2. the allowed CPU time has expired.

For case 1, we're doing away with the m_didFire flag.  Watchdog::terminateSoon()
will set the timer deadlines and m_timeLimit to 0, and m_timerDidFire to true.
This will get the script thread to check Watchdog::didFire() and terminate
execution.

Note: the watchdog only guarantees that script execution will terminate as soon
as possible due to a time limit of 0.  Once we've exited the VM, the client of the
VM is responsible from keeping a flag to prevent new script execution.

In a race condition, if terminateSoon() is called just after execution has gotten
past the client's reentry check and the client is in the process of re-entering,
the worst that can happen is that we will schedule the watchdog timer to fire
after a period of 0.  This will terminate script execution quickly, and thereafter
the client's check should be able to prevent further entry into the VM.

The correctness (i.e. has no race condition) of this type of termination relies
on the termination state being sticky.  Once the script thread is terminated this
way, the VM will continue to terminate scripts quickly until the client sets the
time limit to a non-zero value (or clears it which sets the time limit to
noTimeLimit).

For case 2, the watchdog does not alter m_timeLimit.  If the CPU deadline has
been reached, the script thread will terminate execution and exit the VM.

If the client of the VM starts new script execution, the watchdog will allow
execution for the specified m_timeLimit.  In this case, since m_timeLimit is not
0, the script gets a fresh allowance of CPU time to execute.  Hence, terminations
due to watchdog time outs are no longer sticky.

* API/JSContextRef.cpp:
(JSContextGroupSetExecutionTimeLimit):
(JSContextGroupClearExecutionTimeLimit):
* API/tests/ExecutionTimeLimitTest.cpp:
- Add test scenarios to verify that the watchdog is automatically reset by the VM
  upon throwing the TerminatedExecutionException.

(testResetAfterTimeout):
(testExecutionTimeLimit):
* JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
* JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
* JavaScriptCore.xcodeproj/project.pbxproj:
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::execute):
(JSC::Interpreter::executeCall):
(JSC::Interpreter::executeConstruct):
* jit/JITOpcodes.cpp:
(JSC::JIT::emit_op_loop_hint):
(JSC::JIT::emitSlow_op_loop_hint):
* jit/JITOperations.cpp:
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* runtime/VM.cpp:
(JSC::VM::VM):
(JSC::VM::ensureWatchdog):
* runtime/VM.h:
* runtime/VMInlines.h: Added.
(JSC::VM::shouldTriggerTermination):
* runtime/Watchdog.cpp:
(JSC::Watchdog::Watchdog):
(JSC::Watchdog::setTimeLimit):
(JSC::Watchdog::terminateSoon):
(JSC::Watchdog::didFireSlow):
(JSC::Watchdog::hasTimeLimit):
(JSC::Watchdog::enteredVM):
(JSC::Watchdog::exitedVM):
(JSC::Watchdog::startTimer):
(JSC::Watchdog::stopTimer):
(JSC::Watchdog::hasStartedTimer): Deleted.
(JSC::Watchdog::fire): Deleted.
* runtime/Watchdog.h:
(JSC::Watchdog::didFire):
(JSC::Watchdog::timerDidFireAddress):

Source/WebCore:

No new tests.  The new code is covered by the JSC API tests and an existing test:
fast/workers/worker-terminate-forever.html

* bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::handleEvent):
* bindings/js/WorkerScriptController.cpp:
(WebCore::WorkerScriptController::WorkerScriptController):
- Always create a watchdog for the Web Worker's VM.  We need this in order to support
  Worker.terminate().
(WebCore::WorkerScriptController::evaluate):
(WebCore::WorkerScriptController::scheduleExecutionTermination):
(WebCore::WorkerScriptController::isTerminatingExecution):
(WebCore::WorkerScriptController::forbidExecution):
(WebCore::WorkerScriptController::isExecutionTerminating): Deleted.
* bindings/js/WorkerScriptController.h:

LayoutTests:

* fast/workers/worker-terminate-forever-expected.txt:
* fast/workers/worker-terminate-forever.html:
- Updated to check if the worker actually did terminate.

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

23 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/workers/worker-terminate-forever-expected.txt
LayoutTests/fast/workers/worker-terminate-forever.html
Source/JavaScriptCore/API/JSContextRef.cpp
Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj
Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters
Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/interpreter/Interpreter.cpp
Source/JavaScriptCore/jit/JITOpcodes.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h
Source/JavaScriptCore/runtime/VMInlines.h [new file with mode: 0644]
Source/JavaScriptCore/runtime/Watchdog.cpp
Source/JavaScriptCore/runtime/Watchdog.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSEventListener.cpp
Source/WebCore/bindings/js/WorkerScriptController.cpp
Source/WebCore/bindings/js/WorkerScriptController.h

index 627c88c..a188313 100644 (file)
@@ -1,3 +1,14 @@
+2015-08-26  Mark Lam  <mark.lam@apple.com>
+
+        watchdog m_didFire state erroneously retained.
+        https://bugs.webkit.org/show_bug.cgi?id=131082
+
+        Reviewed by Geoffrey Garen.
+
+        * fast/workers/worker-terminate-forever-expected.txt:
+        * fast/workers/worker-terminate-forever.html:
+        - Updated to check if the worker actually did terminate. 
+
 2015-08-26  Andy Estes  <aestes@apple.com>
 
         REGRESSION (r188987): imported/mozilla/svg/filters/feConvolveMatrix-1.svg fails
index be53ddf..87f12ce 100644 (file)
@@ -1 +1,3 @@
+CONSOLE MESSAGE: line 15: Worker was started
+CONSOLE MESSAGE: line 34: Worker was terminated
 Test Worker.terminate() for a worker that tries to run forever.
index e3ca759..8be12e6 100644 (file)
@@ -1,11 +1,52 @@
 <body>
 <p>Test Worker.terminate() for a worker that tries to run forever.</p>
 <script>
-if (window.testRunner)
+if (window.testRunner) {
     testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
 
 var worker = new Worker('resources/worker-run-forever.js');
-worker.terminate();
+
+function waitForWorkerToStart() {
+    var startTime = Date.now();
+    function checkIfWorkerStarted() {
+        if (internals.workerThreadCount == 1) {
+            console.log("Worker was started");
+            worker.terminate();
+            setTimeout(waitForWorkerToStop, 0);
+
+        } else if (Date.now() - startTime < 5000) {
+            setTimeout(checkIfWorkerStarted, 0);
+
+        } else {
+            console.log("Worker did not show up");
+            testRunner.notifyDone();
+        }            
+    }
+    setTimeout(checkIfWorkerStarted, 0);
+}
+
+function waitForWorkerToStop() {
+    var startTime = Date.now();
+    function checkIfWorkerStopped() {
+        if (internals.workerThreadCount == 0) {
+            console.log("Worker was terminated");
+            testRunner.notifyDone();
+
+        } else if (Date.now() - startTime < 5000) {
+            setTimeout(checkIfWorkerStopped, 0);
+
+        } else {
+            console.log("Did not see worker terminate");
+            testRunner.notifyDone();
+        }            
+    }
+    setTimeout(checkIfWorkerStopped, 0);
+}
+
+window.setTimeout(waitForWorkerToStart, 0);
+
 </script>
 </body>
 </html>
index 4a8f0be..886754a 100644 (file)
@@ -99,9 +99,9 @@ void JSContextGroupSetExecutionTimeLimit(JSContextGroupRef group, double limit,
     Watchdog& watchdog = vm.ensureWatchdog();
     if (callback) {
         void* callbackPtr = reinterpret_cast<void*>(callback);
-        watchdog.setTimeLimit(vm, std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(limit)), internalScriptTimeoutCallback, callbackPtr, callbackData);
+        watchdog.setTimeLimit(std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(limit)), internalScriptTimeoutCallback, callbackPtr, callbackData);
     } else
-        watchdog.setTimeLimit(vm, std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(limit)));
+        watchdog.setTimeLimit(std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(limit)));
 }
 
 void JSContextGroupClearExecutionTimeLimit(JSContextGroupRef group)
@@ -109,7 +109,7 @@ void JSContextGroupClearExecutionTimeLimit(JSContextGroupRef group)
     VM& vm = *toJS(group);
     JSLockHolder locker(&vm);
     if (vm.watchdog)
-        vm.watchdog->setTimeLimit(vm, Watchdog::noTimeLimit);
+        vm.watchdog->setTimeLimit(Watchdog::noTimeLimit);
 }
 
 // From the API's perspective, a global context remains alive iff it has been JSGlobalContextRetained.
index 1451711..4ecac8e 100644 (file)
@@ -83,6 +83,22 @@ struct TierOptions {
     const char* optionsStr;
 };
 
+static void testResetAfterTimeout(bool& failed)
+{
+    JSValueRef v = nullptr;
+    JSValueRef exception = nullptr;
+    const char* reentryScript = "100";
+    JSStringRef script = JSStringCreateWithUTF8CString(reentryScript);
+    v = JSEvaluateScript(context, script, nullptr, nullptr, 1, &exception);
+    if (exception) {
+        printf("FAIL: Watchdog timeout was not reset.\n");
+        failed = true;
+    } else if (!JSValueIsNumber(context, v) || JSValueToNumber(context, v, nullptr) != 100) {
+        printf("FAIL: Script result is not as expected.\n");
+        failed = true;
+    }
+}
+
 int testExecutionTimeLimit()
 {
     static const TierOptions tierOptionsList[] = {
@@ -152,6 +168,8 @@ int testExecutionTimeLimit()
                 printf("FAIL: %s TerminatedExecutionException was not thrown.\n", tierOptions.tier);
                 failed = true;
             }
+
+            testResetAfterTimeout(failed);
         }
 
         /* Test the script timeout's TerminatedExecutionException should NOT be catchable: */
@@ -187,6 +205,8 @@ int testExecutionTimeLimit()
                 printf("FAIL: %s TerminatedExecutionException was caught.\n", tierOptions.tier);
                 failed = true;
             }
+
+            testResetAfterTimeout(failed);
         }
         
         /* Test script timeout with no callback: */
@@ -222,6 +242,8 @@ int testExecutionTimeLimit()
                 printf("FAIL: %s TerminatedExecutionException was not thrown.\n", tierOptions.tier);
                 failed = true;
             }
+
+            testResetAfterTimeout(failed);
         }
         
         /* Test script timeout cancellation: */
index 6572585..7c3a41d 100644 (file)
@@ -1,3 +1,90 @@
+2015-08-26  Mark Lam  <mark.lam@apple.com>
+
+        watchdog m_didFire state erroneously retained.
+        https://bugs.webkit.org/show_bug.cgi?id=131082
+
+        Reviewed by Geoffrey Garen.
+
+        The watchdog can fire for 2 reasons:
+        1. an external controlling entity (i.e. another thread) has scheduled termination
+           of the script thread via watchdog::terminateSoon().
+        2. the allowed CPU time has expired.
+
+        For case 1, we're doing away with the m_didFire flag.  Watchdog::terminateSoon() 
+        will set the timer deadlines and m_timeLimit to 0, and m_timerDidFire to true.
+        This will get the script thread to check Watchdog::didFire() and terminate
+        execution.
+
+        Note: the watchdog only guarantees that script execution will terminate as soon
+        as possible due to a time limit of 0.  Once we've exited the VM, the client of the
+        VM is responsible from keeping a flag to prevent new script execution.
+
+        In a race condition, if terminateSoon() is called just after execution has gotten
+        past the client's reentry check and the client is in the process of re-entering,
+        the worst that can happen is that we will schedule the watchdog timer to fire
+        after a period of 0.  This will terminate script execution quickly, and thereafter
+        the client's check should be able to prevent further entry into the VM.
+
+        The correctness (i.e. has no race condition) of this type of termination relies
+        on the termination state being sticky.  Once the script thread is terminated this
+        way, the VM will continue to terminate scripts quickly until the client sets the
+        time limit to a non-zero value (or clears it which sets the time limit to
+        noTimeLimit).
+
+        For case 2, the watchdog does not alter m_timeLimit.  If the CPU deadline has
+        been reached, the script thread will terminate execution and exit the VM.
+
+        If the client of the VM starts new script execution, the watchdog will allow
+        execution for the specified m_timeLimit.  In this case, since m_timeLimit is not
+        0, the script gets a fresh allowance of CPU time to execute.  Hence, terminations
+        due to watchdog time outs are no longer sticky.
+
+        * API/JSContextRef.cpp:
+        (JSContextGroupSetExecutionTimeLimit):
+        (JSContextGroupClearExecutionTimeLimit):
+        * API/tests/ExecutionTimeLimitTest.cpp:
+        - Add test scenarios to verify that the watchdog is automatically reset by the VM
+          upon throwing the TerminatedExecutionException.
+
+        (testResetAfterTimeout):
+        (testExecutionTimeLimit):
+        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
+        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
+        * JavaScriptCore.xcodeproj/project.pbxproj:
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::execute):
+        (JSC::Interpreter::executeCall):
+        (JSC::Interpreter::executeConstruct):
+        * jit/JITOpcodes.cpp:
+        (JSC::JIT::emit_op_loop_hint):
+        (JSC::JIT::emitSlow_op_loop_hint):
+        * jit/JITOperations.cpp:
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        (JSC::VM::ensureWatchdog):
+        * runtime/VM.h:
+        * runtime/VMInlines.h: Added.
+        (JSC::VM::shouldTriggerTermination):
+        * runtime/Watchdog.cpp:
+        (JSC::Watchdog::Watchdog):
+        (JSC::Watchdog::setTimeLimit):
+        (JSC::Watchdog::terminateSoon):
+        (JSC::Watchdog::didFireSlow):
+        (JSC::Watchdog::hasTimeLimit):
+        (JSC::Watchdog::enteredVM):
+        (JSC::Watchdog::exitedVM):
+        (JSC::Watchdog::startTimer):
+        (JSC::Watchdog::stopTimer):
+        (JSC::Watchdog::hasStartedTimer): Deleted.
+        (JSC::Watchdog::fire): Deleted.
+        * runtime/Watchdog.h:
+        (JSC::Watchdog::didFire):
+        (JSC::Watchdog::timerDidFireAddress):
+
 2015-08-26  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Implement tracking of active stylesheets in the frontend
index 5d45621..70ccf4f 100644 (file)
     <ClInclude Include="..\runtime\Uint32Array.h" />
     <ClInclude Include="..\runtime\Uint8Array.h" />
     <ClInclude Include="..\runtime\VM.h" />
+    <ClInclude Include="..\runtime\VMInlines.h" />
     <ClInclude Include="..\runtime\VMEntryScope.h" />
     <ClInclude Include="..\runtime\VarOffset.h" />
     <ClInclude Include="..\runtime\Watchdog.h" />
index 72ab81c..cd62f92 100644 (file)
     <ClInclude Include="..\runtime\VM.h">
       <Filter>runtime</Filter>
     </ClInclude>
+    <ClInclude Include="..\runtime\VMInlines.h">
+      <Filter>runtime</Filter>
+    </ClInclude>
     <ClInclude Include="..\runtime\VMEntryScope.h">
       <Filter>runtime</Filter>
     </ClInclude>
index 4a40df2..eb62ca5 100644 (file)
                FE5932A6183C5A2600A1ECCC /* VMEntryScope.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = VMEntryScope.h; sourceTree = "<group>"; };
                FE7BA60D1A1A7CEC00F1F7B4 /* HeapVerifier.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = HeapVerifier.cpp; sourceTree = "<group>"; };
                FE7BA60E1A1A7CEC00F1F7B4 /* HeapVerifier.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = HeapVerifier.h; sourceTree = "<group>"; };
+               FE90BB3A1B7CF64E006B3F03 /* VMInlines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = VMInlines.h; sourceTree = "<group>"; };
                FEA0861E182B7A0400F6D851 /* Breakpoint.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = Breakpoint.h; sourceTree = "<group>"; };
                FEA0861F182B7A0400F6D851 /* DebuggerPrimitives.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DebuggerPrimitives.h; sourceTree = "<group>"; };
                FEB51F6A1A97B688001F921C /* Regress141809.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = Regress141809.h; path = API/tests/Regress141809.h; sourceTree = "<group>"; };
                                0FE050241AA9095600D33B33 /* VarOffset.h */,
                                E18E3A570DF9278C00D90B34 /* VM.cpp */,
                                E18E3A560DF9278C00D90B34 /* VM.h */,
+                               FE90BB3A1B7CF64E006B3F03 /* VMInlines.h */,
                                FE5932A5183C5A2600A1ECCC /* VMEntryScope.cpp */,
                                FE5932A6183C5A2600A1ECCC /* VMEntryScope.h */,
                                FED94F2B171E3E2300BE77A4 /* Watchdog.cpp */,
index cbba9c8..0a1624e 100644 (file)
@@ -4107,7 +4107,7 @@ bool ByteCodeParser::parseBlock(unsigned limit)
 
             addToGraph(LoopHint);
             
-            if (m_vm->watchdog && m_vm->watchdog->hasTimeLimit())
+            if (m_vm->watchdog)
                 addToGraph(CheckWatchdogTimer);
             
             NEXT_OPCODE(op_loop_hint);
index cc34a66..10501c1 100644 (file)
@@ -71,8 +71,8 @@
 #include "StrongInlines.h"
 #include "Symbol.h"
 #include "VMEntryScope.h"
+#include "VMInlines.h"
 #include "VirtualRegister.h"
-#include "Watchdog.h"
 
 #include <limits.h>
 #include <stdio.h>
@@ -865,7 +865,7 @@ failedJSONP:
 
     ProgramCodeBlock* codeBlock = program->codeBlock();
 
-    if (UNLIKELY(vm.watchdog && vm.watchdog->didFire(callFrame)))
+    if (UNLIKELY(vm.shouldTriggerTermination(callFrame)))
         return throwTerminatedExecutionException(callFrame);
 
     ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
@@ -928,7 +928,7 @@ JSValue Interpreter::executeCall(CallFrame* callFrame, JSObject* function, CallT
     } else
         newCodeBlock = 0;
 
-    if (UNLIKELY(vm.watchdog && vm.watchdog->didFire(callFrame)))
+    if (UNLIKELY(vm.shouldTriggerTermination(callFrame)))
         return throwTerminatedExecutionException(callFrame);
 
     ProtoCallFrame protoCallFrame;
@@ -998,7 +998,7 @@ JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject* construc
     } else
         newCodeBlock = 0;
 
-    if (UNLIKELY(vm.watchdog && vm.watchdog->didFire(callFrame)))
+    if (UNLIKELY(vm.shouldTriggerTermination(callFrame)))
         return throwTerminatedExecutionException(callFrame);
 
     ProtoCallFrame protoCallFrame;
@@ -1071,7 +1071,7 @@ JSValue Interpreter::execute(CallFrameClosure& closure)
     if (LegacyProfiler* profiler = vm.enabledProfiler())
         profiler->willExecute(closure.oldCallFrame, closure.function);
 
-    if (UNLIKELY(vm.watchdog && vm.watchdog->didFire(closure.oldCallFrame)))
+    if (UNLIKELY(vm.shouldTriggerTermination(closure.oldCallFrame)))
         return throwTerminatedExecutionException(closure.oldCallFrame);
 
     // Execute the code:
@@ -1152,7 +1152,7 @@ JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue
         }
     }
 
-    if (UNLIKELY(vm.watchdog && vm.watchdog->didFire(callFrame)))
+    if (UNLIKELY(vm.shouldTriggerTermination(callFrame)))
         return throwTerminatedExecutionException(callFrame);
 
     ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
index c83bcc8..f30fd02 100644 (file)
@@ -915,7 +915,7 @@ void JIT::emit_op_loop_hint(Instruction*)
     }
 
     // Emit the watchdog timer check:
-    if (m_vm->watchdog && m_vm->watchdog->hasTimeLimit())
+    if (m_vm->watchdog)
         addSlowCase(branchTest8(NonZero, AbsoluteAddress(m_vm->watchdog->timerDidFireAddress())));
 }
 
@@ -941,7 +941,7 @@ void JIT::emitSlow_op_loop_hint(Instruction*, Vector<SlowCaseEntry>::iterator& i
 #endif
 
     // Emit the slow path of the watchdog timer check:
-    if (m_vm->watchdog && m_vm->watchdog->hasTimeLimit()) {
+    if (m_vm->watchdog) {
         linkSlowCase(iter);
         callOperation(operationHandleWatchdogTimer);
 
index b7a3868..a1f7656 100644 (file)
@@ -58,7 +58,7 @@
 #include "ScopedArguments.h"
 #include "TestRunnerUtils.h"
 #include "TypeProfilerLog.h"
-#include "Watchdog.h"
+#include "VMInlines.h"
 #include <wtf/InlineASM.h>
 
 namespace JSC {
@@ -1094,7 +1094,7 @@ UnusedPtr JIT_OPERATION operationHandleWatchdogTimer(ExecState* exec)
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    if (UNLIKELY(vm.watchdog && vm.watchdog->didFire(exec)))
+    if (UNLIKELY(vm.shouldTriggerTermination(exec)))
         vm.throwException(exec, createTerminatedExecutionException(&vm));
 
     return nullptr;
index 663489b..2a24883 100644 (file)
@@ -54,7 +54,7 @@
 #include "ObjectConstructor.h"
 #include "ProtoCallFrame.h"
 #include "StructureRareDataInlines.h"
-#include "Watchdog.h"
+#include "VMInlines.h"
 #include <wtf/StringPrintStream.h>
 
 namespace JSC { namespace LLInt {
@@ -1303,7 +1303,7 @@ LLINT_SLOW_PATH_DECL(slow_path_handle_watchdog_timer)
 {
     LLINT_BEGIN_NO_SET_PC();
     ASSERT(vm.watchdog);
-    if (UNLIKELY(vm.watchdog->didFire(exec)))
+    if (UNLIKELY(vm.shouldTriggerTermination(exec)))
         LLINT_THROW(createTerminatedExecutionException(&vm));
     LLINT_RETURN_TWO(0, exec);
 }
index 93c96fb..5af6caf 100644 (file)
@@ -292,7 +292,7 @@ VM::VM(VMType vmType, HeapType heapType)
     if (Options::watchdog()) {
         std::chrono::milliseconds timeoutMillis(Options::watchdog());
         Watchdog& watchdog = ensureWatchdog();
-        watchdog.setTimeLimit(*this, timeoutMillis);
+        watchdog.setTimeLimit(timeoutMillis);
     }
 }
 
@@ -388,6 +388,11 @@ Watchdog& VM::ensureWatchdog()
         // the LLINT assumes that the internal shape of a std::unique_ptr is the
         // same as a plain C++ pointer, and loads the address of Watchdog from it.
         RELEASE_ASSERT(*reinterpret_cast<Watchdog**>(&watchdog) == watchdog.get());
+
+        // And if we've previously compiled any functions, we need to revert
+        // them because they don't have the needed polling checks for the watchdog
+        // yet.
+        deleteAllCode();
     }
     return *watchdog;
 }
index 9afe9a5..bf925b5 100644 (file)
@@ -236,7 +236,7 @@ public:
     static Ref<VM> createContextGroup(HeapType = SmallHeap);
     JS_EXPORT_PRIVATE ~VM();
 
-    Watchdog& ensureWatchdog();
+    JS_EXPORT_PRIVATE Watchdog& ensureWatchdog();
 
 private:
     RefPtr<JSLock> m_apiLock;
@@ -565,6 +565,8 @@ public:
     JS_EXPORT_PRIVATE void queueMicrotask(JSGlobalObject*, PassRefPtr<Microtask>);
     JS_EXPORT_PRIVATE void drainMicrotasks();
 
+    inline bool shouldTriggerTermination(ExecState*);
+
 private:
     friend class LLIntOffsetsExtractor;
     friend class ClearExceptionScope;
diff --git a/Source/JavaScriptCore/runtime/VMInlines.h b/Source/JavaScriptCore/runtime/VMInlines.h
new file mode 100644 (file)
index 0000000..fcc9388
--- /dev/null
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2015 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef VMInlines_h
+#define VMInlines_h
+
+#include "VM.h"
+#include "Watchdog.h"
+
+namespace JSC {
+    
+bool VM::shouldTriggerTermination(ExecState* exec)
+{
+    if (!watchdog)
+        return false;
+    return watchdog->didFire(exec);
+}
+
+} // namespace JSC
+
+#endif // LLIntData_h
+
index 6e0a020..2d9c882 100644 (file)
@@ -42,7 +42,6 @@ static std::chrono::microseconds currentWallClockTime()
 
 Watchdog::Watchdog()
     : m_timerDidFire(false)
-    , m_didFire(false)
     , m_timeLimit(noTimeLimit)
     , m_cpuDeadline(noTimeLimit)
     , m_wallClockDeadline(noTimeLimit)
@@ -52,126 +51,71 @@ Watchdog::Watchdog()
     , m_timerQueue(WorkQueue::create("jsc.watchdog.queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility))
 {
     m_timerHandler = [this] {
+        LockHolder locker(m_lock);
         this->m_timerDidFire = true;
         this->deref();
     };
 }
 
-inline bool Watchdog::hasStartedTimer()
-{
-    return m_cpuDeadline != noTimeLimit;
-}
-
-void Watchdog::setTimeLimit(VM& vm, std::chrono::microseconds limit,
+void Watchdog::setTimeLimit(std::chrono::microseconds limit,
     ShouldTerminateCallback callback, void* data1, void* data2)
 {
-    bool hadTimeLimit = hasTimeLimit();
-
-    m_didFire = false; // Reset the watchdog.
+    LockHolder locker(m_lock);
 
     m_timeLimit = limit;
     m_callback = callback;
     m_callbackData1 = data1;
     m_callbackData2 = data2;
 
-    // If this is the first time that time limit is being enabled, then any
-    // previously JIT compiled code will not have the needed polling checks.
-    // Hence, we need to flush all the pre-existing compiled code.
-    //
-    // However, if the time limit is already enabled, and we're just changing the
-    // time limit value, then any existing JITted code will have the appropriate
-    // polling checks. Hence, there is no need to re-do this flushing.
-    if (!hadTimeLimit) {
-        // And if we've previously compiled any functions, we need to revert
-        // them because they don't have the needed polling checks yet.
-        vm.deleteAllCode();
-    }
-
     if (m_hasEnteredVM && hasTimeLimit())
-        startTimer(m_timeLimit);
+        startTimer(locker, m_timeLimit);
 }
 
-bool Watchdog::didFireSlow(ExecState* exec)
+JS_EXPORT_PRIVATE void Watchdog::terminateSoon()
 {
-    ASSERT(m_timerDidFire);
-    m_timerDidFire = false;
+    LockHolder locker(m_lock);
 
-    // A legitimate timer is the timer which woke up watchdog and can caused didFireSlow() to be
-    // called after m_wallClockDeadline has expired. All other timers are considered to be stale,
-    // and their wake ups are considered to be spurious and should be ignored.
-    //
-    // didFireSlow() will race against m_timerHandler to clear and set m_timerDidFire respectively.
-    // We need to deal with a variety of scenarios where we can have stale timers and legitimate
-    // timers firing in close proximity to each other.
-    //
-    // Consider the following scenarios:
-    //
-    // 1. A stale timer fires long before m_wallClockDeadline expires.
-    //
-    //    In this case, the m_wallClockDeadline check below will fail and the stale timer will be
-    //    ignored as spurious. We just need to make sure that we clear m_timerDidFire before we
-    //    check m_wallClockDeadline and return below.
-    //
-    // 2. A stale timer fires just before m_wallClockDeadline expires.
-    //    Before the watchdog can gets to the clearing m_timerDidFire above, the legit timer also fires.
-    //
-    //    In this case, m_timerDidFire was set twice by the 2 timers, but we only clear need to clear
-    //    it once. The m_wallClockDeadline below will pass and this invocation of didFireSlow() will
-    //    be treated as the response to the legit timer. The spurious timer is effectively ignored.
-    //
-    // 3. A state timer fires just before m_wallClockDeadline expires.
-    //    Right after the watchdog clears m_timerDidFire above, the legit timer also fires.
-    //
-    //    The fact that the legit timer fails means that the m_wallClockDeadline check will pass.
-    //    As long as we clear m_timerDidFire before we do the check, we are safe. This is the same
-    //    scenario as 2 above.
-    //
-    // 4. A stale timer fires just before m_wallClockDeadline expires.
-    //    Right after we do the m_wallClockDeadline check below, the legit timer fires.
-    //
-    //    The fact that the legit timer fires after the m_wallClockDeadline check means that
-    //    the m_wallClockDeadline check will have failed. This is the same scenario as 1 above.
-    //
-    // 5. A legit timer fires.
-    //    A stale timer also fires before we clear m_timerDidFire above.
-    //
-    //    This is the same scenario as 2 above.
-    //
-    // 6. A legit timer fires.
-    //    A stale timer fires right after we clear m_timerDidFire above.
-    //
-    //    In this case, the m_wallClockDeadline check will pass, and we fire the watchdog
-    //    though m_timerDidFire remains set. This just means that didFireSlow() will be called again due
-    //    to m_timerDidFire being set. The subsequent invocation of didFireSlow() will end up handling
-    //    the stale timer and ignoring it. This is the same scenario as 1 above.
-    //
-    // 7. A legit timer fires.
-    //    A stale timer fires right after we do the m_wallClockDeadline check.
-    //
-    //    This is the same as 6, which means it's the same as 1 above.
-    //
-    // In all these cases, we just need to ensure that we clear m_timerDidFire before we do the
-    // m_wallClockDeadline check below. Hence, a storeLoad fence is needed to ensure that these 2
-    // operations do not get re-ordered.
+    m_timeLimit = std::chrono::microseconds(0);
+    m_cpuDeadline = std::chrono::microseconds(0);
+    m_wallClockDeadline = std::chrono::microseconds(0);
+    m_timerDidFire = true;
+}
 
-    WTF::storeLoadFence();
+bool Watchdog::didFireSlow(ExecState* exec)
+{
+    {
+        LockHolder locker(m_lock);
 
-    if (currentWallClockTime() < m_wallClockDeadline)
-        return false; // Just a stale timer firing. Nothing to do.
+        ASSERT(m_timerDidFire);
+        m_timerDidFire = false;
 
-    m_wallClockDeadline = noTimeLimit;
+        if (currentWallClockTime() < m_wallClockDeadline)
+            return false; // Just a stale timer firing. Nothing to do.
 
-    if (currentCPUTime() >= m_cpuDeadline) {
-        // Case 1: the allowed CPU time has elapsed.
+        // Set m_wallClockDeadline to noTimeLimit here so that we can reject all future
+        // spurious wakes.
+        m_wallClockDeadline = noTimeLimit;
 
-        // If m_callback is not set, then we terminate by default.
-        // Else, we let m_callback decide if we should terminate or not.
-        bool needsTermination = !m_callback
-            || m_callback(exec, m_callbackData1, m_callbackData2);
-        if (needsTermination) {
-            m_didFire = true;
-            return true;
+        auto cpuTime = currentCPUTime();
+        if (cpuTime < m_cpuDeadline) {
+            auto remainingCPUTime = m_cpuDeadline - cpuTime;
+            startTimer(locker, remainingCPUTime);
+            return false;
         }
+    }
+
+    // Note: we should not be holding the lock while calling the callbacks. The callbacks may
+    // call setTimeLimit() which will try to lock as well.
+
+    // If m_callback is not set, then we terminate by default.
+    // Else, we let m_callback decide if we should terminate or not.
+    bool needsTermination = !m_callback
+        || m_callback(exec, m_callbackData1, m_callbackData2);
+    if (needsTermination)
+        return true;
+
+    {
+        LockHolder locker(m_lock);
 
         // If we get here, then the callback above did not want to terminate execution. As a
         // result, the callback may have done one of the following:
@@ -184,15 +128,10 @@ bool Watchdog::didFireSlow(ExecState* exec)
         // In the case of 3, we need to re-start the timer here.
 
         ASSERT(m_hasEnteredVM);
-        if (hasTimeLimit() && !hasStartedTimer())
-            startTimer(m_timeLimit);
-
-    } else {
-        // Case 2: the allowed CPU time has NOT elapsed.
-        auto remainingCPUTime = m_cpuDeadline - currentCPUTime();
-        startTimer(remainingCPUTime);
+        bool callbackAlreadyStartedTimer = (m_cpuDeadline != noTimeLimit);
+        if (hasTimeLimit() && !callbackAlreadyStartedTimer)
+            startTimer(locker, m_timeLimit);
     }
-
     return false;
 }
 
@@ -201,54 +140,45 @@ bool Watchdog::hasTimeLimit()
     return (m_timeLimit != noTimeLimit);
 }
 
-void Watchdog::fire()
-{
-    m_didFire = true;
-}
-
 void Watchdog::enteredVM()
 {
     m_hasEnteredVM = true;
-    if (hasTimeLimit())
-        startTimer(m_timeLimit);
+    if (hasTimeLimit()) {
+        LockHolder locker(m_lock);
+        startTimer(locker, m_timeLimit);
+    }
 }
 
 void Watchdog::exitedVM()
 {
     ASSERT(m_hasEnteredVM);
-    stopTimer();
+    LockHolder locker(m_lock);
+    stopTimer(locker);
     m_hasEnteredVM = false;
 }
 
-void Watchdog::startTimer(std::chrono::microseconds timeLimit)
+void Watchdog::startTimer(LockHolder&, std::chrono::microseconds timeLimit)
 {
     ASSERT(m_hasEnteredVM);
     ASSERT(hasTimeLimit());
+    ASSERT(timeLimit <= m_timeLimit);
 
     m_cpuDeadline = currentCPUTime() + timeLimit;
     auto wallClockTime = currentWallClockTime();
     auto wallClockDeadline = wallClockTime + timeLimit;
 
     if ((wallClockTime < m_wallClockDeadline)
-        && (m_wallClockDeadline <= wallClockDeadline)) {
+        && (m_wallClockDeadline <= wallClockDeadline))
         return; // Wait for the current active timer to expire before starting a new one.
-    }
 
     // Else, the current active timer won't fire soon enough. So, start a new timer.
     this->ref(); // m_timerHandler will deref to match later.
     m_wallClockDeadline = wallClockDeadline;
-    m_timerDidFire = false;
-
-    // We clear m_timerDidFire because we're starting a new timer. However, we need to make sure
-    // that the clearing occurs before the timer thread is started. Thereafter, only didFireSlow()
-    // should clear m_timerDidFire (unless we start yet another timer). Hence, we need a storeStore
-    // fence here to ensure these operations do not get re-ordered.
-    WTF::storeStoreFence();
 
     m_timerQueue->dispatchAfter(std::chrono::nanoseconds(timeLimit), m_timerHandler);
 }
 
-void Watchdog::stopTimer()
+void Watchdog::stopTimer(LockHolder&)
 {
     m_cpuDeadline = noTimeLimit;
 }
index 28b9d69..cfd9fb1 100644 (file)
@@ -26,7 +26,7 @@
 #ifndef Watchdog_h
 #define Watchdog_h
 
-#include <mutex>
+#include <wtf/Lock.h>
 #include <wtf/Ref.h>
 #include <wtf/ThreadSafeRefCounted.h>
 #include <wtf/WorkQueue.h>
@@ -44,14 +44,11 @@ public:
     Watchdog();
 
     typedef bool (*ShouldTerminateCallback)(ExecState*, void* data1, void* data2);
-    void setTimeLimit(VM&, std::chrono::microseconds limit, ShouldTerminateCallback = 0, void* data1 = 0, void* data2 = 0);
+    void setTimeLimit(std::chrono::microseconds limit, ShouldTerminateCallback = 0, void* data1 = 0, void* data2 = 0);
+    JS_EXPORT_PRIVATE void terminateSoon();
 
-    // This version of didFire() will check the elapsed CPU time and call the
-    // callback (if needed) to determine if the watchdog should fire.
     bool didFire(ExecState* exec)
     {
-        if (m_didFire)
-            return true;
         if (!m_timerDidFire)
             return false;
         return didFireSlow(exec);
@@ -61,21 +58,13 @@ public:
     void enteredVM();
     void exitedVM();
 
-    // This version of didFire() is a more efficient version for when we want
-    // to know if the watchdog has fired in the past, and not whether it should
-    // fire right now.
-    bool didFire() { return m_didFire; }
-    JS_EXPORT_PRIVATE void fire();
-
     void* timerDidFireAddress() { return &m_timerDidFire; }
 
     static const std::chrono::microseconds noTimeLimit;
 
 private:
-    void startTimer(std::chrono::microseconds timeLimit);
-    void stopTimer();
-
-    inline bool hasStartedTimer();
+    void startTimer(LockHolder&, std::chrono::microseconds timeLimit);
+    void stopTimer(LockHolder&);
 
     bool didFireSlow(ExecState*);
 
@@ -85,13 +74,16 @@ private:
     // NOTE: m_timerDidFire is only set by the platform specific timer
     // (probably from another thread) but is only cleared in the script thread.
     bool m_timerDidFire;
-    bool m_didFire;
 
     std::chrono::microseconds m_timeLimit;
 
     std::chrono::microseconds m_cpuDeadline;
     std::chrono::microseconds m_wallClockDeadline;
 
+    // Writes to m_timerDidFire and m_timeLimit, and Reads+Writes to m_cpuDeadline and m_wallClockDeadline
+    // must be guarded by this lock.
+    Lock m_lock;
+
     bool m_hasEnteredVM { false };
 
     ShouldTerminateCallback m_callback;
index 35392bc..71cce99 100644 (file)
@@ -1,3 +1,26 @@
+2015-08-26  Mark Lam  <mark.lam@apple.com>
+
+        watchdog m_didFire state erroneously retained.
+        https://bugs.webkit.org/show_bug.cgi?id=131082
+
+        Reviewed by Geoffrey Garen.
+
+        No new tests.  The new code is covered by the JSC API tests and an existing test:
+        fast/workers/worker-terminate-forever.html
+
+        * bindings/js/JSEventListener.cpp:
+        (WebCore::JSEventListener::handleEvent):
+        * bindings/js/WorkerScriptController.cpp:
+        (WebCore::WorkerScriptController::WorkerScriptController):
+        - Always create a watchdog for the Web Worker's VM.  We need this in order to support
+          Worker.terminate().
+        (WebCore::WorkerScriptController::evaluate):
+        (WebCore::WorkerScriptController::scheduleExecutionTermination):
+        (WebCore::WorkerScriptController::isTerminatingExecution):
+        (WebCore::WorkerScriptController::forbidExecution):
+        (WebCore::WorkerScriptController::isExecutionTerminating): Deleted.
+        * bindings/js/WorkerScriptController.h:
+
 2015-08-26  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [Cocoa] PerformanceTest Layout/RegionsShapes.html is failing
index 351baee..280ca12 100644 (file)
@@ -135,9 +135,10 @@ void JSEventListener::handleEvent(ScriptExecutionContext* scriptExecutionContext
         globalObject->setCurrentEvent(savedEvent);
 
         if (is<WorkerGlobalScope>(*scriptExecutionContext)) {
+            auto scriptController = downcast<WorkerGlobalScope>(*scriptExecutionContext).script();
             bool terminatorCausedException = (exec->hadException() && isTerminatedExecutionException(exec->exception()));
-            if (terminatorCausedException || (vm.watchdog && vm.watchdog->didFire()))
-                downcast<WorkerGlobalScope>(*scriptExecutionContext).script()->forbidExecution();
+            if (terminatorCausedException || scriptController->isTerminatingExecution())
+                scriptController->forbidExecution();
         }
 
         if (exception) {
index 5a98c80..0eae64c 100644 (file)
@@ -56,6 +56,7 @@ WorkerScriptController::WorkerScriptController(WorkerGlobalScope* workerGlobalSc
     , m_workerGlobalScopeWrapper(*m_vm)
     , m_executionForbidden(false)
 {
+    m_vm->ensureWatchdog();
     initNormalWorldClientData(m_vm.get());
 }
 
@@ -121,8 +122,7 @@ void WorkerScriptController::evaluate(const ScriptSourceCode& sourceCode, NakedP
     JSC::evaluate(exec, sourceCode.jsSourceCode(), m_workerGlobalScopeWrapper->globalThis(), returnedException);
 
     VM& vm = exec->vm();
-    if ((returnedException && isTerminatedExecutionException(returnedException))
-        || (vm.watchdog && vm.watchdog->didFire())) {
+    if ((returnedException && isTerminatedExecutionException(returnedException)) || isTerminatingExecution()) {
         forbidExecution();
         return;
     }
@@ -149,20 +149,20 @@ void WorkerScriptController::setException(JSC::Exception* exception)
 void WorkerScriptController::scheduleExecutionTermination()
 {
     // The mutex provides a memory barrier to ensure that once
-    // termination is scheduled, isExecutionTerminating will
+    // termination is scheduled, isTerminatingExecution() will
     // accurately reflect that state when called from another thread.
     LockHolder locker(m_scheduledTerminationMutex);
-    if (m_vm->watchdog)
-        m_vm->watchdog->fire();
+    m_isTerminatingExecution = true;
+
+    ASSERT(m_vm->watchdog);
+    m_vm->watchdog->terminateSoon();
 }
 
-bool WorkerScriptController::isExecutionTerminating() const
+bool WorkerScriptController::isTerminatingExecution() const
 {
     // See comments in scheduleExecutionTermination regarding mutex usage.
     LockHolder locker(m_scheduledTerminationMutex);
-    if (m_vm->watchdog)
-        return m_vm->watchdog->didFire();
-    return false;
+    return m_isTerminatingExecution;
 }
 
 void WorkerScriptController::forbidExecution()
index 5747043..b0b6c0f 100644 (file)
@@ -71,7 +71,7 @@ namespace WebCore {
         // forbidExecution()/isExecutionForbidden() to guard against reentry into JS.
         // Can be called from any thread.
         void scheduleExecutionTermination();
-        bool isExecutionTerminating() const;
+        bool isTerminatingExecution() const;
 
         // Called on Worker thread when JS exits with termination exception caused by forbidExecution() request,
         // or by Worker thread termination code to prevent future entry into JS.
@@ -97,6 +97,7 @@ namespace WebCore {
         WorkerGlobalScope* m_workerGlobalScope;
         JSC::Strong<JSWorkerGlobalScope> m_workerGlobalScopeWrapper;
         bool m_executionForbidden;
+        bool m_isTerminatingExecution { false };
         mutable Lock m_scheduledTerminationMutex;
     };