789430 - Pause on next bytecode instead of immediately

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Firebug and Chrome use the pause button to enter the debugger in the next executed JavaScript bytecode (displaying a "pausing..." label until that happens). This is more useful than our current behavior of pausing immediately when there is no JavaScript code running in the event loop. See also bug 785689 comment 6.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I'll work on this next week.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Excellent, thanks!

Honza

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

This implements Firebug's break-on-next behavior by modifying the "interrupt" request and response somewhat, instead of adding a completely new request.

The existing interrupt packet is still used for an immediate pause:

{ "to":thread, "type":"interrupt" }

Adding a new "when" parameter indicates the condition on which execution should pause:

{ "to":thread, "type":"interrupt", "when":"onNext" }

My goal was to mirror the "resumeLimit" functionality in a way that allows further conditions to be added in the future. When the thread actor receives such a packet, will reply immediately with a packet of the form:

{ "from":<i>actor</i>, "type":"willInterrupt" }

And when execution actually pauses, the pause packet will have a reason of:

{ ..., "why":{ "type":"interrupted" } }

...just like the immediate pause without the "when" condition.

If this backend behavior is OK, I'll work on the UI a bit (I need to figure out how to change the pause/resume button to indicate that execution is not paused, but will pause soon) and modify the tests accordingly.

Comment 4

[Jim Blandy :jimb]

Sure, that seems fine. "interrupt" packets are always the right thing for messing with a thread while it's in the "Running" state.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So this would break on the next timer or rAF or event listener or whatever? If so, +1!

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: pause.patch

Rebased and includes a simple frontend change to show a different button state while waiting for the next execution.  Looks like there's been a function added on the thread called `breakOnNext` in the meantime [0], but it's name seems wrong since it seems to be about resuming and not breaking AFAICT.

[0]: https://dxr.mozilla.org/mozilla-central/search?q=breakonnext&redirect=true&case=false&limit=60&offset=0

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: pause.patch

had the old patch applied locally when exporting the last one so it was missing a bit, this one should be right

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8653149 [details]
MozReview Request: Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

https://reviewboard.mozilla.org/r/17379/#review15505

Overall, this looks really good, and I have an r+ deep in my heart, but I'm not quite ready to share it yet.

I think we need some new tests specifically for this new behavior (was originally thinking we could modify browser_dbg_interrupts.js but I think it might be nice to start afresh). 

* At the basic sanity level, I'd like to have a test does a setTimeout/setInterval/requestAnimationFrame and then sets a pause-on-next-execution and asserts that we pause in the right source and location when the JS runs.
* The other test I'd like to have is for interaction with the console. Do we pause on the next console eval? Ignore console evals? Execute frames until (if) we get to a frame from a function that is not from the console source but instead from one of the page's "actual" sources we list in the sidebar? (Might be worth investigating chrome's behavior here instead of reinventing the user experience)
* Related to the above: What happens when the next JS to execute is an eval source without a `//# sourceURL=...` (meaning that we are not showing it from the sources list)? We should probably make the same choice we made with exceptions in such sources when pause-on-exceptions is enabled (I don't remember what that choice is, would need to look it up). Either way, we should have an answer to this question in the form of a test.

::: browser/devtools/debugger/test/browser_dbg_interrupts.js:98
(Diff revision 1)
> +    evalInTab(gTab, "1+1;");



::: toolkit/devtools/server/actors/script.js:1037
(Diff revision 1)
> -      // Tell anyone who cares of the resume (as of now, that's the xpcshell
> +      // Tell subscribers about the resume (used by tool frontends and tests)

I hope this is **not** used by tool frontends! They should be using the RDP (and we should be propagating toolbox level events if needed). We definitely shouldn't be reaching around the RDP in frontends in a way that will break e10s/remote debugging.

Luckily, I think this comment is just incorrect regarding frontends and we should fix it.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9)
> Comment on attachment 8653149 [details]
> MozReview Request: Bug 789430 - Pause on next bytecode instead of
> immediately;r=fitzgen
> 
> https://reviewboard.mozilla.org/r/17379/#review15505
> 
> Overall, this looks really good, and I have an r+ deep in my heart, but I'm
> not quite ready to share it yet.
> 
> I think we need some new tests specifically for this new behavior (was
> originally thinking we could modify browser_dbg_interrupts.js but I think it
> might be nice to start afresh). 
> 
> * At the basic sanity level, I'd like to have a test does a
> setTimeout/setInterval/requestAnimationFrame and then sets a
> pause-on-next-execution and asserts that we pause in the right source and
> location when the JS runs.

Sure, I'll add a test for that.

> * The other test I'd like to have is for interaction with the console. Do we
> pause on the next console eval? Ignore console evals? Execute frames until
> (if) we get to a frame from a function that is not from the console source
> but instead from one of the page's "actual" sources we list in the sidebar?
> (Might be worth investigating chrome's behavior here instead of reinventing
> the user experience)

It pauses on the next console eval by adding an anonymous source (i.e. SCRIPT 0) with the console evaluation.  With Chrome, it actually pauses on the first character typed into the console inside an internal `evaluate: function(expression, objectGroup, injectCommandLineAPI, returnByValue, generatePreview)` functionality.  Our behavior with the patch applied fits my expectations better.

> * Related to the above: What happens when the next JS to execute is an eval
> source without a `//# sourceURL=...` (meaning that we are not showing it
> from the sources list)? We should probably make the same choice we made with
> exceptions in such sources when pause-on-exceptions is enabled (I don't
> remember what that choice is, would need to look it up). Either way, we
> should have an answer to this question in the form of a test.

Same behavior as console input - it adds a SCRIPT 0 under Anonymous Sources and then pauses in that.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9)
> ::: toolkit/devtools/server/actors/script.js:1037
> (Diff revision 1)
> > -      // Tell anyone who cares of the resume (as of now, that's the xpcshell
> > +      // Tell subscribers about the resume (used by tool frontends and tests)
> 
> I hope this is **not** used by tool frontends! They should be using the RDP
> (and we should be propagating toolbox level events if needed). We definitely
> shouldn't be reaching around the RDP in frontends in a way that will break
> e10s/remote debugging.
> 
> Luckily, I think this comment is just incorrect regarding frontends and we
> should fix it.

Yeah the comment is wrong about that.  The call to notifyObservers specifically is only used in xpcshell tests

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9)
> > * The other test I'd like to have is for interaction with the console. Do we
> > pause on the next console eval? Ignore console evals? Execute frames until
> > (if) we get to a frame from a function that is not from the console source
> > but instead from one of the page's "actual" sources we list in the sidebar?
> > (Might be worth investigating chrome's behavior here instead of reinventing
> > the user experience)
> 
> It pauses on the next console eval by adding an anonymous source (i.e.
> SCRIPT 0) with the console evaluation.  With Chrome, it actually pauses on
> the first character typed into the console inside an internal `evaluate:
> function(expression, objectGroup, injectCommandLineAPI, returnByValue,
> generatePreview)` functionality.  Our behavior with the patch applied fits
> my expectations better.
> 
> > * Related to the above: What happens when the next JS to execute is an eval
> > source without a `//# sourceURL=...` (meaning that we are not showing it
> > from the sources list)? We should probably make the same choice we made with
> > exceptions in such sources when pause-on-exceptions is enabled (I don't
> > remember what that choice is, would need to look it up). Either way, we
> > should have an answer to this question in the form of a test.
> 
> Same behavior as console input - it adds a SCRIPT 0 under Anonymous Sources
> and then pauses in that.

Great! That's a lot better than I expected. Please add tests so that this behavior doesn't break :)

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8653149 [details]
MozReview Request: Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8653149 [details]
> MozReview Request: Bug 789430 - Pause on next bytecode instead of
> immediately;r=fitzgen
> 
> Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

I'm still trying to sort out some issue on try with break-on-next-console.js, but I think the patch should address the issues you mentioned.  Let me know if you think we should handle these tests differently.  Also note that the interdiff in reviewboard for existing tests will be a little weird, because they moved from browser/devtools/debugger/test to browser/devtools/debugger/test/mochitest in the mean time.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8653149 [details]
MozReview Request: Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen

https://reviewboard.mozilla.org/r/17379/#review16071

LGTM!

::: browser/devtools/debugger/test/mochitest/browser_dbg_break-on-next.js:7
(Diff revision 2)
> + * into the console in the toolbox.

Awesome test! Thanks!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c151e3624f32

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c151e3624f32