DevTools: Fix disabling all pauses on reload.

DevTools: Fix disabling all pauses on reload.

BUG=115105, 286081
R=yurys@chromium.org,johnjbarton@chromium.org,pfeldman

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161920

[yurys, 2013-11-08 06:38:26]

https://codereview.chromium.org/60203007/diff/1/LayoutTests/http/tests/inspec...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/60203007/diff/1/LayoutTests/http/tests/inspec...
LayoutTests/http/tests/inspector/debugger-test.js:86:
InspectorTest.waitUntilPaused = function(callback, forceWait)
I'd rather have separate methods for the forced case:
waitUntilPausedNextTime/waitUntilResumedNextTime it may be more clear, wdyt?

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
File LayoutTests/inspector/debugger/skip-pauses-until-reload.html (right):

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:65: // FIXME: Test
is flaky with PauseOnAllExceptions due to races in debugger.
Wouldn't it flake with PauseOnUncaughExceptions, what's the source of the
flakiness?

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:87:
InspectorTest.runAfterPendingDispatches(didSetUp);
Can we listen to the setBreakpoint callback instead?

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:111:
InspectorTest.waitUntilPaused(didPauseAfterReload, true);
I wonder why we need this force argument passed to waitUntilPaused, we should
know whether we are on pause when this method is executed or not and in the
former case send continue command and do reload in its callback. Would that
work?

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:114: function
didPageReloaded()
didReloadPage

https://codereview.chromium.org/60203007/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/60203007/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorDebuggerAgent.cpp:1028: if (m_skipAllPauses)
Why shouldSkipBreakpointPause/shouldSkipStepPause are not enough? AFAIU we
shouldn't have come here if those methods work correctly.

[aandrey, 2013-11-08 09:10:39]

PTAL

https://codereview.chromium.org/60203007/diff/1/LayoutTests/http/tests/inspec...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/60203007/diff/1/LayoutTests/http/tests/inspec...
LayoutTests/http/tests/inspector/debugger-test.js:86:
InspectorTest.waitUntilPaused = function(callback, forceWait)
On 2013/11/08 06:38:26, yurys wrote:
> I'd rather have separate methods for the forced case:
> waitUntilPausedNextTime/waitUntilResumedNextTime it may be more clear, wdyt?

Done.

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
File LayoutTests/inspector/debugger/skip-pauses-until-reload.html (right):

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:65: // FIXME: Test
is flaky with PauseOnAllExceptions due to races in debugger.
On 2013/11/08 06:38:26, yurys wrote:
> Wouldn't it flake with PauseOnUncaughExceptions, what's the source of the
> flakiness?

Maybe. Let me put DontPauseOnExceptions for now.

The source of flakiness should be investigated deeper.
What I see is the following sequence of events:
- pause on exception happens at eval("throwException()");
- the ScriptDebugServer::handleV8DebugEvent called, the pause is skipped by
shouldSkipExceptionPause() on the top call frame (this test)
- didCommitLoad and didClearWindowObjectInWorld happens (thus m_skipAllPauses
gets cleared)
- the ScriptDebugServer::handleV8DebugEvent called for the same exception, but
with the top call frame in the VM script (InjectedScript):
  top frame: InjectedScriptSource.js, method _evaluateOn line 603:
    var result = evalFunction.call(object, expression);
- this pause is not skipped

Probably the code is not patched back in V8 not to break on the current
exception any more if there is a CEntry stack frame?
The "evalFunction.call" - is the call to native InjectedScriptHost.evaluate

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:87:
InspectorTest.runAfterPendingDispatches(didSetUp);
On 2013/11/08 06:38:26, yurys wrote:
> Can we listen to the setBreakpoint callback instead?

why? this works like a charm and more stable than sniffing a callback (e.g. if
the callback is already bound, the sniffing will not work).
am I missing smth?

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:111:
InspectorTest.waitUntilPaused(didPauseAfterReload, true);
On 2013/11/08 06:38:26, yurys wrote:
> I wonder why we need this force argument passed to waitUntilPaused, we should
> know whether we are on pause when this method is executed or not and in the
> former case send continue command and do reload in its callback. Would that
> work?

- we are paused at this point, thus waitUntilPaused w/o the flag will call the
callback right away
- if didPauseAfterReload is called the test is FAIL'ed. I do want to see where
it stopped and why.

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:114: function
didPageReloaded()
On 2013/11/08 06:38:26, yurys wrote:
> didReloadPage

Done.

https://codereview.chromium.org/60203007/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/60203007/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorDebuggerAgent.cpp:1028: if (m_skipAllPauses)
On 2013/11/08 06:38:26, yurys wrote:
> Why shouldSkipBreakpointPause/shouldSkipStepPause are not enough? AFAIU we
> shouldn't have come here if those methods work correctly.

The shouldSkip* methods are listener callbacks called from V8 debugger.

The breakProgram() is called from our backend to break the program on
DOM/Event/Xhr breakpoints, console.assert and maybe others, and does break it
w/o checking the shouldSkip* callbacks.

[yurys, 2013-11-13 12:26:01]

lgtm

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
File LayoutTests/inspector/debugger/skip-pauses-until-reload.html (right):

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:65: // FIXME: Test
is flaky with PauseOnAllExceptions due to races in debugger.
On 2013/11/08 09:10:39, aandrey wrote:
> On 2013/11/08 06:38:26, yurys wrote:
> > Wouldn't it flake with PauseOnUncaughExceptions, what's the source of the
> > flakiness?
> 
> Maybe. Let me put DontPauseOnExceptions for now.
> 
> The source of flakiness should be investigated deeper.
> What I see is the following sequence of events:
> - pause on exception happens at eval("throwException()");
> - the ScriptDebugServer::handleV8DebugEvent called, the pause is skipped by
> shouldSkipExceptionPause() on the top call frame (this test)
> - didCommitLoad and didClearWindowObjectInWorld happens (thus m_skipAllPauses
> gets cleared)
> - the ScriptDebugServer::handleV8DebugEvent called for the same exception, but
> with the top call frame in the VM script (InjectedScript):
>   top frame: InjectedScriptSource.js, method _evaluateOn line 603:
>     var result = evalFunction.call(object, expression);
> - this pause is not skipped
> 
> Probably the code is not patched back in V8 not to break on the current
> exception any more if there is a CEntry stack frame?
> The "evalFunction.call" - is the call to native InjectedScriptHost.evaluate
How come didCommitLoad and didClearWindowObjectInWorld happen in between the
moment when the exception is thrown in throwException() and handled inside
InjectedScript.evaluate?

Pause on exceptions can be addressed in another change but we need to address
this.

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:111:
InspectorTest.waitUntilPaused(didPauseAfterReload, true);
On 2013/11/08 09:10:39, aandrey wrote:
> On 2013/11/08 06:38:26, yurys wrote:
> > I wonder why we need this force argument passed to waitUntilPaused, we
should
> > know whether we are on pause when this method is executed or not and in the
> > former case send continue command and do reload in its callback. Would that
> > work?
> 
> - we are paused at this point, thus waitUntilPaused w/o the flag will call the
> callback right away
> - if didPauseAfterReload is called the test is FAIL'ed. I do want to see where
> it stopped and why.
I was wondering why not simply resume execution if we know that we are paused
but in that case you would pause on the next breakpoint before reload begins so
waitUntilNextPause makes sense to me.

[aandrey, 2013-11-13 13:06:08]

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
File LayoutTests/inspector/debugger/skip-pauses-until-reload.html (right):

https://codereview.chromium.org/60203007/diff/1/LayoutTests/inspector/debugge...
LayoutTests/inspector/debugger/skip-pauses-until-reload.html:65: // FIXME: Test
is flaky with PauseOnAllExceptions due to races in debugger.
On 2013/11/13 12:26:01, yurys wrote:
> On 2013/11/08 09:10:39, aandrey wrote:
> > On 2013/11/08 06:38:26, yurys wrote:
> > > Wouldn't it flake with PauseOnUncaughExceptions, what's the source of the
> > > flakiness?
> > 
> > Maybe. Let me put DontPauseOnExceptions for now.
> > 
> > The source of flakiness should be investigated deeper.
> > What I see is the following sequence of events:
> > - pause on exception happens at eval("throwException()");
> > - the ScriptDebugServer::handleV8DebugEvent called, the pause is skipped by
> > shouldSkipExceptionPause() on the top call frame (this test)
> > - didCommitLoad and didClearWindowObjectInWorld happens (thus
m_skipAllPauses
> > gets cleared)
> > - the ScriptDebugServer::handleV8DebugEvent called for the same exception,
but
> > with the top call frame in the VM script (InjectedScript):
> >   top frame: InjectedScriptSource.js, method _evaluateOn line 603:
> >     var result = evalFunction.call(object, expression);
> > - this pause is not skipped
> > 
> > Probably the code is not patched back in V8 not to break on the current
> > exception any more if there is a CEntry stack frame?
> > The "evalFunction.call" - is the call to native InjectedScriptHost.evaluate
> How come didCommitLoad and didClearWindowObjectInWorld happen in between the
> moment when the exception is thrown in throwException() and handled inside
> InjectedScript.evaluate?
> 
> Pause on exceptions can be addressed in another change but we need to address
> this.

I did not know either. As I said it should be investigated deeper.

[commit-bot: I haz the power, 2013-11-13 13:06:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aandrey@chromium.org/60203007/80001

[commit-bot: I haz the power, 2013-11-13 14:51:55]

Change committed as 161920