Allow setting breakpoints in pretty-printed source

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

6 months ago
Posted patch patchSplinter Review
A few parts of the Debugger interface are unimplemented by ReplayDebugger, which prevents breakpoints from being installed in sources that have been converted by the pretty printer (and probably other generated sources).  The attached patch fixes these implementation gaps and the above problem.
Attachment #9033575 - Flags: review?(lsmyth)
Comment on attachment 9033575 [details] [diff] [review]
patch

Review of attachment 9033575 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/replay/debugger.js
@@ +451,4 @@
>    },
>  
>    findScripts(query) {
> +    this._ensurePaused();

We recently landed https://bugzilla.mozilla.org/show_bug.cgi?id=1510463, so we aren't always paused when a breakpoint is added. Assuming this assertion is declaring an existing limitation, does this mean that users will need to manually pause before they can add a breakpoint?
Assignee

Comment 2

6 months ago
(In reply to Logan Smyth [:loganfsmyth] from comment #1)
> Comment on attachment 9033575 [details] [diff] [review]
> patch
> 
> Review of attachment 9033575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/replay/debugger.js
> @@ +451,4 @@
> >    },
> >  
> >    findScripts(query) {
> > +    this._ensurePaused();
> 
> We recently landed https://bugzilla.mozilla.org/show_bug.cgi?id=1510463, so
> we aren't always paused when a breakpoint is added. Assuming this assertion
> is declaring an existing limitation, does this mean that users will need to
> manually pause before they can add a breakpoint?

No, users will not need to manually pause for this.  This _ensurePaused call relates to whether the active child process is paused and is able to receive debugger messages --- web replay child processes have a restriction that messages can only be sent to them when they are paused at a checkpoint or breakpoint, so that we don't have to deal with situations such as the parent sending a message while the child is in the middle of rewinding.  To allow the server code to interact with the debuggee regardless of whether it is paused, we use _ensurePaused to wait until the child process reaches the next point it can pause at, do the operation, and then resume executing in the same direction it was traveling previously.  The latter bit is handled at the end of _performPause, which will be called soon after the pause happens via the event loop.
Comment on attachment 9033575 [details] [diff] [review]
patch

Review of attachment 9033575 [details] [diff] [review]:
-----------------------------------------------------------------

Ahh perfect, looks good then.
Attachment #9033575 - Flags: review?(lsmyth) → review+

Comment 4

5 months ago
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9a7aa1898c
Implement a couple ReplayDebugger interfaces, r=lsmyth.

Backed out 12 changesets (bug 1516578, bug 1513118, bug 1516694) for failing at browser_toolbox_remoteness_change.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3564442734c89fa1b735ff8662588576cf0115

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=4abb81088a9b49d700cfea840848a9dac6a0010d&selectedJob=221519592

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221519592&repo=mozilla-inbound&lineNumber=9675

Log snippet:

[task 2019-01-12T21:42:48.171Z] 21:42:48 INFO - GECKO(1484) | nsStringStats
[task 2019-01-12T21:42:48.171Z] 21:42:48 INFO - GECKO(1484) | => mAllocCount: 1939881
[task 2019-01-12T21:42:48.173Z] 21:42:48 INFO - GECKO(1484) | => mReallocCount: 1
[task 2019-01-12T21:42:48.173Z] 21:42:48 INFO - GECKO(1484) | => mFreeCount: 1939881
[task 2019-01-12T21:42:48.174Z] 21:42:48 INFO - GECKO(1484) | => mShareCount: 1889903
[task 2019-01-12T21:42:48.175Z] 21:42:48 INFO - GECKO(1484) | => mAdoptCount: 67026
[task 2019-01-12T21:42:48.176Z] 21:42:48 INFO - GECKO(1484) | => mAdoptFreeCount: 67986
[task 2019-01-12T21:42:48.177Z] 21:42:48 INFO - GECKO(1484) | => Process ID: 1484, Thread ID: 139783454422848
[task 2019-01-12T21:42:48.210Z] 21:42:48 INFO - TEST-INFO | Main app process: exit 0
[task 2019-01-12T21:42:48.211Z] 21:42:48 ERROR - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_remoteness_change.js | leaked 4 window(s) until shutdown [url = about:blank]
[task 2019-01-12T21:42:48.212Z] 21:42:48 ERROR - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_remoteness_change.js | leaked 1 window(s) until shutdown [url = chrome://devtools/content/webconsole/index.html]
[task 2019-01-12T21:42:48.222Z] 21:42:48 ERROR - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_remoteness_change.js | leaked 1 window(s) until shutdown [url = about:devtools-toolbox]
[task 2019-01-12T21:42:48.223Z] 21:42:48 INFO - TEST-INFO | devtools/client/framework/test/browser_toolbox_remoteness_change.js | windows(s) leaked: [pid = 1484] [serial = 492], [pid = 1484] [serial = 490], [pid = 1484] [serial = 489], [pid = 1484] [serial = 491], [pid = 1484] [serial = 488], [pid = 1484] [serial = 487]
[task 2019-01-12T21:42:48.224Z] 21:42:48 ERROR - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_remoteness_change.js | leaked 2 docShell(s) until shutdown
[task 2019-01-12T21:42:48.224Z] 21:42:48 INFO - TEST-INFO | devtools/client/framework/test/browser_toolbox_remoteness_change.js | docShell(s) leaked: [pid = 1484] [id = {2478efcb-416b-4465-ac17-c720bf9ea414}], [pid = 1484] [id = {4df99fc4-7efa-48f0-b186-723fe21e3008}]
[task 2019-01-12T21:42:48.226Z] 21:42:48 INFO - runtests.py | Application ran for: 0:15:58.112473
[task 2019-01-12T21:42:48.228Z] 21:42:48 INFO - zombiecheck | Reading PID log: /tmp/tmpCS3TLopidlog

Flags: needinfo?(bhackett1024)

Comment 6

5 months ago
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b80743d650
Implement a couple ReplayDebugger interfaces, r=lsmyth.
Assignee

Comment 7

5 months ago

I don't think anything in these patches has anything to do with the browser_toolbox_remoteness_change.js failure: when I test locally, browser_toolbox_remoteness_change.js leaks windows both with and without these changes. The try push below contains two out of three of the bugs whose patches were in the earlier push, and doesn't have any browser_toolbox_remoteness_change.js failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=191ee7f02a12

Flags: needinfo?(bhackett1024)

Comment 8

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.