Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle potential missing point stack execution points #9885

Merged
merged 2 commits into from Nov 9, 2023

Conversation

markerikson
Copy link
Collaborator

This PR:

  • bumps the protocol package to 0.63 to pick up the types changes for getPointStack
  • Tweaks the logic in some caches to handle pointStackFrame.point becoming optional

I think, based on https://github.com/replayio/backend/pull/8924 and https://github.com/replayio/backend/pull/8925 , that this will only ever occur if the frame just has an EnterFrame point and no meaningful frame steps. However, I don't know often that will occur in practice.

The code compiles, and I think the logic tweaks make sense reading it.

Copy link

vercel bot commented Nov 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
devtools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 9, 2023 8:08pm

Copy link

replay-io bot commented Nov 9, 2023

E2E Tests

StatusIn Progress ↗︎
Commitca88571
Results
⚠️ 21 Flaky
68 Passed

Snapshot Tests

StatusIn Progress ↗︎
Commitca88571
Results
100 Passed

Copy link

replay-delta bot commented Nov 9, 2023

const frame = pointStack[frameIndex];
if (frame.point) {
currentPoint = frame.point.point;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return; if frame.point is not set - otherwise the debugger will step in the top frame rather than the selected frame.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yeah, that's what I was wondering.

Copy link

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@replayio/protocol 0.60.0...0.63.0, 0.62.0...0.63.0 None +0/-0 558 kB dmiller_replay

@markerikson
Copy link
Collaborator Author

Talked with Josh about this.

One example where this could happen is class B extends class A, where A has a constructor but B does not. (This occurs in our doc_rr_error.html E2E example.) In that case, there's technically a constructor for B on the stack but it has no code associated with it, so you can't pause there.

In practice, any "top" or "bottom" stack frame (no matter which way you visualize the stack in your head) should have an execution point because there should be actual code involved.

For the Redux and React usage cases, we should never run into that scenario, because we're looking at user code frames that should have actual code.

@markerikson
Copy link
Collaborator Author

markerikson commented Nov 9, 2023

Added further comments clarifying the situation here.

@markerikson markerikson merged commit 14a3f1e into main Nov 9, 2023
23 of 25 checks passed
@bvaughn
Copy link
Collaborator

bvaughn commented Nov 9, 2023

One example where this could happen is class B extends class A, where A has a constructor but B does not. (This occurs in our doc_rr_error.html E2E example.) In that case, there's technically a constructor for B on the stack but it has no code associated with it, so you can't pause there.

This is the kind of thing that's great to capture in unit tests somewhere. I don't have a ton of context (just kind of drive by skimming this thing) but I wonder if there's an opportunity for us to add a test or two here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants