1508314 - Stepping should work "normally"

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Jason Laster [:jlast]]

We expect that the stepping buttons (resume, step in, step out, step forward) should work the same while replaying as they would work without web-replay. 

We also expect that the inverse operations (step back, rewind) should be mirror operations of their counter-parts.

Comment 1

[Jason Laster [:jlast]]

Step out does not pause at parent frame:

STR:

1. go to https://firefox-dev.tools/debugger-examples/examples/todomvc/
2. add a breakpoint at todo-view#33
3. step forward
4. rewind

ER: expect to pause at 33
AR: keep going



STR: 

1. go to https://firefox-dev.tools/debugger-examples/examples/todomvc/
2. add a breakpoint at backbone.js#99
3. step in twice
4. step out from pick


ER: be back at 999
AR: at 1003 where View is finished...

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Watch for hits at temporary checkpoint locations when searching backwards.

The first case is a problem in the navigation code.  When searching backwards in the execution space, breakpoint hits are ignored at execution points where temporary checkpoints were previously created.  This still needs test coverage, which I'm hoping to add soon.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Use parent frame's location when finding step offsets.

The second case is a problem in the code for stepping out when replaying which was added recently.  We look for step offsets in the parent frame but do so using the source location of the callee frame.  This ends up producing offsets that will not be hit after stepping out.  (This also needs test coverage.)

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 9026245 [details] [diff] [review]
Part 1 - Watch for hits at temporary checkpoint locations when searching backwards.

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

::: toolkit/recordreplay/ipc/ChildNavigation.cpp
@@ +290,5 @@
>    void OnRegionEnd();
>  
>  public:
>    // Note: this always rewinds.
> +  void Enter(const CheckpointId& aStart, const Maybe<ExecutionPoint>& aEnd, bool aIncludeEnd);

Maybe it makes sense to have a default value of false for aIncludeEnd?

Comment 5

[Jason Laster [:jlast]]

Comment on attachment 9026246 [details] [diff] [review]
Part 2 - Use parent frame's location when finding step offsets.

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

Looks good to me

Comment 6

[Jason Laster [:jlast]]

I see a couple of things when testing in app-view.js#addOne


I'm stepping into `app.TodoView` stepping out, and then adding breakpoints on different lines in addOne. 

1. sometimes I miss a breakpoint 
2. sometimes I resume and hit a previous step out location (did we somehow not remove an onPop handler)?

Lastly, maybe un-related but when i click the console jump buttons I go to the right spot but the red line is at the top, which is a bit strange because the progress locations should be good because we're jumping to the right spot.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Comment on attachment 9026245 [details] [diff] [review]
> Part 1 - Watch for hits at temporary checkpoint locations when searching
> backwards.
> 
> Review of attachment 9026245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/recordreplay/ipc/ChildNavigation.cpp
> @@ +290,5 @@
> >    void OnRegionEnd();
> >  
> >  public:
> >    // Note: this always rewinds.
> > +  void Enter(const CheckpointId& aStart, const Maybe<ExecutionPoint>& aEnd, bool aIncludeEnd);
> 
> Maybe it makes sense to have a default value of false for aIncludeEnd?

Hmm, I think it would be better to avoid the default value --- I don't think either value for aIncludeEnd has an intrinsic reason to be the common case.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Clear onStep and onPop hooks for all frames.

This patch makes sure that onStep and onPop hooks are cleared for all frames when a thread pauses or resumes without stepping.

onStep and onPop hooks have different semantics from the normal Debugger.  When setting one of these hooks in a Debugger, they are cleared when the frame is popped, but in the ReplayDebugger case they remain in case the user rewinds to a time where the frame exists.  (They are subtly different in other ways related to our inability to uniquely identify frames, in that they will be called for any frame with the same script and depth; these are potentially fixable, but the above issue kind of goes hand in hand with the ability to rewind.)

To make these hooks easier to manage for the thread actor, this patch removes the ability to directly set onStep and onPop hooks to undefined for ReplayDebugger frames, and instead provides a replayClearSteppingHooks method on the debugger itself that clears out these hooks for all frames, including ones not currently on the stack.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/436fe5156b0a
Part 1 - Watch for hits at temporary checkpoint locations when searching backwards, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4a0822756a
Part 2 - Use parent frame's location when finding step offsets, r=jlast.

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/436fe5156b0a
https://hg.mozilla.org/mozilla-central/rev/1d4a0822756a

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f668df79d93b
Part 3 - Clear out stepping hooks via ReplayDebugger, r=lsmyth.

Comment 12

[Daniel Varga [:dvarga]]

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