Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
authorjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2016 06:07:07 +0000 (06:07 +0000)
committerjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2016 06:07:07 +0000 (06:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=161951
<rdar://problem/28295767>

Reviewed by Brian Burg.

Source/JavaScriptCore:

This removes an ambiguity in the protocol when stepping through
JavaScript. Previously, when paused and issuing a Debugger.step*
command the frontend would always receive a Debugger.resumed event and
then, maybe, a Debugger.paused event indicating we paused again (after
stepping). However, this ambiguity means that the frontend needs to
wait for a short period of time to determine if we really resumed
or not. And even still that decision may be incorrect if the step
takes a sufficiently long period of time.

The new approach removes this ambiguity. Now, in response to a
Debugger.step* command the backend MUST send a single Debugger.paused
event or Debugger.resumed event. Now the frontend knows that the
next Debugger event it receives after issuing the step command is
the result (stepped and paused, or stepped and resumed).

To make resuming consistent in all cases, a Debugger.resume command
will always respond with a Debugger.resumed event.

Finally, Debugger.continueToLocation is treated like a "big step"
in cases where we can resolve the location. If we can't resolve the
location it is treated as a resume, maintaining the old behavior.

* inspector/agents/InspectorDebuggerAgent.h:
* inspector/agents/InspectorDebuggerAgent.cpp:
(Inspector::InspectorDebuggerAgent::stepOver):
(Inspector::InspectorDebuggerAgent::stepInto):
(Inspector::InspectorDebuggerAgent::stepOut):
(Inspector::InspectorDebuggerAgent::willStepAndMayBecomeIdle):
(Inspector::InspectorDebuggerAgent::didBecomeIdleAfterStepping):
When stepping register a VM exit observer so that we can issue
a Debugger.resumed event if the step caused us to exit the VM.

(Inspector::InspectorDebuggerAgent::resume):
Set a flag to issue a Debugger.resumed event once we break out
of the nested run loop.

(Inspector::InspectorDebuggerAgent::didPause):
We are issuing Debugger.paused so clear the state to indicate that
we no longer need to issue Debugger.resumed event, we have paused.

(Inspector::InspectorDebuggerAgent::didContinue):
Only issue the Debugger.resumed event if needed (explicitly asked
to resume).

(Inspector::InspectorDebuggerAgent::continueToLocation):
(Inspector::InspectorDebuggerAgent::clearDebuggerBreakpointState):
All places that do continueProgram should be audited. In error cases,
if we are paused and continue we should remember to send Debugger.resumed.

* inspector/protocol/Debugger.json:
Clarify in the protocol description the contract of these methods.

Source/WebCore:

Covered by existing tests that would ASSERT otherwise.

* inspector/InspectorClient.cpp:
(WebCore::InspectorClient::doDispatchMessageOnFrontendPage):
When paused on an exception in the inspected page and evaluating
commands in the inspector frontend page (which evaluates JavaScript)
we ASSERT when entering the Global DOM VM with an existing exception.
This makes it so when we evaluate JavaScript in the frontend we
suspend / ignore the state of the VM for the inspected page, and
restore it when we return from the inspector.

Source/WebInspectorUI:

* UserInterface/Controllers/DebuggerManager.js:
(WebInspector.DebuggerManager.prototype.debuggerDidResume):
Now, Debugger.resumed events really mean the debugger resumed,
so act immediately instead of guessing. We must still guess
in legacy backends.

* UserInterface/Test/Test.js:
When the inspector frontend encounters an issue, log it.

* UserInterface/Views/DebuggerSidebarPanel.js:
(WebInspector.DebuggerSidebarPanel.prototype._debuggerDidPause):
(WebInspector.DebuggerSidebarPanel.prototype._debuggerActiveCallFrameDidChange):
Always enable the step out button. I don't think it makes sense to disable
it sometimes, and if there are issues with this we should solve the issues
instead of hiding them.

LayoutTests:

Rewrite tests to be more deterministic. For tests that
relied on a Resumed event to happen after a short amount
of time, instead have the test dispatch an event when it is
appropriate to continue. Take this opportunity to rewrite
some tests using new style and best practices.

* inspector/debugger/break-in-constructor-before-super.html:
* inspector/debugger/break-on-exception-throw-in-promise.html:
* inspector/debugger/break-on-exception.html:
* inspector/debugger/break-on-uncaught-exception-throw-in-promise.html:
* inspector/debugger/break-on-uncaught-exception.html:
* inspector/debugger/breakpoint-syntax-error-top-level.html:
* inspector/debugger/command-line-api-exception-expected.txt:
* inspector/debugger/command-line-api-exception-nested-catch.html:
* inspector/debugger/command-line-api-exception.html:
* inspector/debugger/csp-exceptions.html:
* inspector/debugger/didSampleProbe-multiple-probes.html:
* inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:
* inspector/debugger/evaluateOnCallFrame-errors.html:
* inspector/debugger/pause-reason-expected.txt:
* inspector/debugger/pause-reason.html:
* inspector/debugger/paused-scopes-expected.txt:
* inspector/debugger/paused-scopes.html:
* inspector/debugger/resources/exceptions.js:
* inspector/debugger/scriptParsed.html:
* inspector/debugger/sourceURL-repeated-identical-executions.html:
* inspector/debugger/sourceURLs.html:
* inspector/debugger/stepping/stepping-pause-in-inner-step-to-parent.html:

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

35 files changed:
LayoutTests/ChangeLog
LayoutTests/inspector/debugger/break-in-constructor-before-super.html
LayoutTests/inspector/debugger/break-on-exception-throw-in-promise.html
LayoutTests/inspector/debugger/break-on-exception.html
LayoutTests/inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
LayoutTests/inspector/debugger/break-on-uncaught-exception.html
LayoutTests/inspector/debugger/breakpoint-syntax-error-top-level.html
LayoutTests/inspector/debugger/command-line-api-exception-expected.txt
LayoutTests/inspector/debugger/command-line-api-exception-nested-catch.html
LayoutTests/inspector/debugger/command-line-api-exception.html
LayoutTests/inspector/debugger/csp-exceptions.html
LayoutTests/inspector/debugger/didSampleProbe-multiple-probes.html
LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html
LayoutTests/inspector/debugger/evaluateOnCallFrame-errors.html
LayoutTests/inspector/debugger/pause-reason-expected.txt
LayoutTests/inspector/debugger/pause-reason.html
LayoutTests/inspector/debugger/paused-scopes-expected.txt
LayoutTests/inspector/debugger/paused-scopes.html
LayoutTests/inspector/debugger/resources/exceptions.js
LayoutTests/inspector/debugger/scriptParsed.html
LayoutTests/inspector/debugger/sourceURL-repeated-identical-executions.html
LayoutTests/inspector/debugger/sourceURLs.html
LayoutTests/inspector/debugger/stepping/stepping-pause-in-inner-step-to-parent.html
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/debugger/Debugger.cpp
Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp
Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h
Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp
Source/JavaScriptCore/inspector/protocol/Debugger.json
Source/WebCore/ChangeLog
Source/WebCore/inspector/InspectorClient.cpp
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js
Source/WebInspectorUI/UserInterface/Test/Test.js
Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js

index 9ac9c46..ed3111f 100644 (file)
@@ -1,3 +1,40 @@
+2016-11-09  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
+        https://bugs.webkit.org/show_bug.cgi?id=161951
+        <rdar://problem/28295767>
+
+        Reviewed by Brian Burg.
+
+        Rewrite tests to be more deterministic. For tests that
+        relied on a Resumed event to happen after a short amount
+        of time, instead have the test dispatch an event when it is
+        appropriate to continue. Take this opportunity to rewrite
+        some tests using new style and best practices.
+
+        * inspector/debugger/break-in-constructor-before-super.html:
+        * inspector/debugger/break-on-exception-throw-in-promise.html:
+        * inspector/debugger/break-on-exception.html:
+        * inspector/debugger/break-on-uncaught-exception-throw-in-promise.html:
+        * inspector/debugger/break-on-uncaught-exception.html:
+        * inspector/debugger/breakpoint-syntax-error-top-level.html:
+        * inspector/debugger/command-line-api-exception-expected.txt:
+        * inspector/debugger/command-line-api-exception-nested-catch.html:
+        * inspector/debugger/command-line-api-exception.html:
+        * inspector/debugger/csp-exceptions.html:
+        * inspector/debugger/didSampleProbe-multiple-probes.html:
+        * inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:
+        * inspector/debugger/evaluateOnCallFrame-errors.html:
+        * inspector/debugger/pause-reason-expected.txt:
+        * inspector/debugger/pause-reason.html:
+        * inspector/debugger/paused-scopes-expected.txt:
+        * inspector/debugger/paused-scopes.html:
+        * inspector/debugger/resources/exceptions.js:
+        * inspector/debugger/scriptParsed.html:
+        * inspector/debugger/sourceURL-repeated-identical-executions.html:
+        * inspector/debugger/sourceURLs.html:
+        * inspector/debugger/stepping/stepping-pause-in-inner-step-to-parent.html:
+
 2016-11-09  Chris Dumez  <cdumez@apple.com>
 
         [WK2][NETWORK_SESSION] Add support for downloading file backed blobs
index 08c026f..b0c41df 100644 (file)
@@ -34,7 +34,7 @@ function test()
     suite.addTestCase({
         name: "TriggerPauseInConstructorBeforeSuper",
         description: "Trigger a pause in a constructor before calling super should not crash.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             ProtocolTest.evaluateInPage("triggerPause()");
 
             InspectorProtocol.eventHandler["Debugger.paused"] = (messageObject) => {
index 8f31650..8e3682f 100644 (file)
@@ -15,7 +15,7 @@ function test()
     function addTestCase({name, description, expression}) {
         suite.addTestCase({
             name, description,
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 InspectorTest.evaluateInPage(expression);
                 WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                     let targetData = WebInspector.debuggerManager.dataForTarget(WebInspector.debuggerManager.activeCallFrame.target);
@@ -59,7 +59,7 @@ function test()
     suite.addTestCase({
         name: "BreakOnAnyException.Promise.ExceptionInPromiseThenAndRethrownInCatch",
         description: "Break on an exception thrown in Promise then handler, and then again when rethrown in catch handler.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorTest.evaluateInPage("setTimeout(testThrowingInPromiseWithRethrowInCatch)");
 
             function logPauseLocation() {
index bd174fa..34bd290 100644 (file)
@@ -15,7 +15,7 @@ function test()
     function addTestCase({name, description, expression}) {
         suite.addTestCase({
             name, description,
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 InspectorTest.evaluateInPage(expression);
                 WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                     let targetData = WebInspector.debuggerManager.dataForTarget(WebInspector.debuggerManager.activeCallFrame.target);
@@ -53,7 +53,7 @@ function test()
     addTestCase({
         name: "BreakOnAnyException.CaughtException",
         description: "Break on a caught exception.",
-        expression: "setTimeout(testCatch)",
+        expression: `setTimeout(() => { testCatch(); })`,
     });
 
     suite.runTestCasesAndFinish();
index 03a22b8..8898e7d 100644 (file)
@@ -21,7 +21,7 @@ function test()
     function addTestCase({name, description, expression}) {
         suite.addTestCase({
             name, description,
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 InspectorTest.evaluateInPage(expression);
                 InspectorTest.evaluateInPage(`setTimeout(() => { TestPage.dispatchEventToFrontend("AfterTestFunction"); }, 50)`);
 
index 389ad99..c4b9987 100644 (file)
@@ -15,7 +15,7 @@ function test()
     function addTestCase({name, description, expression}) {
         suite.addTestCase({
             name, description,
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 InspectorTest.evaluateInPage(expression);
                 WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                     let targetData = WebInspector.debuggerManager.dataForTarget(WebInspector.debuggerManager.activeCallFrame.target);
@@ -53,7 +53,7 @@ function test()
     suite.addTestCase({
         name: "BreakOnUncaughtException.CaughtException",
         description: "No break on a caught exception.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorTest.evaluateInPage(`setTimeout(() => {
                 testCatch();
                 TestPage.dispatchEventToFrontend("AfterTestFunction");
index b885c7e..bba2759 100644 (file)
@@ -20,7 +20,7 @@ function test()
     suite.addTestCase({
         name: "TopLevelSyntaxErrorDontCrash",
         description: "Make sure exceptions from top-level syntax errors don't cause us to crash.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorProtocol.eventHandler["Debugger.paused"] = function(messageObject) {
                 InspectorProtocol.sendCommand("Debugger.resume");
 
index ba5a9dc..f3e139a 100644 (file)
@@ -19,30 +19,39 @@ PASS: $exception should be undefined if there is no exception.
 
 -- Running test case: UncaughtTypeException
 $exception => TypeError: undefined is not an object (evaluating '[].x.x')
+Uncaught exception in test page: TypeError: undefined is not an object (evaluating '[].x.x') [exceptions.js:4]
 
 -- Running test case: UncaughtReferenceException
 $exception => ReferenceError: Can't find variable: variableThatDoesNotExist
+Uncaught exception in test page: ReferenceError: Can't find variable: variableThatDoesNotExist [exceptions.js:10]
 
 -- Running test case: UncaughtSyntaxException
 $exception => SyntaxError: Unexpected token ')'
+Uncaught exception in test page: SyntaxError: Unexpected token ')' [exceptions.js:16]
 
 -- Running test case: UncaughtDOMException
 $exception => IndexSizeError (DOM Exception 1): The index is not in the allowed range.
+Uncaught exception in test page: IndexSizeError (DOM Exception 1): The index is not in the allowed range. [exceptions.js:22]
 
 -- Running test case: UncaughtString
 $exception => thrown string
+Uncaught exception in test page: thrown string [exceptions.js:27]
 
 -- Running test case: UncaughtNumber
 $exception => 123.456
+Uncaught exception in test page: 123.456 [exceptions.js:32]
 
 -- Running test case: UncaughtNull
 $exception => null
+Uncaught exception in test page: null [exceptions.js:37]
 
 -- Running test case: UncaughtObject
 $exception => Object
+Uncaught exception in test page: [object Object] [exceptions.js:42]
 
 -- Running test case: UncaughtNode
 $exception => <body>
+Uncaught exception in test page: [object HTMLBodyElement] [exceptions.js:47]
 
 -- Running test case: CatchTypeException
 PASS: Paused, stepping out to catch block...
index b66f768..4beee23 100644 (file)
@@ -28,7 +28,7 @@ function test()
     suite.addTestCase({
         name: "EmptyBefore",
         description: "Without exceptions, $exception should be undefined",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             WebInspector.runtimeManager.evaluateInInspectedWindow("$exception", {objectGroup: "test", includeCommandLineAPI: true, doNotPauseOnExceptionsAndMuteConsole: true}, (result, wasThrown) => {
                 InspectorTest.expectThat(result.description === "undefined", "$exception should be undefined if there is no exception.");
                 resolve();
@@ -39,7 +39,7 @@ function test()
     suite.addTestCase({
         name: "CheckExceptionInsideNestedCatchBlocks",
         description: "Check $exception is always the current exception object when stepping through catch blocks.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorTest.evaluateInPage("setTimeout(nestedCatchBlocks, 0)");
 
             let phase = 0;
@@ -75,9 +75,9 @@ function test()
                 if (phase === 4) {
                     dumpCommandLineAPIValue("OUTER 2");
                     checkIfExceptionValueMatchesVariable("e1");
+                    InspectorTest.singleFireEventListener("AfterTestFunction", resolve);
                     WebInspector.debuggerManager.resume().then(() => {
                         WebInspector.debuggerManager.removeEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange, listener);
-                        resolve();
                     }, reject);
                     return;
                 }
@@ -88,7 +88,7 @@ function test()
     suite.addTestCase({
         name: "EmptyAfter",
         description: "Without exceptions, $exception should be undefined",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             WebInspector.runtimeManager.evaluateInInspectedWindow("$exception", {objectGroup: "test", includeCommandLineAPI: true, doNotPauseOnExceptionsAndMuteConsole: true}, (result, wasThrown) => {
                 InspectorTest.expectThat(result.description === "undefined", "$exception should be undefined if there is no exception.");
                 resolve();
index 0de0478..b5063da 100644 (file)
@@ -4,8 +4,8 @@
 <script src="../../http/tests/inspector/resources/inspector-test.js"></script>
 <script src="./resources/exceptions.js"></script>
 <script>
-// We expect uncaught exceptions, so avoid logs for them.
-window.onerror = function(){};
+TestPage.allowUncaughtExceptions = true;
+TestPage.needToSanitizeUncaughtExceptionURLs = true;
 
 function test()
 {
@@ -16,7 +16,7 @@ function test()
     function addNoExceptionTest(name) {
         suite.addTestCase({
             name, description: "Without exceptions, $exception should be undefined",
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 WebInspector.runtimeManager.evaluateInInspectedWindow("$exception", {objectGroup: "test", includeCommandLineAPI: true, doNotPauseOnExceptionsAndMuteConsole: true}, (result, wasThrown) => {
                     InspectorTest.expectThat(result.description === "undefined", "$exception should be undefined if there is no exception.");
                     resolve();
@@ -28,7 +28,7 @@ function test()
     function addUncaughtExceptionTest(name, testFunction) {
         suite.addTestCase({
             name, description: "Trigger an uncaught exception and check $exception.",
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 InspectorTest.evaluateInPage(`setTimeout(${testFunction})`);
                 WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                     WebInspector.runtimeManager.evaluateInInspectedWindow("$exception", {objectGroup: "test", includeCommandLineAPI: true, doNotPauseOnExceptionsAndMuteConsole: true}, (result, wasThrown) => {
@@ -43,7 +43,7 @@ function test()
     function addCaughtExceptionTest(name, expression) {
         suite.addTestCase({
             name, description: "Trigger a caught exception and check $exception.",
-            test: (resolve, reject) => {
+            test(resolve, reject) {
                 InspectorTest.evaluateInPage(expression);
 
                 let didStep = false;
@@ -67,10 +67,10 @@ function test()
                         });
                         WebInspector.runtimeManager.evaluateInInspectedWindow("$exception", {objectGroup: "test", includeCommandLineAPI: true, doNotPauseOnExceptionsAndMuteConsole: true}, (result, wasThrown) => {
                             InspectorTest.log("$exception => " + result.description);
+                            InspectorTest.singleFireEventListener("AfterTestFunction", resolve);
                             WebInspector.debuggerManager.resume().then(() => {
                                 WebInspector.debuggerManager.removeEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange, listener);
-                                resolve();
-                            }, reject);
+                            }).catch(reject);
                         });
                         return;
                     }
@@ -91,9 +91,9 @@ function test()
     addUncaughtExceptionTest("UncaughtObject", "throwObject");
     addUncaughtExceptionTest("UncaughtNode", "throwNode");
 
-    addCaughtExceptionTest("CatchTypeException", "setTimeout(function() { catcher(triggerUncaughtTypeException); })");
-    addCaughtExceptionTest("CatchThrownString", "setTimeout(function() { catcher(throwString); })");
-    addCaughtExceptionTest("CatchThrownObject", "setTimeout(function() { catcher(throwObject); })");
+    addCaughtExceptionTest("CatchTypeException", `setTimeout(function() { catcher(triggerUncaughtTypeException); TestPage.dispatchEventToFrontend("AfterTestFunction"); })`);
+    addCaughtExceptionTest("CatchThrownString", `setTimeout(function() { catcher(throwString); TestPage.dispatchEventToFrontend("AfterTestFunction"); })`);
+    addCaughtExceptionTest("CatchThrownObject", `setTimeout(function() { catcher(throwObject); TestPage.dispatchEventToFrontend("AfterTestFunction"); })`);
 
     addNoExceptionTest("AfterExceptions");
 
index 48c77aa..8cd5996 100644 (file)
@@ -24,14 +24,11 @@ function test()
     suite.addTestCase({
         name: "TriggerCSPExceptionInsideOfScript",
         description: "Trigger a CSP Exception inside of script should pause.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorTest.evaluateInPage("setTimeout(triggerCSPExceptionInsideScript)");
             WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                 InspectorTest.pass("CSP Exception caused by script evaluation should pause.");
-                WebInspector.debuggerManager.resume();
-            });
-            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Resumed, (event) => {
-                resolve();
+                WebInspector.debuggerManager.resume().then(resolve, reject);
             });
         }
     });
@@ -39,7 +36,7 @@ function test()
     suite.addTestCase({
         name: "TriggerCSPExceptionOutsideOfScript",
         description: "Trigger a CSP Exception outside of script should not pause or crash.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorTest.evaluateInPage("setTimeout(triggerCSPExceptionOutsideScript)");
             let tempPauseFailListener = WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                 InspectorTest.fail("CSP Exception caused outside of script evaluation should not pause, but did.");
index b992bd6..a3a860c 100644 (file)
@@ -39,7 +39,13 @@ function test()
         {
             message: "SampleIds from a any probe are sequential and start counting from one.",
             predicate: function samplesHaveSequentialIds() {
-                return samples.every(function(sample, idx) { return sample.sampleId === idx + 1; });
+                let initialSampleId = samples[0].sampleId;
+                for (let i = 0; i < samples.length; ++i) {
+                    let expectedSampleId = initialSampleId + i;
+                    if (samples[i].sampleId !== expectedSampleId)
+                        return false;
+                }
+                return true;
             },
             error: dumpSamples
         },
@@ -59,7 +65,7 @@ function test()
                 ]
             };
 
-            InspectorProtocol.sendCommand("Debugger.setBreakpoint", {location: location, options: options}, function(responseObject) {
+            InspectorProtocol.sendCommand("Debugger.setBreakpoint", {location, options}, function(responseObject) {
                 InspectorProtocol.checkForError(responseObject);
 
                 ProtocolTest.log("Running breakpointActions to trigger probe samples.");
index d4657cd..fb77588 100644 (file)
@@ -77,7 +77,7 @@ function test()
     suite.addTestCase({
         name: "ValidateCallFrames",
         description: "Test6 evaluate can access CommandLineAPI methods.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             let targetData = WebInspector.debuggerManager.dataForTarget(WebInspector.debuggerManager.activeCallFrame.target);
             InspectorTest.expectThat(WebInspector.debuggerManager.activeCallFrame.functionName === "bar", "Strict call frame is `bar`.");
             InspectorTest.expectThat(targetData.callFrames[1].functionName === "foo", "Non-strict call frame is `foo`.");
@@ -97,7 +97,7 @@ function test()
     suite.addTestCase({
         name: "AccessCommandLineAPI",
         description: "Test evaluate can access CommandLineAPI methods.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             testEvaluateOnStrictCallFrame("keys.toString()", (resultValue) => {
                 InspectorTest.expectThat(resultValue.includes("[Command Line API]"), "CommandLineAPI `keys` can be accessed in the `bar` strict call frame.");
             });
@@ -114,7 +114,7 @@ function test()
     suite.addTestCase({
         name: "AccessGlobalVariable",
         description: "Test evaluate can access global variables.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             testEvaluateOnStrictCallFrame("varGlobalVariable", (x) => { InspectorTest.expectThat(x === 1, "Should be able to access var in global scope in strict call frame."); });
             testEvaluateOnStrictCallFrame("letGlobalVariable", (x) => { InspectorTest.expectThat(x === 2, "Should be able to access let in global scope in strict call frame."); });
             testEvaluateOnStrictCallFrame("constGlobalVariable", (x) => { InspectorTest.expectThat(x === 3, "Should be able to access const in global scope in strict call frame."); });
@@ -128,7 +128,7 @@ function test()
     suite.addTestCase({
         name: "NoVariablesCreatedOnCallFrame",
         description: "Test evaluate may not create new local variables.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             testEvaluateOnStrictCallFrame(`
                 var createdVarLocalVariable = 1;
                 let createdLetLocalVariable = 2;
@@ -152,7 +152,7 @@ function test()
     suite.addTestCase({
         name: "NonStrictAndStrictEvaluations",
         description: "Test evaluate can run strict and non-strict programs.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             // Non strict, this is okay.
             testEvaluateOnNonStrictCallFrame("arguments.callee.name", (resultValue) => {
                 InspectorTest.expectThat(resultValue === "foo", "Non-strict evaluation. Should be able to access arguments.callee.");
@@ -168,7 +168,7 @@ function test()
     suite.addTestCase({
         name: "CommandLineAPIDoesNotShadowLocalVariables",
         description: "Test CommandLineAPI does not shadow local variables.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             testEvaluateOnNonStrictCallFrame("keys", (resultValue) => {
                 InspectorTest.expectThat(resultValue === 123, "Local variable `keys` should not be shadowed by CommandLineAPI `keys` function in call frame for `foo`.");
                 resolve();
@@ -179,7 +179,7 @@ function test()
     suite.addTestCase({
         name: "CommandLineAPIDoesNotShadowGlobalVariables",
         description: "Test CommandLineAPI does not shadow global variables.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             testEvaluateOnStrictCallFrame("keys.toString()", (resultValue) => {
                 InspectorTest.expectThat(resultValue.includes("[Command Line API]"), "CommandLineAPI `keys` can be accessed in the `bar` strict call frame.");
             });
@@ -200,7 +200,7 @@ function test()
     suite.addTestCase({
         name: "CommandLineAPIDoesNotShadowGlobalObjectProperties",
         description: "Test CommandLineAPI does not shadow global object properties.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             testEvaluateOnStrictCallFrame("values.toString()", (resultValue) => {
                 InspectorTest.expectThat(resultValue === "[object HTMLDivElement]", "`values` should be `window.values` and not shadowed by CommandLineAPI `values` function in strict call frame.");
             });
@@ -213,7 +213,7 @@ function test()
 
     suite.addTestCase({
         name: "Complete",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             WebInspector.debuggerManager.resume();
             resolve();
         }
index 98f5660..e812a2e 100644 (file)
@@ -17,7 +17,7 @@ function test()
     suite.addTestCase({
         name: "EvaluateOnCallFrameNotPaused",
         description: "Triggering evaluate on call frame when not paused should issue an error.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             ProtocolTest.evaluateInPage("triggerPause()");
 
             let callFrameIdentifier;
@@ -41,7 +41,7 @@ function test()
     suite.addTestCase({
         name: "EvaluateOnCallFrameBadCallFrameIdentifier",
         description: "Triggering evaluate on call frame with a bad call frame identifier should issue an error.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             ProtocolTest.evaluateInPage("triggerPause()");
 
             InspectorProtocol.eventHandler["Debugger.paused"] = (messageObject) => {
index 88e330d..9e08fb3 100644 (file)
@@ -2,29 +2,36 @@ CONSOLE MESSAGE: line 8: TypeError: undefined is not an object (evaluating '[].x
 CONSOLE MESSAGE: line 18: Assertion message
 Test that pausing in different ways triggers different pause reasons.
 
+
+== Running test suite: Debugger.PauseReason
+-- Running test case: Debugger.PauseReason.Exception
 PAUSE #1
   REASON: exception
   DATA: {"description":"TypeError: undefined is not an object (evaluating '[].x.x')"}
+Uncaught exception in test page: TypeError: undefined is not an object (evaluating '[].x.x') [pause-reasons.js:8]
 RESUMED
 
+-- Running test case: Debugger.PauseReason.DebuggerStatement
 PAUSE #2
   REASON: debugger-statement
   NO DATA
 RESUMED
 
+-- Running test case: Debugger.PauseReason.Assertion
 PAUSE #3
   REASON: assertion
   DATA: {"message":"Assertion message"}
 RESUMED
 
+-- Running test case: Debugger.PauseReason.Breakpoint
 PAUSE #4
   REASON: breakpoint
   DATA: {"breakpointId":"pause-reasons.js:3:0"}
 RESUMED
 
+-- Running test case: Debugger.PauseReason.PauseImmediately
 PAUSE #5
   REASON: pause-on-next-statement
   NO DATA
 RESUMED
 
-
index f5b2310..fc427e0 100644 (file)
 <script type="text/javascript" src="../../http/tests/inspector/resources/inspector-test.js"></script>
 <script type="text/javascript" src="./resources/pause-reasons.js"></script>
 <script>
-// We expect uncaught exceptions, so avoid logs for them.
-window.onerror = function(){};
+TestPage.allowUncaughtExceptions = true;
+TestPage.needToSanitizeUncaughtExceptionURLs = true;
 
 function test()
 {
-    var pauses = 0;
-    var index = 0;
-    var testCases = [
-        { expression: "setTimeout(triggerException, 0)" },
-        { expression: "setTimeout(triggerDebuggerStatement, 0)" },
-        { expression: "setTimeout(triggerAssert, 0)" },
-        { expression: "setTimeout(triggerBreakpoint, 0)" },
-        { expression: "setTimeout(function() { 1+1; }, 50)", setup: function() { WebInspector.debuggerManager.pause(); } },
-    ];
-
-    function nextTestCase()
-    {
-        var test = testCases[index++];
-        if (!test) {
-            InspectorTest.completeTest();
-            return;
-        }
-
-        InspectorTest.evaluateInPage(test.expression);
-
-        if (test.setup)
-            test.setup();
+    WebInspector.debuggerManager.allExceptionsBreakpoint.disabled = false;
+    WebInspector.debuggerManager.assertionsBreakpoint.disabled = false;
+
+    for (let script of WebInspector.debuggerManager.dataForTarget(WebInspector.mainTarget).scripts) {
+        if (!/pause-reasons\.js$/.test(script.url))
+            continue;
+        let sourceCodeLocation = script.createSourceCodeLocation(3, 0);
+        let breakpoint = new WebInspector.Breakpoint(sourceCodeLocation);
+        WebInspector.debuggerManager.addBreakpoint(breakpoint);
+        break;
     }
 
-    function sanitizePauseData(data)
-    {
-        // Sanitize RemoteObjects to just output the description.
+    function sanitizePauseData(data) {
         if (data.description)
             return {description: data.description};
-
-        // Sanitize BreakpointIdentifier path.
         if (data.breakpointId)
             return {breakpointId: data.breakpointId.substring(data.breakpointId.indexOf("pause-reasons.js"))};
-
         return data;
     }
 
-    WebInspector.debuggerManager.allExceptionsBreakpoint.disabled = false;
-    WebInspector.debuggerManager.assertionsBreakpoint.disabled = false;
 
-    WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.ScriptAdded, function(event) {
-        var scriptObject = event.data.script;
-        if (!/pause-reasons\.js$/.test(scriptObject.url))
-            return;
+    let suite = InspectorTest.createAsyncSuite("Debugger.PauseReason");
 
-        var sourceCodeLocation = scriptObject.createSourceCodeLocation(3, 0);
-        var breakpoint = new WebInspector.Breakpoint(sourceCodeLocation);
-        WebInspector.debuggerManager.addBreakpoint(breakpoint);
+    let pauses = 0;
 
-        nextTestCase();
+    function addTestCase({name, description, expression, setup}) {
+        suite.addTestCase({
+            name, description,
+            test(resolve, reject) {
+                if (setup)
+                    setup();
+
+                expression += `;setTimeout(() => { TestPage.dispatchEventToFrontend("AfterTestFunction"); })`;
+                InspectorTest.evaluateInPage(expression);
+
+                WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
+                    let targetData = WebInspector.debuggerManager.dataForTarget(WebInspector.debuggerManager.activeCallFrame.target);
+                    InspectorTest.log("PAUSE #" + (++pauses));
+                    InspectorTest.log("  REASON: " + targetData.pauseReason);
+                    if (targetData.pauseData)
+                        InspectorTest.log("  DATA: " + JSON.stringify(sanitizePauseData(targetData.pauseData)));
+                    else
+                        InspectorTest.log("  NO DATA");
+
+                    WebInspector.debuggerManager.resume().catch(reject);
+                    InspectorTest.singleFireEventListener("AfterTestFunction", resolve);
+                });
+
+                WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Resumed, function(event) {
+                    InspectorTest.log("RESUMED");
+                });
+            }
+        });
+    }
+
+    addTestCase({
+        name: "Debugger.PauseReason.Exception",
+        description: "Pause for exception should have expected pause reason.",
+        expression: `setTimeout(triggerException)`,
     });
 
-    WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.Paused, function(event) {
-        let targetData = WebInspector.debuggerManager.dataForTarget(WebInspector.debuggerManager.activeCallFrame.target);
-        InspectorTest.log("PAUSE #" + (++pauses));
-        InspectorTest.log("  REASON: " + targetData.pauseReason);
-        if (targetData.pauseData)
-            InspectorTest.log("  DATA: " + JSON.stringify(sanitizePauseData(targetData.pauseData)));
-        else
-            InspectorTest.log("  NO DATA");
+    addTestCase({
+        name: "Debugger.PauseReason.DebuggerStatement",
+        description: "Pause for debugger statement should have expected pause reason.",
+        expression: "setTimeout(triggerDebuggerStatement)",
+    });
 
-        WebInspector.debuggerManager.resume();
+    addTestCase({
+        name: "Debugger.PauseReason.Assertion",
+        description: "Pause for assertion should have expected pause reason.",
+        expression: "setTimeout(triggerAssert)",
     });
 
-    WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.Resumed, function(event) {
-        InspectorTest.log("RESUMED");
-        InspectorTest.log("");
+    addTestCase({
+        name: "Debugger.PauseReason.Breakpoint",
+        description: "Pause for breakpoint should have expected pause reason.",
+        expression: "setTimeout(triggerBreakpoint)",
+    });
 
-        nextTestCase();
+    addTestCase({
+        name: "Debugger.PauseReason.PauseImmediately",
+        description: "Pause for Debugger.pause command should have expected pause reason.",
+        expression: "setTimeout(() => { 1 + 1 })",
+        setup() { WebInspector.debuggerManager.pause(); },
     });
 
-    InspectorTest.reloadPage();
+    suite.runTestCasesAndFinish();
 }
 </script>
 </head>
 <body onload="runTest()">
-    <p>Test that pausing in different ways triggers different pause reasons.</p>
+<p>Test that pausing in different ways triggers different pause reasons.</p>
 </body>
 </html>
index eede9fe..a7f28e7 100644 (file)
@@ -332,3 +332,5 @@ CALLFRAME: entry
   SCOPE: Name () - Type (Global) - Hash ()
 
 
+-- Running test case: PausedCallFrameScope.Complete
+
index 7dc88a5..b077090 100644 (file)
@@ -86,9 +86,9 @@ function test()
     suite.addTestCase({
         name: "PausedCallFrameScope.TriggerFirstPause",
         description: "Dump CallFrames and Scopes with the first pause.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             InspectorTest.evaluateInPage("setTimeout(entry)");
-            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange, (event) => {
+            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
                 dumpCallFrames().then(resolve, reject);
             });
         }
@@ -97,13 +97,10 @@ function test()
     suite.addTestCase({
         name: "PausedCallFrameScope.TriggerSecondPause",
         description: "Dump CallFrames and Scopes with the second pause.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             WebInspector.debuggerManager.resume();
-            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange, (event) => {
-                dumpCallFrames().then(() => {
-                    WebInspector.debuggerManager.resume();
-                    resolve();
-                }, reject);
+            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
+                dumpCallFrames().then(resolve, reject);
             });
         }
     });
@@ -111,17 +108,21 @@ function test()
     suite.addTestCase({
         name: "PausedCallFrameScope.TriggerThirdPause",
         description: "Dump CallFrames and Scopes with the third pause.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             WebInspector.debuggerManager.resume();
-            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange, (event) => {
-                dumpCallFrames().then(() => {
-                    WebInspector.debuggerManager.resume();
-                    resolve();
-                }, reject);
+            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {
+                dumpCallFrames().then(resolve, reject);
             });
         }
     });
 
+    suite.addTestCase({
+        name: "PausedCallFrameScope.Complete",
+        test(resolve, reject) {
+            WebInspector.debuggerManager.resume().then(resolve, reject);
+        }
+    });
+
     suite.runTestCasesAndFinish();
 }
 </script>
index f5d38e5..9323326 100644 (file)
@@ -68,4 +68,6 @@ function nestedCatchBlocks()
         }
         console.log(e1);
     }
+
+    TestPage.dispatchEventToFrontend("AfterTestFunction");
 }
index f677101..d4f8f38 100644 (file)
@@ -59,7 +59,7 @@ function test()
 
     suite.addTestCase({
         name: "EnableDebuggerDomainAndCheckInitialScripts",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
 
             let initialScriptParsedMessages = [];
             let receivingInitialScripts = true;
index c243d02..23e93c1 100644 (file)
@@ -37,7 +37,7 @@ function test()
     suite.addTestCase({
         name: "CheckFunctionConstructorMultipleTimes",
         description: "Trigger multiple Function constructor calls with the same source and expect multiple Scripts added with sourceURLs.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             let seen = 0;
             WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.ScriptAdded, (event) => {
                 if (event.data.script.sourceURL === "test-Function-constructor.js")
@@ -53,7 +53,7 @@ function test()
     suite.addTestCase({
         name: "CheckProgramMultipleTimes",
         description: "Trigger multiple program executions with the same source and expect multiple Scripts added with sourceURLs.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             let seen = 0;
             WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.ScriptAdded, (event) => {
                 if (event.data.script.sourceURL === "test-program.js")
@@ -69,7 +69,7 @@ function test()
     suite.addTestCase({
         name: "CheckEvalMultipleTimes",
         description: "Trigger eval with the same source and expect multiple Scripts added with sourceURLs.",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             let seen = 0;
             WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.ScriptAdded, (event) => {
                 if (event.data.script.sourceURL === "test-eval.js")
index 6775eb7..9cc1f8c 100644 (file)
@@ -30,7 +30,7 @@ function test()
 
     suite.addTestCase({
         name: "TestExpressionsForSourceURL",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             for (let expression of expressions)
                 ProtocolTest.evaluateInPage(expression);
 
@@ -54,7 +54,7 @@ function test()
 
     suite.addTestCase({
         name: "TestExpressionsForSourceMappingURL",
-        test: (resolve, reject) => {
+        test(resolve, reject) {
             // Rewrite the "sourceURL" to "sourceMappingURL" in the original expressions.
             for (let expression of expressions)
                 ProtocolTest.evaluateInPage(expression.replace(/sourceURL/g, "sourceMappingURL"));
index 1d4edc2..e8e9673 100644 (file)
@@ -16,8 +16,6 @@ function pause() {
 
 function test()
 {
-    InspectorTest.debug();
-
     let suite = InspectorTest.createAsyncSuite("Debugger.stepping.inner-to-outer");
 
     window.initializeSteppingTestSuite(suite);
index 076a06f..d01deb7 100644 (file)
@@ -1,3 +1,63 @@
+2016-11-08  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
+        https://bugs.webkit.org/show_bug.cgi?id=161951
+        <rdar://problem/28295767>
+
+        Reviewed by Brian Burg.
+
+        This removes an ambiguity in the protocol when stepping through
+        JavaScript. Previously, when paused and issuing a Debugger.step*
+        command the frontend would always receive a Debugger.resumed event and
+        then, maybe, a Debugger.paused event indicating we paused again (after
+        stepping). However, this ambiguity means that the frontend needs to
+        wait for a short period of time to determine if we really resumed
+        or not. And even still that decision may be incorrect if the step
+        takes a sufficiently long period of time.
+
+        The new approach removes this ambiguity. Now, in response to a
+        Debugger.step* command the backend MUST send a single Debugger.paused
+        event or Debugger.resumed event. Now the frontend knows that the
+        next Debugger event it receives after issuing the step command is
+        the result (stepped and paused, or stepped and resumed).
+
+        To make resuming consistent in all cases, a Debugger.resume command
+        will always respond with a Debugger.resumed event.
+
+        Finally, Debugger.continueToLocation is treated like a "big step"
+        in cases where we can resolve the location. If we can't resolve the
+        location it is treated as a resume, maintaining the old behavior.
+
+        * inspector/agents/InspectorDebuggerAgent.h:
+        * inspector/agents/InspectorDebuggerAgent.cpp:
+        (Inspector::InspectorDebuggerAgent::stepOver):
+        (Inspector::InspectorDebuggerAgent::stepInto):
+        (Inspector::InspectorDebuggerAgent::stepOut):
+        (Inspector::InspectorDebuggerAgent::willStepAndMayBecomeIdle):
+        (Inspector::InspectorDebuggerAgent::didBecomeIdleAfterStepping):
+        When stepping register a VM exit observer so that we can issue
+        a Debugger.resumed event if the step caused us to exit the VM.
+
+        (Inspector::InspectorDebuggerAgent::resume):
+        Set a flag to issue a Debugger.resumed event once we break out
+        of the nested run loop.
+
+        (Inspector::InspectorDebuggerAgent::didPause):
+        We are issuing Debugger.paused so clear the state to indicate that
+        we no longer need to issue Debugger.resumed event, we have paused.
+
+        (Inspector::InspectorDebuggerAgent::didContinue):
+        Only issue the Debugger.resumed event if needed (explicitly asked
+        to resume).
+
+        (Inspector::InspectorDebuggerAgent::continueToLocation):
+        (Inspector::InspectorDebuggerAgent::clearDebuggerBreakpointState):
+        All places that do continueProgram should be audited. In error cases,
+        if we are paused and continue we should remember to send Debugger.resumed.
+
+        * inspector/protocol/Debugger.json:
+        Clarify in the protocol description the contract of these methods.
+
 2016-11-09  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Associate Worker Resources with the Worker and not the Page
index a43fa24..8b20223 100644 (file)
@@ -34,7 +34,6 @@
 #include "MarkedSpaceInlines.h"
 #include "Parser.h"
 #include "Protect.h"
-#include "VMEntryScope.h"
 
 namespace {
 
index f688249..30880ed 100644 (file)
@@ -478,6 +478,9 @@ void InspectorDebuggerAgent::removeBreakpoint(ErrorString&, const String& breakp
 
 void InspectorDebuggerAgent::continueToLocation(ErrorString& errorString, const InspectorObject& location)
 {
+    if (!assertPaused(errorString))
+        return;
+
     if (m_continueToLocationBreakpointID != JSC::noBreakpointID) {
         m_scriptDebugServer.removeBreakpoint(m_continueToLocationBreakpointID);
         m_continueToLocationBreakpointID = JSC::noBreakpointID;
@@ -492,6 +495,7 @@ void InspectorDebuggerAgent::continueToLocation(ErrorString& errorString, const
     auto scriptIterator = m_scripts.find(sourceID);
     if (scriptIterator == m_scripts.end()) {
         m_scriptDebugServer.continueProgram();
+        m_frontendDispatcher->resumed();
         errorString = ASCIILiteral("No script for id: ") + String::number(sourceID);
         return;
     }
@@ -504,6 +508,7 @@ void InspectorDebuggerAgent::continueToLocation(ErrorString& errorString, const
     resolveBreakpoint(script, breakpoint);
     if (!breakpoint.resolved) {
         m_scriptDebugServer.continueProgram();
+        m_frontendDispatcher->resumed();
         errorString = ASCIILiteral("Could not resolve breakpoint");
         return;
     }
@@ -511,13 +516,20 @@ void InspectorDebuggerAgent::continueToLocation(ErrorString& errorString, const
     bool existing;
     setBreakpoint(breakpoint, existing);
     if (existing) {
+        // There is an existing breakpoint at this location. Instead of
+        // acting like a series of steps, just resume and we will either
+        // hit this new breakpoint or not.
         m_scriptDebugServer.continueProgram();
+        m_frontendDispatcher->resumed();
         return;
     }
 
     m_continueToLocationBreakpointID = breakpoint.id;
 
-    resume(errorString);
+    // Treat this as a series of steps until reaching the new breakpoint.
+    // So don't issue a resumed event unless we exit the VM without pausing.
+    willStepAndMayBecomeIdle();
+    m_scriptDebugServer.continueProgram();
 }
 
 void InspectorDebuggerAgent::searchInContent(ErrorString& error, const String& scriptIDStr, const String& query, const bool* optionalCaseSensitive, const bool* optionalIsRegex, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::GenericTypes::SearchMatch>>& results)
@@ -588,6 +600,7 @@ void InspectorDebuggerAgent::resume(ErrorString& errorString)
         return;
 
     m_scriptDebugServer.continueProgram();
+    m_conditionToDispatchResumed = ShouldDispatchResumed::WhenContinued;
 }
 
 void InspectorDebuggerAgent::stepOver(ErrorString& errorString)
@@ -595,6 +608,7 @@ void InspectorDebuggerAgent::stepOver(ErrorString& errorString)
     if (!assertPaused(errorString))
         return;
 
+    willStepAndMayBecomeIdle();
     m_scriptDebugServer.stepOverStatement();
 }
 
@@ -603,6 +617,7 @@ void InspectorDebuggerAgent::stepInto(ErrorString& errorString)
     if (!assertPaused(errorString))
         return;
 
+    willStepAndMayBecomeIdle();
     m_scriptDebugServer.stepIntoStatement();
 }
 
@@ -611,9 +626,35 @@ void InspectorDebuggerAgent::stepOut(ErrorString& errorString)
     if (!assertPaused(errorString))
         return;
 
+    willStepAndMayBecomeIdle();
     m_scriptDebugServer.stepOutOfFunction();
 }
 
+void InspectorDebuggerAgent::willStepAndMayBecomeIdle()
+{
+    // When stepping the backend must eventually trigger a "paused" or "resumed" event.
+    // If the step causes us to exit the VM, then we should issue "resumed".
+    m_conditionToDispatchResumed = ShouldDispatchResumed::WhenIdle;
+
+    if (!m_registeredIdleCallback) {
+        m_registeredIdleCallback = true;
+        JSC::VM& vm = m_scriptDebugServer.vm();
+        vm.whenIdle([this]() {
+            didBecomeIdleAfterStepping();
+        });
+    }
+}
+
+void InspectorDebuggerAgent::didBecomeIdleAfterStepping()
+{
+    m_registeredIdleCallback = false;
+
+    if (m_conditionToDispatchResumed == ShouldDispatchResumed::WhenIdle)
+        m_frontendDispatcher->resumed();
+
+    m_conditionToDispatchResumed = ShouldDispatchResumed::No;
+}
+
 void InspectorDebuggerAgent::setPauseOnExceptions(ErrorString& errorString, const String& stringPauseState)
 {
     JSC::Debugger::PauseOnExceptionsState pauseState;
@@ -809,7 +850,10 @@ void InspectorDebuggerAgent::didPause(JSC::ExecState& scriptState, JSC::JSValue
         m_hasExceptionValue = true;
     }
 
+    m_conditionToDispatchResumed = ShouldDispatchResumed::No;
+
     m_frontendDispatcher->paused(currentCallFrames(injectedScript), m_breakReason, m_breakAuxData);
+
     m_javaScriptPauseScheduled = false;
 
     if (m_continueToLocationBreakpointID != JSC::noBreakpointID) {
@@ -859,7 +903,8 @@ void InspectorDebuggerAgent::didContinue()
     clearBreakDetails();
     clearExceptionValue();
 
-    m_frontendDispatcher->resumed();
+    if (m_conditionToDispatchResumed == ShouldDispatchResumed::WhenContinued)
+        m_frontendDispatcher->resumed();
 }
 
 void InspectorDebuggerAgent::breakProgram(DebuggerFrontendDispatcher::Reason breakReason, RefPtr<InspectorObject>&& data)
@@ -901,7 +946,10 @@ void InspectorDebuggerAgent::clearDebuggerBreakpointState()
     m_javaScriptPauseScheduled = false;
     m_hasExceptionValue = false;
 
-    m_scriptDebugServer.continueProgram();
+    if (isPaused()) {
+        m_scriptDebugServer.continueProgram();
+        m_frontendDispatcher->resumed();
+    }
 }
 
 void InspectorDebuggerAgent::didClearGlobalObject()
index 999a999..f334813 100644 (file)
@@ -141,6 +141,10 @@ private:
     void clearBreakDetails();
     void clearExceptionValue();
 
+    enum class ShouldDispatchResumed { No, WhenIdle, WhenContinued };
+    void willStepAndMayBecomeIdle();
+    void didBecomeIdleAfterStepping();
+
     RefPtr<InspectorObject> buildBreakpointPauseReason(JSC::BreakpointID);
     RefPtr<InspectorObject> buildExceptionPauseReason(JSC::JSValue exception, const InjectedScript&);
 
@@ -165,11 +169,13 @@ private:
     JSC::BreakpointID m_continueToLocationBreakpointID;
     DebuggerFrontendDispatcher::Reason m_breakReason;
     RefPtr<InspectorObject> m_breakAuxData;
+    ShouldDispatchResumed m_conditionToDispatchResumed { ShouldDispatchResumed::No };
     bool m_enabled { false };
     bool m_javaScriptPauseScheduled { false };
     bool m_hasExceptionValue { false };
     bool m_didPauseStopwatch { false };
     bool m_pauseOnAssertionFailures { false };
+    bool m_registeredIdleCallback { false };
 };
 
 } // namespace Inspector
index b50129c..34e8851 100644 (file)
@@ -45,7 +45,6 @@
 #include "SourceCode.h"
 #include "TypeProfiler.h"
 #include "TypeProfilerLog.h"
-#include "VMEntryScope.h"
 #include <wtf/CurrentTime.h>
 
 using namespace JSC;
index 82e42d6..e58d23f 100644 (file)
             "parameters": [
                 { "name": "location", "$ref": "Location", "description": "Location to continue to." }
             ],
-            "description": "Continues execution until specific location is reached."
+            "description": "Continues execution until specific location is reached. This will trigger either a Debugger.paused or Debugger.resumed event."
         },
         {
             "name": "stepOver",
-            "description": "Steps over the statement."
+            "description": "Steps over the statement. This will trigger either a Debugger.paused or Debugger.resumed event."
         },
         {
             "name": "stepInto",
-            "description": "Steps into the function call."
+            "description": "Steps into the function call. This will trigger either a Debugger.paused or Debugger.resumed event."
         },
         {
             "name": "stepOut",
-            "description": "Steps out of the function call."
+            "description": "Steps out of the function call. This will trigger either a Debugger.paused or Debugger.resumed event."
         },
         {
             "name": "pause",
         },
         {
             "name": "resume",
-            "description": "Resumes JavaScript execution."
+            "description": "Resumes JavaScript execution. This will trigger a Debugger.resumed event."
         },
         {
             "name": "searchInContent",
index 1be0585..29c53fd 100644 (file)
@@ -1,5 +1,24 @@
 2016-11-09  Joseph Pecoraro  <pecoraro@apple.com>
 
+        Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
+        https://bugs.webkit.org/show_bug.cgi?id=161951
+        <rdar://problem/28295767>
+
+        Reviewed by Brian Burg.
+
+        Covered by existing tests that would ASSERT otherwise.
+
+        * inspector/InspectorClient.cpp:
+        (WebCore::InspectorClient::doDispatchMessageOnFrontendPage):
+        When paused on an exception in the inspected page and evaluating
+        commands in the inspector frontend page (which evaluates JavaScript)
+        we ASSERT when entering the Global DOM VM with an existing exception.
+        This makes it so when we evaluate JavaScript in the frontend we
+        suspend / ignore the state of the VM for the inspected page, and
+        restore it when we return from the inspector.
+
+2016-11-09  Joseph Pecoraro  <pecoraro@apple.com>
+
         Web Inspector: Associate Worker Resources with the Worker and not the Page
         https://bugs.webkit.org/show_bug.cgi?id=164342
         <rdar://problem/29075775>
index 4ae763d..21ee2b3 100644 (file)
 #include "config.h"
 #include "InspectorClient.h"
 
+#include "InspectorController.h"
 #include "MainFrame.h"
 #include "Page.h"
 #include "ScriptController.h"
 #include "ScriptSourceCode.h"
+#include <interpreter/FrameTracers.h>
 
+using namespace JSC;
 using namespace Inspector;
 
 namespace WebCore {
@@ -45,6 +48,7 @@ void InspectorClient::doDispatchMessageOnFrontendPage(Page* frontendPage, const
     if (!frontendPage)
         return;
 
+    SuspendExceptionScope scope(&frontendPage->inspectorController().vm());
     String dispatchToFrontend = makeString("InspectorFrontendAPI.dispatchMessageAsync(", message, ");");
     frontendPage->mainFrame().script().evaluate(ScriptSourceCode(dispatchToFrontend));
 }
index 0d2a3ab..c8796e4 100644 (file)
@@ -1,5 +1,29 @@
 2016-11-09  Joseph Pecoraro  <pecoraro@apple.com>
 
+        Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
+        https://bugs.webkit.org/show_bug.cgi?id=161951
+        <rdar://problem/28295767>
+
+        Reviewed by Brian Burg.
+
+        * UserInterface/Controllers/DebuggerManager.js:
+        (WebInspector.DebuggerManager.prototype.debuggerDidResume):
+        Now, Debugger.resumed events really mean the debugger resumed,
+        so act immediately instead of guessing. We must still guess
+        in legacy backends.
+
+        * UserInterface/Test/Test.js:
+        When the inspector frontend encounters an issue, log it.
+
+        * UserInterface/Views/DebuggerSidebarPanel.js:
+        (WebInspector.DebuggerSidebarPanel.prototype._debuggerDidPause):
+        (WebInspector.DebuggerSidebarPanel.prototype._debuggerActiveCallFrameDidChange):
+        Always enable the step out button. I don't think it makes sense to disable
+        it sometimes, and if there are issues with this we should solve the issues
+        instead of hiding them.
+
+2016-11-09  Joseph Pecoraro  <pecoraro@apple.com>
+
         Web Inspector: Associate Worker Resources with the Worker and not the Page
         https://bugs.webkit.org/show_bug.cgi?id=164342
         <rdar://problem/29075775>
index 0ff31e4..ef2b109 100644 (file)
@@ -611,9 +611,18 @@ WebInspector.DebuggerManager = class DebuggerManager extends WebInspector.Object
     {
         // Called from WebInspector.DebuggerObserver.
 
-        // We delay clearing the state and firing events so the user interface does not flash
-        // between brief steps or successive breakpoints.
-        this._delayedResumeTimeout = setTimeout(this._didResumeInternal.bind(this, target), 50);
+        // COMPATIBILITY (iOS 10): Debugger.resumed event was ambiguous. When stepping
+        // we would receive a Debugger.resumed and we would not know if it really meant
+        // the backend resumed or would pause again due to a step. Legacy backends wait
+        // 50ms, and treat it as a real resume if we haven't paused in that time frame.
+        // This delay ensures the user interface does not flash between brief steps
+        // or successive breakpoints.
+        if (!DebuggerAgent.setPauseOnAssertions) {
+            this._delayedResumeTimeout = setTimeout(this._didResumeInternal.bind(this, target), 50);
+            return;
+        }
+
+        this._didResumeInternal(target);
     }
 
     playBreakpointActionSound(breakpointActionIdentifier)
index 74cbadb..9ca29a7 100644 (file)
@@ -113,3 +113,5 @@ WebInspector.updateVisibilityState = () => {};
 window.InspectorTest = new FrontendTestHarness();
 
 InspectorTest.redirectConsoleToTestOutput();
+
+WebInspector.reportInternalError = (e) => { console.error(e); }
index a9223e3..90b1984 100644 (file)
@@ -328,6 +328,7 @@ WebInspector.DebuggerSidebarPanel = class DebuggerSidebarPanel extends WebInspec
         this._debuggerPauseResumeButtonItem.toggled = true;
         this._debuggerStepOverButtonItem.enabled = true;
         this._debuggerStepIntoButtonItem.enabled = true;
+        this._debuggerStepOutButtonItem.enabled = true;
 
         this.element.classList.add(WebInspector.DebuggerSidebarPanel.DebuggerPausedStyleClassName);
     }
@@ -669,13 +670,6 @@ WebInspector.DebuggerSidebarPanel = class DebuggerSidebarPanel extends WebInspec
 
         if (this._activeCallFrameTreeElement)
             this._activeCallFrameTreeElement.isActiveCallFrame = true;
-
-        // FIXME: What is this, and is it still relevant?
-        // It is useful to turn off the step out button when there is no call frame to go through
-        // since there might be call frames in the backend that were removed when processing the call
-        // frame payload.
-        let indexOfActiveCallFrame = callFrames.indexOf(WebInspector.debuggerManager.activeCallFrame);
-        this._debuggerStepOutButtonItem.enabled = indexOfActiveCallFrame < callFrames.length - 1;
     }
 
     _breakpointsBeneathTreeElement(treeElement)