Closed Bug 1516694 Opened 6 years ago Closed 6 years ago

Allow setting breakpoints in pretty-printed source

Categories

(Core Graveyard :: Web Replay, defect)

Product:
Core Graveyard
Component:
Web Replay
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

patch
(deleted), patch
loganfsmyth
: review+
Details | Diff | Splinter Review
Attached patch patch (deleted) — — Splinter 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?
(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+
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)
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/25b80743d650 Implement a couple ReplayDebugger interfaces, r=lsmyth.

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)

https://hg.mozilla.org/mozilla-central/rev/25b80743d650

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: