1470795 - Web Replay: Record/replay debugger v2

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

This bug includes the new version of the replay debugger and its supporting code, after being rewritten per bug 1465289 comment 9.  The general goals here are to do as much as possible in JS, and to avoid changes to the existing debugger code as much as possible.  The new structure for the replay debugger is as follows:

- When devtools server code wants to create a Debugger, it first checks if it is running in a middleman process.  If so, it instead creates a ReplayDebugger object (devtools/server/actors/replay/debugger.js), which is implemented in JS and implements nearly the same interface as Debugger itself.

- The ReplayDebugger code responds to calls and property accesses from the server code by sending JSON requests and other commands to the recording/replaying child process, receiving responses synchronously.  These commands are performed through the RecordReplayControl global property.

- RecordReplayControl is defined in toolkit/recordreplay/ipc/JSControl.cpp and forwards the commands it receives through the record/replay IPC machinery to the child process, where they are received by the child navigation code.

- The navigation code is defined in toolkit/recordreplay/ipc/ChildNavigation.cpp, and manages the process position as it executes to find the next or previous breakpoint hit.  It handles some of the commands by calling into a singleton JS sandbox, in which record/replay JS code runs.

- The record/replay JS sandbox code (devtools/server/actors/replay/replay.js) creates a Debugger it can use to inspect the JS running in its process, set breakpoints and so forth, and respond to the commands originally sent from the ReplayDebugger in the middleman process.

How this works is basically the same as how the previous implementation worked.  The main reasons for this change are to avoid the massive changes to Debugger.cpp and related code which the previous implementation required, to remove a couple thousand lines of new C++ code, and to make it easier to expand the interface with the recording/replaying process in the future (e.g. for handling console messages).  The total size of the Web Replay diff has been reduced by nearly 20%, from 1.69MB to 1.44MB.

I've attached a rolled up patch compared to the previous stuff (including the rollback of most of the previous implementation), and will attach patches for individual portions shortly.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Debugger changes for recording/replaying.

Add a Debugger method to get the current process kind (which the server code can use to figure out whether to create Debuggers or ReplayDebuggers), and some Debugger.Script methods for queries needed when recording/replaying.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Record/replay API changes for replay debugger.

Changes to public and internal record/replay APIs for the new replay debugger.  Since all the C++ code lives in toolkit/recordreplay now, rather than being split with js/src/vm, the public API can be slimmed down a lot.  A few new methods are needed in the public API for use from JS, the API for script file contents (bug 1465292) has moved here, and some new internal APIs are needed for functionality that was previously in the now-defunct js/public/ReplayHooks.h.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Trivial renaming due to API changes.

Renaming and other simple internal fixes needed as a result of the API reworking.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Move progress tracking logic to JSScript.

Move the logic for whether a script's progress should be tracked to script->trackRecordReplayProgress(), and make some simple renaming changes due to the API reworking.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Update content parse users for API movement.

The content parsing API from bug 1465292 is now part of the record/replay public API, rather than the JS API.  This fixes the users of this API.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Part 6 - Add navigation logic for managing breakpoints in child process.

This patch has the navigation code used to manage the position of the recording/replaying process as it responds to messages from the middleman.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Part 7 - Add RecordReplayControl JS interface.

Add code for defining the RecordReplayControl object, with methods for interacting with the navigation code and other parts of the record/replay infrastructure.  This also has definitions for the JS sandbox which runs in recording/replaying processes.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Part 8 - Define RecordReplayControl object via IJSDebugger.

Define RecordReplayControl in the same globals where we define Debugger.  The two are related and it doesn't seem necessary to duplicate all the glue code needed to make these separate interfaces.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: Part 9 - Add ReplayDebugger and record/replay sandbox JS files.

Add JS code for the ReplayDebugger used in devtools server code, and its counterpart that runs in the sandbox created in recording/replaying processes.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: Part 10 - Include progress counter in record/replay assertion streams.

Include checks in the record/replay assertion stream that the main thread's JS progress counter has consistent values between recording and replaying.  If the progress counter is not consistent (due either to JS compilation mode affecting the progress counter, or different JS code running) then things will break.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: Part 11 - Fix bug in record/replay channel initialization.

This isn't strictly related to the other patches here, but this fixes a bug with channel initialization that was causing tests to fail.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: Part 12 - Devtools server changes for new replay debugger.

Devtools server side changes to use ReplayDebugger.  This applies on top of the patches in bug 1465488.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8987412 [details] [diff] [review]
Part 5 - Update content parse users for API movement.

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

::: js/src/vm/JSScript.cpp
@@ +2225,5 @@
>              ScriptSource::PinnedChars chars(xdr->cx(), this, holder, 0, length());
>              if (!chars.get())
>                  return xdr->fail(JS::TranscodeResult_Throw);
> +            mozilla::recordreplay::NoteContentParse(this, filename(), "application/javascript",
> +                                                    chars.get(), length());

It's not super-clear to me what the surrounding context here is, but other calls to this function in this patch are guarded by `if (recordreplay::IsRecordingOrReplaying())`, as well as calls to the component *ContentParse* functions; should this function have the same guard as well?

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8987410 [details] [diff] [review]
Part 3 - Trivial renaming due to API changes.

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

r/rs=me

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8987409 [details] [diff] [review]
Part 2 - Record/replay API changes for replay debugger.

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

::: mfbt/RecordReplay.h
@@ +349,5 @@
> +// function is entered or loop entry point is reached, so that no script
> +// location may be hit twice while the progress counter is the same. See
> +// JSControl.h for more.
> +typedef uint64_t ProgressCounter;
> +MFBT_API ProgressCounter* ExecutionProgressCounter();

WDYT about making this provide a read-only view of the ProgressCounter?  AdvanceExecutionProgressCounter would need to be changed, probably with an intermediate ::detail API that allows read-write access.  Unless there are legitimate reasons for the users of the API to modify the progress counter directly?

::: toolkit/recordreplay/ProcessRecordReplay.h
@@ +260,5 @@
> +// checkoints, malloc and other standard library functions suffice to allocate
> +// memory in the record/replay system. The routines below are used for handling
> +// redirections for the raw system calls underlying the standard libraries, and
> +// for cases where allocated memory should be untracked: the contents are
> +// ignored when saving/restoring checkpoints.contents 

Nit: trailing space.

@@ +263,5 @@
> +// for cases where allocated memory should be untracked: the contents are
> +// ignored when saving/restoring checkpoints.contents 
> +
> +// Different kinds of memory used in the system.
> +enum class MemoryKind {

Thank you!

@@ +310,3 @@
>  
> +  template <typename T>
> +  void free_(T* aPtr, size_t aSize) {

Is this code reflective of the understanding you and Waldo came to?  I confess to not watching the #jsapi chatter closely.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8987411 [details] [diff] [review]
Part 4 - Move progress tracking logic to JSScript.

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

::: js/src/vm/JSScript-inl.h
@@ +211,5 @@
> +JSScript::trackRecordReplayProgress() const
> +{
> +    // Progress is only tracked when recording or replaying, and only for
> +    // scripts associated with the main thread's runtime. Whether self hosted
> +    // scripts execute may depend on performed Ion optimizations, so they are

Hm which optimizations? Just curious, I'm fine with ignoring self-hosted scripts (they're just like C++ natives), but it's probably good to be more specific in this comment.

@@ +214,5 @@
> +    // scripts associated with the main thread's runtime. Whether self hosted
> +    // scripts execute may depend on performed Ion optimizations, so they are
> +    // ignored. Some scripts are internal to record/replay and run
> +    // non-deterministically, so are also ignored.
> +    return mozilla::recordreplay::IsRecordingOrReplaying()

Maybe MOZ_UNLIKELY(mozilla::recordreplay::IsRecordingOrReplaying())

@@ +215,5 @@
> +    // scripts execute may depend on performed Ion optimizations, so they are
> +    // ignored. Some scripts are internal to record/replay and run
> +    // non-deterministically, so are also ignored.
> +    return mozilla::recordreplay::IsRecordingOrReplaying()
> +        && !runtimeFromAnyThread()->parentRuntime

Maybe at some point JSRuntime could have a flag for this? It could replace both the IsRecordingOrReplaying and parentRuntime checks and it would be very explicit.

Comment 17

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> Comment on attachment 8987412 [details] [diff] [review]
> Part 5 - Update content parse users for API movement.
> 
> Review of attachment 8987412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/JSScript.cpp
> @@ +2225,5 @@
> >              ScriptSource::PinnedChars chars(xdr->cx(), this, holder, 0, length());
> >              if (!chars.get())
> >                  return xdr->fail(JS::TranscodeResult_Throw);
> > +            mozilla::recordreplay::NoteContentParse(this, filename(), "application/javascript",
> > +                                                    chars.get(), length());
> 
> It's not super-clear to me what the surrounding context here is, but other
> calls to this function in this patch are guarded by `if
> (recordreplay::IsRecordingOrReplaying())`, as well as calls to the component
> *ContentParse* functions; should this function have the same guard as well?

Yeah, the diff could show more of the surrounding code, but we do test above this that we are recording or replaying, per the patch in bug 1465292 part 2.

Comment 18

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> ::: mfbt/RecordReplay.h
> @@ +349,5 @@
> > +// function is entered or loop entry point is reached, so that no script
> > +// location may be hit twice while the progress counter is the same. See
> > +// JSControl.h for more.
> > +typedef uint64_t ProgressCounter;
> > +MFBT_API ProgressCounter* ExecutionProgressCounter();
> 
> WDYT about making this provide a read-only view of the ProgressCounter? 
> AdvanceExecutionProgressCounter would need to be changed, probably with an
> intermediate ::detail API that allows read-write access.  Unless there are
> legitimate reasons for the users of the API to modify the progress counter
> directly?

The JITs need to know the actual address of the progress counter so that the code they emit can update it.

> @@ +310,3 @@
> >  
> > +  template <typename T>
> > +  void free_(T* aPtr, size_t aSize) {
> 
> Is this code reflective of the understanding you and Waldo came to?  I
> confess to not watching the #jsapi chatter closely.

This reflects the current patch in bug 1309552, but I'll update it to reflect any additional changes required to the AllocPolicy API when that bug gets reviewed.

Comment 19

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #16)
> Comment on attachment 8987411 [details] [diff] [review]
> Part 4 - Move progress tracking logic to JSScript.
> 
> Review of attachment 8987411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/JSScript-inl.h
> @@ +211,5 @@
> > +JSScript::trackRecordReplayProgress() const
> > +{
> > +    // Progress is only tracked when recording or replaying, and only for
> > +    // scripts associated with the main thread's runtime. Whether self hosted
> > +    // scripts execute may depend on performed Ion optimizations, so they are
> 
> Hm which optimizations? Just curious, I'm fine with ignoring self-hosted
> scripts (they're just like C++ natives), but it's probably good to be more
> specific in this comment.

Many of the TypedObject Ion optimizations are short circuiting self hosted code, though I think there are other cases as well.  I'll update the comment.

> @@ +215,5 @@
> > +    // scripts execute may depend on performed Ion optimizations, so they are
> > +    // ignored. Some scripts are internal to record/replay and run
> > +    // non-deterministically, so are also ignored.
> > +    return mozilla::recordreplay::IsRecordingOrReplaying()
> > +        && !runtimeFromAnyThread()->parentRuntime
> 
> Maybe at some point JSRuntime could have a flag for this? It could replace
> both the IsRecordingOrReplaying and parentRuntime checks and it would be
> very explicit.

Could JSRuntime have a forProcessMainThread() method that checks its parentRuntime field?  I could also add an isWorkerRuntime() with the opposite semantics, but want to be careful since historically we've avoided this nomenclature in JS.

Comment 20

[Andrew McCreight [:mccr8]]

Could you explain what NewCheckpoint is doing when it returns false? You are telling it to save the current execution position, so I think I understand the case when it returns true, but why would it restore it when you tell it to save it? Does that mean you've already saved it so you aren't going to save it again? And then somehow the caller has to deal with that differently?

Comment 21

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #20)
> Could you explain what NewCheckpoint is doing when it returns false? You are
> telling it to save the current execution position, so I think I understand
> the case when it returns true, but why would it restore it when you tell it
> to save it? Does that mean you've already saved it so you aren't going to
> save it again? And then somehow the caller has to deal with that differently?

When NewCheckpoint() returns false, it indicates that the process has just rewound to this point of execution:

1. At some point in the past, at this point in the main thread's control flow it called NewCheckpoint(), which returned true.
2. More code was executed, and then the main thread called RestoreCheckpointAndResume() on the checkpoint id associated with the earlier NewCheckpoint() call.
3. The RestoreCheckpointAndResume() call updates the contents of all thread stacks and registers (excepting some internal record/replay threads which were spawned via Thread::SpawnNonRecordedThread), and all heap memory (excepting that which was allocated using an untracked alloc policy, such as NavigationState and all its contents) to their values when the NewCheckpoint() call occurred.
4. The main thread's control flow returns (again) from the NewCheckpoint() call, except that now it returns false to indicate that the restore/rewind occurred.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8987413 [details] [diff] [review]
Part 6 - Add navigation logic for managing breakpoints in child process.

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

It took me a bit to get my head around this, but I think I at least have the general idea of it now, if not all of the specifics.

::: toolkit/recordreplay/ipc/ChildInternal.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_toolkit_recordreplay_ipc_ChildInternal_h
> +#define mozilla_toolkit_recordreplay_ipc_ChildInternal_h

nit: this should be based on the namespace, not the directory. So, mozilla_recordreplay_ChildInternal_h

@@ +31,5 @@
> +
> +// In a replaying process, set the recording endpoint. |index| is used to
> +// differentiate different endpoints that have been sequentially written to
> +// the recording file as it has been flushed.
> +void SetRecordingEndpoint(size_t index, const js::ExecutionPoint& endpoint);

nit: aIndex and aEndpoint.

@@ +48,5 @@
> +// can't rewind, or already diverged here and then had an unhandled divergence.
> +bool MaybeDivergeFromRecording();
> +
> +// Notify navigation that an execution position was hit.
> +void PositionHit(const js::ExecutionPosition& position);

nit: aPosition.

::: toolkit/recordreplay/ipc/ChildNavigation.cpp
@@ +58,5 @@
> +
> +typedef AllocPolicy<MemoryKind::Navigation> UntrackedAllocPolicy;
> +
> +static CheckpointId
> +NextTemporaryCheckpoint(const CheckpointId& aCheckpoint)

This feels like it should be a method on CheckpointId.

@@ +64,5 @@
> +  return CheckpointId(aCheckpoint.mNormal, aCheckpoint.mTemporary + 1);
> +}
> +
> +static CheckpointId
> +NextNormalCheckpoint(const CheckpointId& aCheckpoint)

Likewise.

@@ +78,5 @@
> +{
> +  // All virtual members should only be accessed through NavigationState.
> +  friend class NavigationState;
> +
> +  MOZ_NORETURN void Unsupported(const char* aOperation) {

Should this method be protected?

@@ +114,5 @@
> +    Unsupported("HandleDebuggerRequest");
> +  }
> +
> +  // Called when a debugger request wants to try an operation that may
> +  // trigger an unhandled divergence from the recording.

Out of curiosity, what sort of things is this used for? Seems like an odd thing to need.

@@ +131,5 @@
> +  }
> +};
> +
> +// Information about a debugger request sent by the middleman.
> +struct RequestInfo {

nit: { on new line.

@@ +153,5 @@
> +
> +typedef InfallibleVector<uint32_t> BreakpointVector;
> +
> +// Phase when the replaying process is paused at a breakpoint.
> +class BreakpointPausedPhase : public NavigationPhase

You should mark all of these classes phase final.

@@ +250,5 @@
> +  bool mSavedTemporaryCheckpoint;
> +
> +  // The time at which we started running forward from the initial
> +  // checkpoint, in microseconds.
> +  double mStartTime;

mozilla::TimeStamp is the standard Gecko way to represent time, though it looks like the code that uses this is very simple and should be okay with time moving backwards.

@@ +301,5 @@
> +    {}
> +  };
> +  InfallibleVector<TrackedPosition, 4, UntrackedAllocPolicy> mTrackedPositions;
> +
> +  TrackedPosition FindTrackedPosition(const ExecutionPosition& aPos);

Should this return a reference instead of a copy? I don't have a sense of how big the tracked positions actually are.

@@ +522,5 @@
> +
> +  gNavigation->SetPhase(this);
> +
> +  if (ThisProcessCanRewind()) {
> +    // Immediately save a temporary checkpoint and upate the point to be

nit: "update"

@@ +860,5 @@
> +}
> +
> +// The number of milliseconds to elapse during a ReachBreakpoint search before
> +// we will save a temporary checkpoint.
> +static const double TemporaryCheckpointThresholdMs = 10;

nit: this constant should start with k (eg kTemporaryCheckpointThresholdMs).

@@ +969,5 @@
> +  for (TrackedPosition& tracked : mTrackedPositions) {
> +    if (tracked.mPosition.Subsumes(aPoint.mPosition)) {
> +      tracked.mLastHit = aPoint;
> +      tracked.mLastHitCount = mCounter;
> +      break;

Is there any danger of having multiple tracked positions being subsumed by a single point? Maybe it doesn't matter if the later one never has anything set on it?

@@ +1027,5 @@
> +                                                /* aAtRecordingEndpoint = */ false);
> +      Unreachable();
> +    }
> +  }
> +

Am I right in thinking that this replays through the entire region, decides whichever point that matches the criteria in the region occurred last, then does another replay to get to that point again? I guess that's faster than saving every time we find a later breakpoint in the initial runthrough?

Comment 23

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #22)
> @@ +114,5 @@
> > +    Unsupported("HandleDebuggerRequest");
> > +  }
> > +
> > +  // Called when a debugger request wants to try an operation that may
> > +  // trigger an unhandled divergence from the recording.
> 
> Out of curiosity, what sort of things is this used for? Seems like an odd
> thing to need.

Some operations the debugger can do will try to interact with the system in a way we can't handle while replaying.  For example, say a user pauses in the replay and tries to evaluate 'alert("foo")'.  In these cases we want to just return a failure message to the debugger (e.g. this operation could not be completed), but at the time we detect the problem we're deep inside some code which is calling into the system, and need the unhandled divergence stuff to restore a checkpoint taken when we first reached the breakpoint and then redoing the old debugger requests to restore all local state we expect to have (e.g. mappings between IDs and particular JSObjects) at this point.

> @@ +969,5 @@
> > +  for (TrackedPosition& tracked : mTrackedPositions) {
> > +    if (tracked.mPosition.Subsumes(aPoint.mPosition)) {
> > +      tracked.mLastHit = aPoint;
> > +      tracked.mLastHitCount = mCounter;
> > +      break;
> 
> Is there any danger of having multiple tracked positions being subsumed by a
> single point? Maybe it doesn't matter if the later one never has anything
> set on it?

It's OK if there are multiple identical tracked positions, as we'll use the mLastHit/mLastHitCount fields to determine the last execution point where *any* breakpoint was hit, and then when reaching that point in the ReachBreakpoint phase we will find all breakpoints hit at that point and notify the middleman about all of them.  Keeping separate mLastHit/mLastHitCount fields for each tracked position is done so that we can separately determine if there is a point where the frame containing the last breakpoint was entered, so that we can place a temporary checkpoint there for better stepping performance.

> @@ +1027,5 @@
> > +                                                /* aAtRecordingEndpoint = */ false);
> > +      Unreachable();
> > +    }
> > +  }
> > +
> 
> Am I right in thinking that this replays through the entire region, decides
> whichever point that matches the criteria in the region occurred last, then
> does another replay to get to that point again? I guess that's faster than
> saving every time we find a later breakpoint in the initial runthrough?

Yeah, this is correct.  It's not necessarily faster than saving checkpoints whenever we hit breakpoints, but it is more predictable.  If there is a long running loop there could be thousands or millions of breakpoint hits we find when running backwards, and we would need a fallback strategy in that case.  The temporary checkpoints we save at frame entry help mitigate this, and if there are no breakpoint hits at all when running backwards (the usual case) then we only need to execute the code once.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 8987418 [details] [diff] [review]
Part 11 - Fix bug in record/replay channel initialization.

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

So, this patch changes things so that we explicitly pass in whether we're recording or not, rather than relying on a special property of the channel id. I guess the first subprocess wasn't always the recording process? Was there some race condition at startup or something causing this?

::: toolkit/recordreplay/ipc/ChildIPC.cpp
@@ +203,5 @@
>    Maybe<AutoPassThroughThreadEvents> pt;
>    pt.emplace();
>  
>    gMonitor = new Monitor();
> +  gChannel = new Channel(channelID.ref(), false, ChannelMessageHandler);

Please add a /* aMiddlemanRecording = */ comment to false.

Comment 25

[Andrew McCreight [:mccr8]]

Sorry for the delay here. I have a few questions that will help me better review the remaining patch.

Could you give a description of what the different ExecutionPosition cases are? Break I guess is a breakpoint, and NewScript I assume is when we start executing some JS script, but what are OnStep, OnPop, and EnterFrame? I can't really review ExecutionPosition::Subsumes without that.

Could you also please give me more of a description of what ExecutionPosition and ExecutionPoint are? The names are practically synonyms, and so I'm having trouble working out exactly why they are different. I guess ExecutionPosition is like a possible breakpoint position?

Comment 26

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> So, this patch changes things so that we explicitly pass in whether we're
> recording or not, rather than relying on a special property of the channel
> id. I guess the first subprocess wasn't always the recording process? Was
> there some race condition at startup or something causing this?

The problem was actually pretty simple: when we are replaying a saved recording, there is no recording subprocess.  Sorry for the lame patch description.

Comment 27

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #25)
> Sorry for the delay here. I have a few questions that will help me better
> review the remaining patch.
> 
> Could you give a description of what the different ExecutionPosition cases
> are? Break I guess is a breakpoint, and NewScript I assume is when we start
> executing some JS script, but what are OnStep, OnPop, and EnterFrame? I
> can't really review ExecutionPosition::Subsumes without that.

Sure, here are the descriptions (I wrote these when doing the console integration stuff, and will update this patch to include these as comments).

Break: Break at a script offset. Requires script/offset.

OnStep: Break for an on-step handler within a frame. Requires script/offset/frameIndex.

OnPop: Break either when any frame is popped, or when a specific frame is popped. Requires script/frameIndex in the latter case.

EnterFrame: Break when entering any frame.

NewScript: Break when a new top-level script is created.

> Could you also please give me more of a description of what
> ExecutionPosition and ExecutionPoint are? The names are practically
> synonyms, and so I'm having trouble working out exactly why they are
> different. I guess ExecutionPosition is like a possible breakpoint position?

ExecutionPosition is a place where a breakpoint can be set or where the process can pause and have its state inspected.  An ExecutionPosition may be hit any number of times during the process' execution.

ExecutionPoint is a particular point in the execution of the process: an ExecutionPosition, plus information (a progress count and checkpoint) to identify where in the execution that position was hit.

Would renaming ExecutionPosition to BreakpointPosition make things easier to follow?

Comment 28

[Andrew McCreight [:mccr8]]

Thanks for the description of the Kinds. That really helps.

(In reply to Brian Hackett (:bhackett) from comment #27)
> Would renaming ExecutionPosition to BreakpointPosition make things easier to follow?

Yes, that would be better, thanks.

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 8987414 [details] [diff] [review]
Part 7 - Add RecordReplayControl JS interface.

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

::: toolkit/recordreplay/ipc/JSControl.cpp
@@ +41,5 @@
> +// ExecutionPosition Conversion
> +///////////////////////////////////////////////////////////////////////////////
> +
> +static JSObject*
> +EncodeExecutionPosition(JSContext* aCx, const ExecutionPosition& aPosition)

This feels like it could be a method on ExecutionPosition.

@@ +46,5 @@
> +{
> +  RootedString kindString(aCx, JS_NewStringCopyZ(aCx, aPosition.KindString()));
> +  RootedObject obj(aCx, JS_NewObject(aCx, nullptr));
> +  if (!kindString || !obj ||
> +      !JS_DefineProperty(aCx, obj, "kind", kindString, JSPROP_ENUMERATE) ||

Now that this code isn't in js/, it would be better if it was using WebIDL instead of raw JSAPI, for improved maintainability by people who aren't SpiderMonkey hackers. That can be left as potential followup work, though that will require resolving whatever quirks of how this code works as compared to WebIDL. (eg bz said that this code requires things are numbers, rather than coercing things to numbers, vs the string case which does coerce.)

@@ +48,5 @@
> +  RootedObject obj(aCx, JS_NewObject(aCx, nullptr));
> +  if (!kindString || !obj ||
> +      !JS_DefineProperty(aCx, obj, "kind", kindString, JSPROP_ENUMERATE) ||
> +      (aPosition.mScript != ExecutionPosition::EMPTY_SCRIPT &&
> +       !JS_DefineProperty(aCx, obj, "script",

I think you should use constants for these four field names, as they each appear twice.

@@ +49,5 @@
> +  if (!kindString || !obj ||
> +      !JS_DefineProperty(aCx, obj, "kind", kindString, JSPROP_ENUMERATE) ||
> +      (aPosition.mScript != ExecutionPosition::EMPTY_SCRIPT &&
> +       !JS_DefineProperty(aCx, obj, "script",
> +                          (uint32_t) aPosition.mScript, JSPROP_ENUMERATE)) ||

You are writing these out as uint32_t, but they have type size_t. Can the C++ fields just be uint32_t?  Or maybe this operation should fail if we overflow rather than silently return bad values?

@@ +70,5 @@
> +  if (!JS_GetProperty(aCx, aObject, aProperty, &v)) {
> +    return false;
> +  }
> +  if (v.isNumber()) {
> +    *aResult = (size_t) v.toNumber();

What happens here if v is something like NaN or infinity?

@@ +97,5 @@
> +      break;
> +    }
> +  }
> +  if (aResult->mKind == ExecutionPosition::Invalid) {
> +    JS_ReportErrorASCII(aCx, "Could not decode execution position kind");

Be sure to change this error if you rename ExecutionPosition.

@@ +117,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +
> +// Keep track of all replay debuggers in existence, so that they can all be
> +// invalidated when the process is unpaused.
> +static StaticInfallibleVector<PersistentRootedObject*> gReplayDebuggers;

I guess this just leaks replay debuggers? Or should they get unregistered somewhere?

@@ +464,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +
> +struct ContentInfo
> +{
> +  const void* mToken;

I was worried about pointer replay for mToken (no pun intended) but I see that you clear mToken in the End method, and while the ScriptLoader or HTML parser that is the source of the token is alive, so there's no danger of that. I think that at least the declaration of the API methods (BeginContentParse etc.) should have a comment that is more explicit about the lifetime of the token. EG you aren't allowed to reuse a token until after you call the End method.

@@ +474,5 @@
> +    : mToken(aToken),
> +      mURL(strdup(aURL)),
> +      mContentType(strdup(aContentType))
> +  {}
> +};

This should free mURL and mContentType in the dtor. (Even though they aren't being freed right now.)

@@ +477,5 @@
> +  {}
> +};
> +
> +// All content that has been parsed so far. Protected by child::gMonitor.
> +static StaticInfallibleVector<ContentInfo> gContent;

These are never freed. I guess there's no way to know when the client of the API is done with a particular token?

@@ +491,5 @@
> +
> +  RecordReplayAssert("BeginContentParse %s", aURL);
> +
> +  MonitorAutoLock lock(*child::gMonitor);
> +  for (ContentInfo& info : gContent) {

I guess all of these linear searches of gContent aren't a problem?

@@ +675,5 @@
> +
> +extern "C" {
> +
> +MOZ_EXPORT bool
> +RecordReplayInterface_DefineRecordReplayControlObject(JSContext* aCx, JSObject* aObjectArg)

Is this the right name? I noticed in part 8 you are calling mozilla::recordreplay::DefineRecordReplayControlObject(), which seems like it is the same thing.

::: toolkit/recordreplay/ipc/JSControl.h
@@ +29,5 @@
> +//   interface which allows the JS to query the current process.
> +
> +// Identification for an execution position --- anyplace a breakpoint can be
> +// created --- during JS execution in a child process.
> +struct ExecutionPosition

As discussed, please consider renaming this class.

@@ +31,5 @@
> +// Identification for an execution position --- anyplace a breakpoint can be
> +// created --- during JS execution in a child process.
> +struct ExecutionPosition
> +{
> +  enum Kind {

As discussed, please include the descriptions you posted as a comment in the bug. They were very helpful.

@@ +43,5 @@
> +  size_t mScript;
> +  size_t mOffset;
> +  size_t mFrameIndex;
> +
> +  static const size_t KIND_LIMIT = NewScript + 1;

This should be defined as part of the enum, so that if somebody adds a new one it'll automatically keep working.

@@ +61,5 @@
> +  {}
> +
> +  bool IsValid() const { return mKind != Invalid; }
> +
> +  inline bool operator ==(const ExecutionPosition& o) const {

nit: no space between "operator" and "==".

@@ +68,5 @@
> +        && mOffset == o.mOffset
> +        && mFrameIndex == o.mFrameIndex;
> +  }
> +
> +  inline bool operator !=(const ExecutionPosition& o) const { return !(*this == o); }

likewise

@@ +74,5 @@
> +  // Return whether an execution point matching |o| also matches this.
> +  inline bool Subsumes(const ExecutionPosition& o) const {
> +    return (*this == o)
> +        || (mKind == OnPop && o.mKind == OnPop && mScript == EMPTY_SCRIPT)
> +        || (mKind == Break && o.mKind == OnStep && mScript == o.mScript && mOffset == o.mOffset);

Why does Break subsume OnStop (and not also the other way around)?

@@ +129,5 @@
> +    // ExecutionPoint positions must be as precise as possible, and cannot
> +    // subsume other positions.
> +    MOZ_RELEASE_ASSERT(aPosition.mKind != ExecutionPosition::OnPop ||
> +                       aPosition.mScript != ExecutionPosition::EMPTY_SCRIPT);
> +    MOZ_RELEASE_ASSERT(aPosition.mKind != ExecutionPosition::Break);

Is it okay for aPosition to be invalid with this ctor?

Comment 30

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #29)
> @@ +117,5 @@
> > +///////////////////////////////////////////////////////////////////////////////
> > +
> > +// Keep track of all replay debuggers in existence, so that they can all be
> > +// invalidated when the process is unpaused.
> > +static StaticInfallibleVector<PersistentRootedObject*> gReplayDebuggers;
> 
> I guess this just leaks replay debuggers? Or should they get unregistered
> somewhere?

Yeah, these are leaked.  We hold onto memory indefinitely in a fair number of places in the system, and fixing these doesn't seem like a big priority for now since recording/replaying tabs should be relatively short lived.  This one here would only be a problem if someone opens and closes the devtools over and over, since each occurrence will create some new replay debuggers.

> @@ +477,5 @@
> > +  {}
> > +};
> > +
> > +// All content that has been parsed so far. Protected by child::gMonitor.
> > +static StaticInfallibleVector<ContentInfo> gContent;
> 
> These are never freed. I guess there's no way to know when the client of the
> API is done with a particular token?

Yeah, the devtools could ask for any piece of content that has ever been seen in the past.

> @@ +675,5 @@
> > +
> > +extern "C" {
> > +
> > +MOZ_EXPORT bool
> > +RecordReplayInterface_DefineRecordReplayControlObject(JSContext* aCx, JSObject* aObjectArg)
> 
> Is this the right name? I noticed in part 8 you are calling
> mozilla::recordreplay::DefineRecordReplayControlObject(), which seems like
> it is the same thing.

Yeah, these extern "C" methods are used for calling into the record/replay system through the interface in mfbt/RecordReplay.h, which has to lookup these symbols before calling them to circumvent linking issues.

> @@ +74,5 @@
> > +  // Return whether an execution point matching |o| also matches this.
> > +  inline bool Subsumes(const ExecutionPosition& o) const {
> > +    return (*this == o)
> > +        || (mKind == OnPop && o.mKind == OnPop && mScript == EMPTY_SCRIPT)
> > +        || (mKind == Break && o.mKind == OnStep && mScript == o.mScript && mOffset == o.mOffset);
> 
> Why does Break subsume OnStop (and not also the other way around)?

OnStep positions are only hit when a script offset is hit with a particular stack depth, rather than all stack depths.  The comment here isn't very good, it should read 'all' rather than 'an'.

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 8987407 [details] [diff] [review]
Part 1 - Debugger changes for recording/replaying.

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

Clearing r?. Looks good, but new Debugger features need docs in js/src/doc/Debugger/Debugger.Script.md and tests in jit-test/tests/debug.

How hard would it be to get record and replay working in the shell? Seems like it would be good value for money...

Anyway, even if not, test that
- each property/method actually works for the kind of thing you want to do with it (like, maybe you want to use .mainOffset to set breakpoints from onNewScript...?)
- s.getSuccessorOffsets(x).includes(y) <==> s.getPredecessorOffsets(y).includes(x)
- as we step through a function, as long as frame.implementation == "interpreter" anyway, each offset where we stop is a successor of the previous offset, across a few different control flow structures
- whatever's tricky about this stuff, if anything (seems super straightforward)

Comment 32

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Debugger changes for recording/replaying.

Comment 33

[Brian Hackett [Laid off!]]

(In reply to Jason Orendorff [:jorendorff] from comment #31)
> How hard would it be to get record and replay working in the shell? Seems
> like it would be good value for money...

Basic recording and replaying could be done without a huge amount of trouble, but since we don't attempt to precisely replay any of the non-deterministic features of the JS engine --- the GC, JITs, anything that happens on helper threads --- there wouldn't be much value in recording/replaying the shell I think.

Comment 34

[Jason Orendorff [:jorendorff]]

Comment on attachment 8992117 [details] [diff] [review]
Part 1 - Debugger changes for recording/replaying.

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

::: js/src/jit-test/tests/debug/Script-getSuccessorOrPredecessorOffsets-01.js
@@ +15,5 @@
> +for (var n in scripts) {
> +  g.eval("function f" + n + "() { " + scripts[n] + " }");
> +}
> +
> +function contains(arr, n) {

This is `arr.includes(n)`.

Comment 35

[Jim Blandy :jimb]

Comment on attachment 8987416 [details] [diff] [review]
Part 9 - Add ReplayDebugger and record/replay sandbox JS files.

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

Just curious: this patch doesn't reflect what's currently on the tip in the ash repository, and my pull has no changeset 91090f54ea00. Should this patch be updated?

(I'd just like to be able to review the change in the context in which it will actually land, so I was trying to read the code in the 'ash' repo directly.)

Comment 36

[Jim Blandy :jimb]

Comment on attachment 8987416 [details] [diff] [review]
Part 9 - Add ReplayDebugger and record/replay sandbox JS files.

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

Comments on debugger.js only; still looking at replay.js.

::: devtools/server/actors/replay/debugger.js
@@ +25,5 @@
> +
> +function ReplayDebugger() {
> +  RecordReplayControl.registerReplayDebugger(this);
> +
> +  this._breakpoints = [];

This array holds an interesting assortment of data. It would help to have a comment explaining that it also holds other sorts of handler functions, and that per-frame handlers live here indexed by the stable frame index, and so on. It would save people the trouble of having to infer it from the code.

@@ +176,5 @@
> +  /////////////////////////////////////////////////////////
> +  // Frame methods
> +  /////////////////////////////////////////////////////////
> +
> +  _newestFrameIndex: -1,

It would be a little clearer if this were just a const at the top-level. When it's a property, I wonder whether it's going to be updated at some point, but it's just a named constant.

@@ +178,5 @@
> +  /////////////////////////////////////////////////////////
> +
> +  _newestFrameIndex: -1,
> +
> +  _getFrame(index) {

A comment at some point that frames are numbered from zero on up, with zero referring to the oldest frame, would be helpful.

@@ +195,5 @@
> +
> +    if (index == this._newestFrameIndex) {
> +      if ("index" in data) {
> +        index = data.index;
> +      }

It feels like this should just be an assertion. If data.index isn't set, then we end up populating this._frames[-1], which I think is not intended.

@@ +379,5 @@
> +
> +  _positionMatches(position, kind) {
> +    return position.kind == kind
> +        && position.script == this._data.script
> +        && position.frameIndex == this._data.index;

If the frame index matches, doesn't that mean the script would also match anyway?

Comment 37

[Jim Blandy :jimb]

Comment on attachment 8987416 [details] [diff] [review]
Part 9 - Add ReplayDebugger and record/replay sandbox JS files.

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

More partial review comments. I still need to look at replay.js some more tomorrow.

::: devtools/server/actors/replay/replay.js
@@ +47,5 @@
> +  }
> +}
> +
> +// Bidirectional map between objects and IDs.
> +function IdMap() {

If I understand correctly, this uses a Map whose keys are Debugger.Object, Debugger.Environment, Debugger.Script, and Debugger.Source objects. Wouldn't it work just as well to simply store the ID directly on the Debugger.Foo object, perhaps under a symbol? That's how the Debugger.Foo objects were originally meant to be used: as a place to store metadata about things in the debuggee.

We'd still need the id->Debugger.Foo array, of course.

@@ +124,5 @@
> +function addScriptSource(source) {
> +  gScriptSources.add(source);
> +}
> +
> +function considerScript(script) {

If replay added only content globals as debuggees, then Debugger would do this filtering automatically. Or at least, this is the kind of the "debuggee / non-debuggee" distinction was supposed to help with.

Comment 38

[Brian Hackett [Laid off!]]

Attachment: Part 9 - Add ReplayDebugger and record/replay sandbox JS files.

Here is an updated patch, reflecting what I'm planning of landing as of the last rebase I did (about two weeks ago).  The main difference between this and the previous patch are formatting and other simple fixes to satisfy ESLint.  I did make a couple other (slightly) more substantial fixes, which I'll attach an interdiff for in a moment.

The stuff on ash pretty mostly reflects what is going to land, but there has been some divergence for work that I've done since posting the original patches. These changes won't be included in the Web Replay landing (since they haven't been reviewed) but I'll start posting patches for them sometime afterwards. The main differences:

- Web console integration has been added.  This includes some changes to the files added by this patch.

- Various fixes have gone in to support recording/replaying mochitests.  These mainly touch C++ files and don't affect this patch.

Comment 39

[Brian Hackett [Laid off!]]

Attachment: ReplayDebugger interdiff

Interdiff between the two versions of this patch, besides linting fixes.  These fixes address problems that showed up after a rebase.

Comment 40

[Brian Hackett [Laid off!]]

(In reply to Jim Blandy :jimb from comment #36)
> @@ +379,5 @@
> > +
> > +  _positionMatches(position, kind) {
> > +    return position.kind == kind
> > +        && position.script == this._data.script
> > +        && position.frameIndex == this._data.index;
> 
> If the frame index matches, doesn't that mean the script would also match
> anyway?

Yes, it should, but it is possible for the debugger to have old breakpoints left over from frames it paused at in the past, in which case the scripts could differ.  (Though it might be good to improve checks in the debugger to make sure such cases don't happen.)

Comment 41

[Brian Hackett [Laid off!]]

(In reply to Jim Blandy :jimb from comment #37)
> Comment on attachment 8987416 [details] [diff] [review]
> Part 9 - Add ReplayDebugger and record/replay sandbox JS files.
> 
> Review of attachment 8987416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> More partial review comments. I still need to look at replay.js some more
> tomorrow.
> 
> ::: devtools/server/actors/replay/replay.js
> @@ +47,5 @@
> > +  }
> > +}
> > +
> > +// Bidirectional map between objects and IDs.
> > +function IdMap() {
> 
> If I understand correctly, this uses a Map whose keys are Debugger.Object,
> Debugger.Environment, Debugger.Script, and Debugger.Source objects. Wouldn't
> it work just as well to simply store the ID directly on the Debugger.Foo
> object, perhaps under a symbol? That's how the Debugger.Foo objects were
> originally meant to be used: as a place to store metadata about things in
> the debuggee.
> 
> We'd still need the id->Debugger.Foo array, of course.

Storing the IDs on the Debugger.Foo objects would work, but it seems more complicated to me, since this would spread the data out more and make it less clear that the bidirectional map is internally consistent (i.e. x -> y iff y -> x).

> @@ +124,5 @@
> > +function addScriptSource(source) {
> > +  gScriptSources.add(source);
> > +}
> > +
> > +function considerScript(script) {
> 
> If replay added only content globals as debuggees, then Debugger would do
> this filtering automatically. Or at least, this is the kind of the "debuggee
> / non-debuggee" distinction was supposed to help with.

Sure, I can experiment with this, considerScript() is pretty ugly.  Looking at _shouldAddNewGlobalAsDebuggee in devtools/server/actors/tab.js, though, filtering for content globals looks non-trivial, so it is OK to file a bug and leave this for followup?

Comment 42

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #41)
> Sure, I can experiment with this, considerScript() is pretty ugly.  Looking
> at _shouldAddNewGlobalAsDebuggee in devtools/server/actors/tab.js, though,
> filtering for content globals looks non-trivial, so it is OK to file a bug
> and leave this for followup?

Absolutely. I just wanted to make sure you knew that there was An Official Way to do that.

Comment 43

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #41)
> Storing the IDs on the Debugger.Foo objects would work, but it seems more
> complicated to me, since this would spread the data out more and make it
> less clear that the bidirectional map is internally consistent (i.e. x -> y
> iff y -> x).

Well, that's why I suggested using a symbol. The point of symbols, it seemed to me, was that you can find all the uses of a symbol the same way you can find all the uses of a given object (like the table). If the only uses of the symbol are in the IdMap methods, then the consistency rules remain clear.

Whichever approach you prefer is fine. As before, I'm just pointing out that the Debugger API had An Official Way to do this.

Comment 44

[Jim Blandy :jimb]

Comment on attachment 8993422 [details] [diff] [review]
Part 9 - Add ReplayDebugger and record/replay sandbox JS files.

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

Okay, r=me. One bug, and the rest are all requests for comments. Thanks!!

::: devtools/server/actors/replay/replay.js
@@ +9,5 @@
> +// This code is loaded into all recording/replaying processes, and responds to
> +// requests and other instructions from the middleman via the exported symbols
> +// defined at the end of this file.
> +//
> +// Code in this file runs independently from the recording: middleman requests

The phrasing "runs independently" threw me off a bit: this code's state *is* included in snapshots and affected by snapshot restoration, right?

It might be clearer to say:

Like all other JavaScript in the recording/replaying process, this code's state is included in snapshots and reset by snapshot restoration. But in the process of handling the middleman's requests, its state must be allowed to vary between recording and replaying, and from one replay to another. As a result, we have to be very careful about performing operations that might interact with debuggee JavaScript, whose behavior must be deterministic. The RecordReplayControl.maybeDiverFromRecording function should be used at any point where such interactions might occur.

@@ +84,5 @@
> +    return this._idToObject.length - 1;
> +  },
> +};
> +
> +function countScriptFrames() {

Maybe this isn't worth addressing right now, but:

Walking the stack with Debugger isn't super-cheap. Debugger doesn't just build a linked list of frames; it actually uses ScriptFrameIterator to walk down the stack lazily to find debuggee parent frames. This can be expensive: Iterating over IonMonkey frames has to expand inline frames, etc. etc.

This module seems to iterate over the stack pretty freely, even in cases like this where all that's needed is a count. It might be good to keep an eye on this, and if the performance shows up, just walk the stack once and cache it until the next resume.

@@ +114,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +// Persistent State
> +///////////////////////////////////////////////////////////////////////////////
> +
> +const gScripts = new IdMap();

I would have found a comment like the following really helpful:

The indices that this table assigns to scripts are stable across the entire recording, even though this table (like all JS state) is included in snapshots, rolled back on rewinds, etc. In debuggee time, this table only grows (there is no method to remove entries); scripts created for debugger activity (evals, etc.) are ignored; and off-thread compilation is disabled, so this table acquires the same scripts in the same order as we roll back and forth in a recording.

@@ +170,5 @@
> +
> +// Whether there is a position handler for EnterFrame.
> +let gHasEnterFrameHandler = false;
> +
> +// Handlers we tried to install but couldn't due to a script not existing.

It seems obvious now, but it took me a while to understand why scripts don't exist. Could we add a comment like:

Breakpoints requested by the middleman, which RecordReplayControl preserves across snapshot restoration, identify target scripts by their stable id in gScripts. This array holds the breakpoints we can't actually insert yet because they refer to script ids we haven't seen yet.

@@ +246,5 @@
> +    case "OnStep":
> +      let debugScript;
> +      if (position.script) {
> +        debugScript = gScripts.getObject(position.script);
> +        if (!debugScript) {

Just to check my understanding:

This code's JavaScript state is included in snapshots, so rewinding past an onNewScript call causes its scripts' entries to disappear from gScripts. The set of active Web Replay breakpoints is managed by the RecordReplayControl object, and is, naturally, retained across snapshot restoration. This means that Web Replay breakpoint positions will in general include script indices that gScripts has no entry for (yet). When running forward, we retain such as-of-yet-unresolvable Web Replay breakpoint entries in gPendingHandlers; when a breakpoint's script does appear, we set the corresponding Debugger breakpoints.

If this is correct, could you put a comment to this effect on gPendingHandlers? (Feel free to use my text, if you think it's okay.)

@@ +254,5 @@
> +      }
> +
> +      gInstalledPcHandlers.forEach(({ script, offset }) => {
> +        if (script == position.script && offset == position.offset) {
> +          return;

This call to forEach has no effect. A return in an arrow function only returns from the arrow function, not the enclosing function. You could say:

if (gInstalledPcHandlers.some(predicate)) return;

It seems like Debugger's breakpoint API is making your life difficult here.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2500fc654564
Part 2 - Record/replay API changes for replay debugger, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/669eb891fa02
Part 3 - Trivial renaming due to API changes, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/301403057435
Part 6 - Add navigation logic for managing breakpoints in child process, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fede6c8a76a
Part 7 - Add RecordReplayControl JS interface, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd58259f140a
Part 9 - Add ReplayDebugger and record/replay sandbox JS files, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a06f53d689
Part 10 - Include progress counter in record/replay assertion streams, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/22d28e5778fd
Part 11 - Fix bug in record/replay channel initialization, r=mccr8.

Comment 46

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2500fc654564
https://hg.mozilla.org/mozilla-central/rev/669eb891fa02
https://hg.mozilla.org/mozilla-central/rev/301403057435
https://hg.mozilla.org/mozilla-central/rev/1fede6c8a76a
https://hg.mozilla.org/mozilla-central/rev/dd58259f140a
https://hg.mozilla.org/mozilla-central/rev/a4a06f53d689
https://hg.mozilla.org/mozilla-central/rev/22d28e5778fd

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb449f0184c
Part 1 - Debugger changes for recording/replaying, r=jorendorff.
https://hg.mozilla.org/integration/mozilla-inbound/rev/818c1a79b41d
Part 4 - Move progress tracking logic to JSScript, r=jandem.
https://hg.mozilla.org/integration/mozilla-inbound/rev/337cda619aee
Part 5 - Update content parse users for API movement, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0edf4a2020
Part 8 - Define RecordReplayControl object via IJSDebugger, r=jorendorff.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54722c7628d6
Part 12 - Devtools server changes for new replay debugger, r=jimb.

Comment 49

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

https://hg.mozilla.org/mozilla-central/rev/cfb449f0184c
https://hg.mozilla.org/mozilla-central/rev/818c1a79b41d
https://hg.mozilla.org/mozilla-central/rev/337cda619aee
https://hg.mozilla.org/mozilla-central/rev/6f0edf4a2020
https://hg.mozilla.org/mozilla-central/rev/54722c7628d6