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)
commit1dfbe36109cba535ff482f62f61576ab8a951e02
tree73e6c94e6e65761fb2851bd5d3939937a2a5f82a
parentf2629566c52aceab2e9d820e73dd7f7e76629df0
Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
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: https://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