Pause on next bytecode instead of immediately

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: past, Assigned: bgrins)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [firebug-p2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

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.
Assignee: nobody → past
Priority: -- → P2
I'll work on this next week.
Status: NEW → ASSIGNED
Excellent, thanks!

Honza
Whiteboard: [firebug-p2]
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.
Attachment #760918 - Flags: feedback?(jimb)

Comment 4

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

Updated

4 years ago
Attachment #760918 - Flags: feedback?(jimb) → feedback+
So this would break on the next timer or rAF or event listener or whatever? If so, +1!

Updated

3 years ago
Summary: Make the "pause" button behave like Firebug's "Break on next" → Pause on next bytecode instead of immediately

Updated

3 years ago
Blocks: 1074488
(Assignee)

Comment 6

2 years ago
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
Attachment #760918 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
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
Attachment #8647601 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1191916
(Assignee)

Comment 8

2 years ago
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
Attachment #8653149 - Flags: review?(nfitzgerald)
(Assignee)

Updated

2 years ago
Assignee: past → bgrinstead
Attachment #8653149 - Flags: review?(nfitzgerald)
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.
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
(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
(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 :)
(Assignee)

Comment 13

2 years ago
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
Attachment #8653149 - Flags: review?(nfitzgerald)
(Assignee)

Comment 14

2 years ago
(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 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!
Attachment #8653149 - Flags: review?(nfitzgerald) → review+

Comment 16

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/c151e3624f32
https://hg.mozilla.org/mozilla-central/rev/c151e3624f32
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(Assignee)

Updated

a year ago
See Also: → bug 1241961

Updated

8 months ago
Depends on: 1326786

Updated

8 months ago
Depends on: 1327011
You need to log in before you can comment on or make changes to this bug.