Closed Bug 1465488 Opened 4 years ago Closed 4 years ago

Web Replay: Server/client changes

Categories

(DevTools :: Debugger, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Future
Tracking Status
firefox63 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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.
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.
Assignee: nobody → bhackett1024
Attachment #8981909 - Flags: review?(jimb)
Specify whether the thread should rewind in resume/stepping thread client methods, and add new methods for reverse stepping and rewinding.
Attachment #8981912 - Flags: review?(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.
Attachment #8981912 - Flags: review?(jdescottes) → review?(jlaster)
Priority: -- → P3
Target Milestone: --- → Future
Product: Firefox → DevTools
Hi, do you know when you'll be able to get around to these reviews?
Flags: needinfo?(jlaster)
Flags: needinfo?(jimb)
I'd be happy to look tomorrow.
Flags: needinfo?(jlaster)
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
Attachment #8981912 - Flags: review?(jlaster) → review+
The changes in Part 1 - all look pretty sensible. Jim what do you think?
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.
Attachment #8981909 - Flags: review?(jimb) → review+
(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.
(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.
Flags: needinfo?(jimb)
Depends on: 1477030
Pushed by bhackett@mozilla.com:
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.
You need to log in before you can comment on or make changes to this bug.