Closed Bug 1498012 Opened Last year Closed 11 months ago

Fix ReplayDebugger problems when searching for scripts and enumerating object properties

Categories

(Core :: Web Replay, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch patch (obsolete) — 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 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?
(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.
Attached patch patch (obsolete) — Splinter Review
Updated patch per comments.
Attachment #9016059 - Attachment is obsolete: true
Attachment #9016059 - Flags: review?(lsmyth)
Attachment #9016092 - Flags: review?(lsmyth)
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.
(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.
Attached patch patchSplinter Review
Updated patch, thanks for the comments!
Attachment #9016092 - Attachment is obsolete: true
Attachment #9016092 - Flags: review?(lsmyth)
Attachment #9017013 - Flags: review?(lsmyth)
Attachment #9017013 - Flags: review?(lsmyth) → review+
Attached patch patchSplinter Review
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)
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)
Attachment #9018225 - Flags: review?(lsmyth) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/8ac95e858506
https://hg.mozilla.org/mozilla-central/rev/40a4e0406d3d
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.