1465488 - Web Replay: Server/client changes

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[Brian Hackett [Laid off!]]

This bug has changes to devtools code for Web Replay, other than those already reviewed in bug 1207696 and those which have already landed to the debugger.html github repository.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Web Replay server side devtools changes.

Debugger server changes other than those already reviewed in bug 1207696.  The most substantive changes are that when using a replaying debugger, the server code needs to choose offsets at which the debugger should stop when setting an onStep handler.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Web Replay client side devtools changes.

Specify whether the thread should rewind in resume/stepping thread client methods, and add new methods for reverse stepping and rewinding.

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8981912 [details] [diff] [review]
Part 2 - Web Replay client side devtools changes.

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

Forwarding to Jason who should be a better reviewer here.

Comment 4

[Brian Hackett [Laid off!]]

Hi, do you know when you'll be able to get around to these reviews?

Comment 5

[Jason Laster [:jlast]]

I'd be happy to look tomorrow.

Comment 6

[Jason Laster [:jlast]]

Comment on attachment 8981912 [details] [diff] [review]
Part 2 - Web Replay client side devtools changes.

This looks good. Feel free to land it with a green try run

Comment 7

[Jason Laster [:jlast]]

The changes in Part 1 - all look pretty sensible. Jim what do you think?

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8981909 [details] [diff] [review]
Part 1 - Web Replay server side devtools changes.

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

I have a small change requested and few questions, but overall this looks good for landing preffed off. Thanks!

::: devtools/server/actors/frame.js
@@ +54,5 @@
> +      if (!this.frame.environment) {
> +        return {};
> +      }
> +    } catch (e) {
> +      // |this.frame| might not be live.

I think this might be a pre-existing problem in the server. We should never be seeing a dead frame at this point. If you have steps to reproduce this, let's get a separate bug filed for that, but let's not just add try/catch clauses that cover up problems.

::: devtools/server/actors/tab.js
@@ +22,5 @@
>  var DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  var { assert } = DevToolsUtils;
>  var { TabSources } = require("./utils/TabSources");
>  var makeDebugger = require("./utils/make-debugger");
> +const Debugger = require("Debugger");

What does this refer to? I can't find a Debugger.js or .jsm in the current ash source tree.

::: devtools/server/actors/thread.js
@@ +560,5 @@
>    },
>  
>    /**
> +   * Find the offsets in a frame for a replaying process where the onStep hook
> +   * should fire at.

I think this comment would be better phrased as:

Given that we are stepping forward (rewinding == false) or backwards (rewinding == true), return an array of all the step targets that could be reached next from startLocation. This is where the onStep hook should fire next.

Also, you might consider using Set for the return type. It'd make the users simpler too, I think.

@@ +563,5 @@
> +   * Find the offsets in a frame for a replaying process where the onStep hook
> +   * should fire at.
> +   */
> +  _findReplayingStepOffsets: function(startLocation, frame, rewinding) {
> +    let worklist = [frame.offset], seen = [], result = [];

I think these could be Sets instead of arrays, and then your membership tests could be simpler.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Jim Blandy :jimb from comment #8)
> Comment on attachment 8981909 [details] [diff] [review]
> Part 1 - Web Replay server side devtools changes.
> 
> Review of attachment 8981909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a small change requested and few questions, but overall this looks
> good for landing preffed off. Thanks!
> 
> ::: devtools/server/actors/frame.js
> @@ +54,5 @@
> > +      if (!this.frame.environment) {
> > +        return {};
> > +      }
> > +    } catch (e) {
> > +      // |this.frame| might not be live.
> 
> I think this might be a pre-existing problem in the server. We should never
> be seeing a dead frame at this point. If you have steps to reproduce this,
> let's get a separate bug filed for that, but let's not just add try/catch
> clauses that cover up problems.

OK, I'll file a bug for this.  Is it OK to leave the try/catch here and refer to the bug number in a comment?

> ::: devtools/server/actors/tab.js
> @@ +22,5 @@
> >  var DevToolsUtils = require("devtools/shared/DevToolsUtils");
> >  var { assert } = DevToolsUtils;
> >  var { TabSources } = require("./utils/TabSources");
> >  var makeDebugger = require("./utils/make-debugger");
> > +const Debugger = require("Debugger");
> 
> What does this refer to? I can't find a Debugger.js or .jsm in the current
> ash source tree.

This getter is already defined (i.e. not as part of Web Replay) by devtools/shared/builtin-modules.js.  Debugger here is the C++ js/src/vm/Debugger.cpp object.

Comment 10

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #9)
> OK, I'll file a bug for this.  Is it OK to leave the try/catch here and
> refer to the bug number in a comment?

Sure, thanks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19725e5ab62c
Part 1 - Web Replay server side devtools changes, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/af2decbe03d6
Part 2 - Web Replay client side devtools changes, r=jlast.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/19725e5ab62c
https://hg.mozilla.org/mozilla-central/rev/af2decbe03d6