1465289 - Web Replay: Record/replay debugger

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

This bug includes patches for adding the replay debugger and associated logic.  Debuggers created in middleman processes by devtools server code have a replay debugger which they use for getting information about a recording/replaying child process and for controlling its execution.  Likewise, the child process is able to respond to queries from the replay debugger in the middleman, and is able to play forward or backward to the last point where a breakpoint was hit.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Debugger changes for interfacing with replay debugger.

Most changes to the debugger itself are just forwarding property accesses / calls to methods in the replay debugger.  Other changes include refactoring handling of script related methods so they don't need an actual JSScript*, and adding a few new methods on the debugger which devtools server code will need when dealing with a replay debugger.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Add replay debugger.

The replay debugger fetches information from a recording/replaying process by sending JSON requests and getting JSON responses.  Within the recording/replaying process additional logic is needed so that the process can keep track of what it is doing as it scans forward or backward for breakpoint hits (which requires rewinding, often multiple times, and needs state that persists across these rewinds).

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Keep track of JS execution progress made when there is a replay debugger.

The replay debugger identifies points of execution within a recording as a pair of a progress count and a script location or other place where a breakpoint can be set.  The idea is that the debugger has a progress counter that advances at predictable points, mainly when entering a script or hitting a LOOPENTRY opcode, and needs to have the same value regardless of what JIT compilations have been performed.

Comment 4

[Jan de Mooij [:jandem]]

What are the printfs for? If it's necessary it would be nice if we could abstract that somewhere with a comment explaining what's going on.

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> What are the printfs for? If it's necessary it would be nice if we could
> abstract that somewhere with a comment explaining what's going on.

I used the printfs for debugging count variances between different compiler options, but it's probably better to remove them.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8981713 [details] [diff] [review]
Part 3 - Keep track of JS execution progress made when there is a replay debugger.

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

Sorry for the delay. The idea makes sense to me. You need something similar for Wasm baseline/Ion right?

::: js/src/jit/BaselineBailouts.cpp
@@ +1032,5 @@
>          }
>          op = JSOp(*pc);
> +        if (skippedLoopEntry && ReplayDebugger::trackProgress(script)) {
> +            if (const char* str = ReplayDebugger::progressString("SkippedLoopEntry", script, skippedLoopEntry))
> +                fprintf(stderr, "%s", str);

Please remove the printfs here and elsewhere.

@@ +1234,5 @@
>                  opReturnAddr = baselineScript->prologueEntryAddr();
>                  JitSpew(JitSpew_BaselineBailouts, "      Resuming into prologue.");
>  
> +                // The comment above is wrong and there are times when we end
> +                // up here even though the script has started executing. Undo

We should just fix the comment or the code.

::: js/src/jit/IonBuilder.cpp
@@ +1675,5 @@
>          if (!succ->addPredecessor(alloc(), current))
>              return abort(AbortReason::Alloc);
>      }
>  
> +    if (ins->brokenLoopEntry()) {

This needs a comment.

@@ +2503,5 @@
>      if (!graph().removeSuccessorBlocks(header))
>          return abort(AbortReason::Alloc);
>      graph().removeBlockFromList(header);
>  
> +    // Find the LOOPENTRY pc for this loop.

Hm I remember running into this JSOP_LOOPENTRY issue with the WIP patch for Ion optimization levels. I'll try to split that off and post a patch for that, so you don't need to do this here.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +128,5 @@
>  
> +    if (ReplayDebugger::trackProgress(gen->info().script())) {
> +        if (const char* str = ReplayDebugger::progressString("Prologue", gen->info().script()))
> +            masm.printf(str);
> +        masm.inc64(AbsoluteAddress(&ReplayDebugger::gProgressCounter));

We don't support workers (yet)? Wondering about races.

::: js/src/vm/Stack.cpp
@@ +223,5 @@
>              lexicalEnv = &cx->global()->lexicalEnvironment();
>              varObjRoot = cx->global();
>          }
> +        if (!CheckGlobalDeclarationConflicts(cx, script, lexicalEnv, varObjRoot)) {
> +            // Treat this as a script entry, for consistency with Ion.

After this we call probes::EnterScript. What about moving the EnterScript call before the CheckGlobalDeclarationConflicts call, would that fix this?

Comment 7

[Brian Hackett [Laid off!]]

Thanks for the feedback, I'll update the patch based on the one in bug 1467496.

(In reply to Jan de Mooij [:jandem] from comment #6)
> Comment on attachment 8981713 [details] [diff] [review]
> Part 3 - Keep track of JS execution progress made when there is a replay
> debugger.
> 
> Review of attachment 8981713 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. The idea makes sense to me. You need something similar
> for Wasm baseline/Ion right?

Eventually, yeah, but I don't think we support wasm yet (the signal handler it uses is disabled, though I don't know what repercussions that has in terms of what works or not).

> ::: js/src/jit/shared/CodeGenerator-shared.cpp
> @@ +128,5 @@
> >  
> > +    if (ReplayDebugger::trackProgress(gen->info().script())) {
> > +        if (const char* str = ReplayDebugger::progressString("Prologue", gen->info().script()))
> > +            masm.printf(str);
> > +        masm.inc64(AbsoluteAddress(&ReplayDebugger::gProgressCounter));
> 
> We don't support workers (yet)? Wondering about races.

We can record/replay tabs that use workers, but they can't be debugged yet.  ReplayDebugger::trackProcess returns true only for scripts associated with the main thread's runtime.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Keep track of JS execution progress made when there is a replay debugger.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 8981711 [details] [diff] [review]
Part 1 - Debugger changes for interfacing with replay debugger.

So, after working on web console integration (bug 1448607 part 40) I have some new thoughts about this.  I think that modifying the Debugger C++ object to add functionality for replaying is the wrong approach.  Instead, I'd like to make the replay debugger self hosted, to as great a degree as possible.

The middleman process can create a JS object which mimics the interface of the C++ Debugger, and performs queries by sending JSON requests to the recording/replaying process and receiving JSON responses.  In the recording/replaying process these JSON requests are handled by JS code which manages a Debugger to inspect the state of things.  The only changes to the Debugger should be additions to support this inspection.

Note that this strategy is basically the same as what we do now, except most things are happening in JS rather than C++.  This should make everything a lot tidier and the replay-specific changes separated from existing logic, and will be better for future work as we expand the interface between the middleman and recording/replaying processes (e.g. for web console integration).

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8981712 [details] [diff] [review]
Part 2 - Add replay debugger.

Many parts of this patch need rewriting per the new approach outlined above.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8985707 [details] [diff] [review]
Part 3 - Keep track of JS execution progress made when there is a replay debugger.

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

Sorry for the delay. r=me with comment below addressed.

::: js/src/jit/MIR.h
@@ +8374,5 @@
>  // Check whether we need to fire the interrupt handler.
>  class MInterruptCheck : public MNullaryInstruction
>  {
> +    CompilerScript script_;
> +    jsbytecode* pc_;

At least the pc_ field isn't used anymore AFAICT. I think instead of script/pc all we need now is a |bool recordReplayTrackProgress_ = false;| or something with a method to set it to |true|.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 8981711 [details] [diff] [review]
Part 1 - Debugger changes for interfacing with replay debugger.

Bug 1470795 has the new version of the replay debugger.

Comment 13

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #9)
> Comment on attachment 8981711 [details] [diff] [review]
> Part 1 - Debugger changes for interfacing with replay debugger.
> 
> So, after working on web console integration (bug 1448607 part 40) I have
> some new thoughts about this.  I think that modifying the Debugger C++
> object to add functionality for replaying is the wrong approach.  Instead,
> I'd like to make the replay debugger self hosted, to as great a degree as
> possible.
> 
> The middleman process can create a JS object which mimics the interface of
> the C++ Debugger, and performs queries by sending JSON requests to the
> recording/replaying process and receiving JSON responses.  In the
> recording/replaying process these JSON requests are handled by JS code which
> manages a Debugger to inspect the state of things.  The only changes to the
> Debugger should be additions to support this inspection.
> 
> Note that this strategy is basically the same as what we do now, except most
> things are happening in JS rather than C++.  This should make everything a
> lot tidier and the replay-specific changes separated from existing logic,
> and will be better for future work as we expand the interface between the
> middleman and recording/replaying processes (e.g. for web console
> integration).

Yeah, I like the sound of this a lot.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f53213595e
Part 3 - Keep track of JS execution progress made when there is a replay debugger, r=jandem.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/a3f53213595e