Skip to content

[RUN-3009] Keep track of ReplayScriptAlive and error-out executeCommand calls if its not#1068

Merged
Domiii merged 16 commits intomasterfrom
dominik/run-3009-error-no-context-group-available-for-isolate
Jan 11, 2024
Merged

[RUN-3009] Keep track of ReplayScriptAlive and error-out executeCommand calls if its not#1068
Domiii merged 16 commits intomasterfrom
dominik/run-3009-error-no-context-group-available-for-isolate

Conversation

@Domiii
Copy link
Copy Markdown

@Domiii Domiii commented Jan 6, 2024

…n-3009-error-no-context-group-available-for-isolate
@Domiii Domiii changed the title [RUN-3009] Keep track of gAlive and error-out executeCommand calls if its not [RUN-3009] Keep track of ReplayScriptAlive and error-out executeCommand calls if its not Jan 8, 2024
@Domiii
Copy link
Copy Markdown
Author

Domiii commented Jan 8, 2024

TODO: Fix cross-domain navigation.

InitializeRecordReplay(GetIsolate(), GetFrame(), context);
}

if (doInit || GetFrame() == gRecordReplayFrame) {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note: This did not handle potential cross-domain (and maybe other types of?) navigation where the frame does not get re-used.

Copy link
Copy Markdown

@toshok toshok left a comment

Choose a reason for hiding this comment

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

feels like this PR is circling similar issues to what I'm running into with #1042

Comment on lines 239 to 241
// Some of these are duplicated in gSourceMapScript, so watch out when making
// modifications to update both versions...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

these comments (and including utils.js above) are making less sense now. should we move other things up above (like assert, isFunction, isObject, typeofMaybeNull)? or remove the utils.js comment?

// about to reset, and its ExecutionContext and our V8 debugger session with it.
// From this point forward, command handling is not possible anymore
// until a new page is spawned.
// Note: JS can still execute, even after this happened.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ugh (re: the Note)


// NOTE: The root `LocalFrame` will actually not change over time.
gLocalRootFrame = localFrame;
// NOTE: The root `LocalFrame` will not necessarily change over time.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this isn't a strong enough assertion. how about:

Suggested change
// NOTE: The root `LocalFrame` will not necessarily change over time.
// NOTE: The root `LocalFrame` can change over time.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Its also less wordy!

…n-3009-error-no-context-group-available-for-isolate
@Domiii
Copy link
Copy Markdown
Author

Domiii commented Jan 10, 2024

@toshok:

feels like this PR is circling similar issues to what I'm running into with #1042

Its rather different:

  • Your patch is trying to make certain aspects of shutdown more deterministic.
  • What we are doing here is track liveness of our root frame, and with it, our gReplayScript and debugger sessions. This is not a concern of determinism and more about knowing when which commands are allowed to execute.

…n-3009-error-no-context-group-available-for-isolate
@Domiii Domiii merged commit 659ac46 into master Jan 11, 2024
@Domiii Domiii deleted the dominik/run-3009-error-no-context-group-available-for-isolate branch January 11, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants