Closed
Bug 1498012
Opened 2 years ago
Closed 2 years ago
Fix ReplayDebugger problems when searching for scripts and enumerating object properties
Categories
(Core Graveyard :: Web Replay, enhancement)
Core Graveyard
Web Replay
Tracking
(firefox64 fixed)
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(3 files, 2 obsolete files)
3.64 KB,
patch
|
loganfsmyth
:
review+
|
Details | Diff | Splinter Review |
558 bytes,
patch
|
loganfsmyth
:
review+
|
Details | Diff | Splinter Review |
629 bytes,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
The attached patch fixes a couple problems I noticed while working on bug 1497393: - Handle the query argument to ReplayDebugger.findScripts. Currently we ignore this argument, which breaks callers and causes e.g. setting a breakpoint on some line in a file to set a breakpoint for that line in *every* file. - Fill in the contents of property descriptors on demand. When the ReplayDebugger fetches the properties of an object from the replaying process, it immediately generates ReplayDebugger.Objects for all the other objects referred to by those properties (object valued properties, getter properties, etc.). This can require hundreds or thousands of IPC calls to the replaying process for large objects (like windows), whose results will not be used if the server code is only interested in a few specific properties. By waiting until a specific query is made on a property before instantiating its property descriptor, we avoid this potentially unnecessary IPC.
Attachment #9016059 -
Flags: review?(lsmyth)
Comment 1•2 years ago
|
||
Comment on attachment 9016059 [details] [diff] [review] patch Review of attachment 9016059 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/replay/debugger.js @@ +183,5 @@ > const data = this._sendRequest({ type: "findScripts" }); > + const result = []; > + data.forEach(scriptData => { > + const script = this._addScript(scriptData); > + if (script._matchesQuery(query)) { Why do we have to re-implement this matching instead of passing the query through in the request?
Assignee | ||
Comment 2•2 years ago
|
||
(In reply to Logan Smyth [:loganfsmyth] from comment #1) > Comment on attachment 9016059 [details] [diff] [review] > patch > > Review of attachment 9016059 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/server/actors/replay/debugger.js > @@ +183,5 @@ > > const data = this._sendRequest({ type: "findScripts" }); > > + const result = []; > > + data.forEach(scriptData => { > > + const script = this._addScript(scriptData); > > + if (script._matchesQuery(query)) { > > Why do we have to re-implement this matching instead of passing the query > through in the request? We could pass the query through, but we would have to encode and decode components of the query that refer to things in the debugger server --- the .global and .source properties. That will need new logic but it's stuff we'll need in other places in the future and does seem like a better design, especially in fixing the not-yet-implemented bits. I'll put together a new patch.
Assignee | ||
Comment 3•2 years ago
|
||
Updated patch per comments.
Attachment #9016059 -
Attachment is obsolete: true
Attachment #9016059 -
Flags: review?(lsmyth)
Attachment #9016092 -
Flags: review?(lsmyth)
Comment 4•2 years ago
|
||
Comment on attachment 9016092 [details] [diff] [review] patch Review of attachment 9016092 [details] [diff] [review]: ----------------------------------------------------------------- A few small nits. ::: devtools/server/actors/replay/debugger.js @@ +179,5 @@ > return this._scripts[data.id]; > }, > > + _convertScriptQuery(query) { > + const rv = query; I'd usually favor const rv = Object.assign({}, query); for this type of thing to avoid mutating the input options. @@ +184,5 @@ > + for (const name in query) { > + if (name == "global" || name == "source") { > + rv[name] = query[name]._data.id; > + } else { > + rv[name] = query[name]; Looks like this is a no-op since the prop is already there in `rv`. @@ +654,5 @@ > } > }, > > + _convertPropertyDescriptor(desc) { > + let rv = {}; I realize this, and the original code, is missing the other fields that exist on descriptors. Seems like this should copy all the props from `desc` and then reassign the individual ones we override? ::: devtools/server/actors/replay/replay.js @@ +530,5 @@ > return RecordReplayControl.repaint(); > }, > > + findScripts({ query }) { > + if ("global" in query) { Same here for mutating the input query object. It'd be safer to do `query = Object.assign({}, query);` to shallow-copy first.
Assignee | ||
Comment 5•2 years ago
|
||
(In reply to Logan Smyth [:loganfsmyth] from comment #4) > @@ +654,5 @@ > > } > > }, > > > > + _convertPropertyDescriptor(desc) { > > + let rv = {}; > > I realize this, and the original code, is missing the other fields that > exist on descriptors. Seems like this should copy all the props from `desc` > and then reassign the individual ones we override? Good catch, this is actually just a problem in the new code, as in the old code we are mutating the descriptor in place.
Assignee | ||
Comment 6•2 years ago
|
||
Updated patch, thanks for the comments!
Attachment #9016092 -
Attachment is obsolete: true
Attachment #9016092 -
Flags: review?(lsmyth)
Attachment #9017013 -
Flags: review?(lsmyth)
Updated•2 years ago
|
Attachment #9017013 -
Flags: review?(lsmyth) → review+
Assignee | ||
Comment 7•2 years ago
|
||
This addendum is needed to fix a problem that showed up in tests. If we do a findScripts() and specify a source that doesn't exist yet (in which case the replaying processes will specify null for the 'source' property of the query), the C++ Debugger throws an exception. There isn't a similar issue for 'global' because the ReplayDebugger can only specify JS objects that exist at the point we are paused at.
Attachment #9018225 -
Flags: review?(lsmyth)
Assignee | ||
Comment 8•2 years ago
|
||
If we throw an exception while handling a debugger request, we'll try to create a time warp target for that exception and crash. Returning zero here avoids the crash and will be interpreted by callers as an empty/invalid warp target.
Attachment #9018227 -
Flags: review?(continuation)
Updated•2 years ago
|
Attachment #9018225 -
Flags: review?(lsmyth) → review+
Updated•2 years ago
|
Attachment #9018227 -
Flags: review?(continuation) → review+
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac95e858506 Part 1 - Fix ReplayDebugger problems when searching for scripts and enumerating object properties, r=lsmyth. https://hg.mozilla.org/integration/mozilla-inbound/rev/40a4e0406d3d Part 2 - Tolerate time warp targets being created when handling debugger requests, r=mccr8.
Comment 10•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8ac95e858506 https://hg.mozilla.org/mozilla-central/rev/40a4e0406d3d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•1 year ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•