1498012 - Fix ReplayDebugger problems when searching for scripts and enumerating object properties

Status: RESOLVED FIXED

(Core :: Web Replay, enhancement)

Description

[Brian Hackett (:bhackett)]

Attachment: patch

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.

Comment 1

[Logan Smyth [:loganfsmyth]]

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?

Comment 2

[Brian Hackett (:bhackett)]

(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.

Comment 3

[Brian Hackett (:bhackett)]

Attachment: patch

Updated patch per comments.

Comment 4

[Logan Smyth [:loganfsmyth]]

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.

Comment 5

[Brian Hackett (:bhackett)]

(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.

Comment 6

[Brian Hackett (:bhackett)]

Attachment: patch

Updated patch, thanks for the comments!

Comment 7

[Brian Hackett (:bhackett)]

Attachment: patch

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.

Comment 8

[Brian Hackett (:bhackett)]

Attachment: Tolerate time warp targets being created when handling debugger requests.

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8ac95e858506
https://hg.mozilla.org/mozilla-central/rev/40a4e0406d3d