Last Comment Bug 789430 - Pause on next bytecode instead of immediately
: Pause on next bytecode instead of immediately
Status: RESOLVED FIXED
[firebug-p2]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 43
Assigned To: Brian Grinstead [:bgrins]
:
Mentors:
Depends on: 1191916
Blocks: dbg-control
  Show dependency treegraph
 
Reported: 2012-09-07 03:49 PDT by Panos Astithas [:past]
Modified: 2016-03-30 12:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Patch v1 (6.38 KB, patch)
2013-06-11 07:43 PDT, Panos Astithas [:past]
jimb: feedback+
Details | Diff | Splinter Review
pause.patch (7.34 KB, patch)
2015-08-13 10:23 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review
pause.patch (7.97 KB, patch)
2015-08-13 10:27 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review
MozReview Request: Bug 789430 - Pause on next bytecode instead of immediately;r=fitzgen (40 bytes, text/x-review-board-request)
2015-08-26 15:34 PDT, Brian Grinstead [:bgrins]
nfitzgerald: review+
Details | Review

Description Panos Astithas [:past] 2012-09-07 03:49:42 PDT
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 [:past] 2013-06-07 08:11:40 PDT
I'll work on this next week.
Comment 2 Jan Honza Odvarko [:Honza] 2013-06-07 08:12:53 PDT
Excellent, thanks!

Honza
Comment 3 Panos Astithas [:past] 2013-06-11 07:43:25 PDT
Created attachment 760918 [details] [diff] [review]
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 2013-07-29 16:38:17 PDT
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] [⏰PDT; UTC-7] 2014-06-05 14:49:32 PDT
So this would break on the next timer or rAF or event listener or whatever? If so, +1!
Comment 6 Brian Grinstead [:bgrins] 2015-08-13 10:23:37 PDT
Created attachment 8647601 [details] [diff] [review]
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] 2015-08-13 10:27:49 PDT
Created attachment 8647604 [details] [diff] [review]
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] 2015-08-26 15:34:42 PDT
Created 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 9 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2015-08-27 05:45:33 PDT
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] 2015-08-27 09:40:02 PDT
(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] 2015-08-27 09:42:43 PDT
(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] [⏰PDT; UTC-7] 2015-08-27 11:11:52 PDT
(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] 2015-09-01 13:30:02 PDT
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] 2015-09-01 13:31:46 PDT
(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] [⏰PDT; UTC-7] 2015-09-01 14:06:42 PDT
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 17 Carsten Book [:Tomcat] - PTO-back Sept 4th 2015-09-03 04:18:15 PDT
https://hg.mozilla.org/mozilla-central/rev/c151e3624f32

Note You need to log in before you can comment on or make changes to this bug.