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

Allow backend to send commands to execute preamble scripts. #1122

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

klochek
Copy link

@klochek klochek commented Feb 7, 2024

Introduce a new function on __RECORD_REPLAY__ called registerGlobal, which receives one argument, the name of a global, that can be accessed by any descendant context (read: iframe, with appropriate sandboxing). registerGlobal stores the string in a new top-level global, __RECORD_REPLAY_GLOBALS__. Example usage:

  // ... js code here ...

  Target_evaluatePrivileged.chromiumRunPointEvaluationArg = ...;

  // Ensure Target_evaluatePrivileged is available across all descendant contexts.
  __RECORD_REPLAY_GLOBALS__.registerGlobal("Target_evaluatePrivileged");

Introduce a new command, Target.loadPreamble, which is given a string as an argument. The string is a script that will be immediately executed in the root (global) context by the browser. After execution, a second script is run which propagates all Replay globals declared in __RECORD_REPLAY_GLOBALS__ to all descendent child contexts, so that they can be accessed as if they were in the context's top-level scope.

Any time a new context is created, we re-execute the propagation code, which ensures the registered globals are available in that context.

@klochek klochek requested review from Domiii and cweld510 February 7, 2024 21:01
@@ -823,6 +824,10 @@ const char* gOnNewWindowScript = R""""(
// __RECORD_REPLAY__?
window.__RECORD_REPLAY__ = window.top.__RECORD_REPLAY__;
window.__RECORD_REPLAY_ARGUMENTS__ = window.top.__RECORD_REPLAY_ARGUMENTS__;

for (g of window.top.__RECORD_REPLAY_GLOBALS__) {
Copy link

@Domiii Domiii Feb 8, 2024

Choose a reason for hiding this comment

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

  • We also need to pass down __RECORD_REPLAY_GLOBALS__ itself.
    • In fact, I'd vote to only pass down __RECORD_REPLAY_GLOBALS__, and just force any user of this API to go through that, rather than adding random globals into a global context that is shared with arbitrary user JS.
  • linter failure.

@Domiii
Copy link

Domiii commented Feb 8, 2024

I want to see the API used and working before hitting Approve (if at all possible)

@@ -2502,6 +2547,10 @@ static void InitializeRecordReplayApiObjects(v8::Isolate* isolate, LocalFrame* l
DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_ARGUMENTS__",
args);

v8::Local<v8::Object> globals = v8::Object::New(isolate);
DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_GLOBALS__",
Copy link

@Domiii Domiii Feb 8, 2024

Choose a reason for hiding this comment

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

This will reset the globals on every root-level navigation. We probably want to keep them around or re-run the preamble.

Copy link

Choose a reason for hiding this comment

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

yeah, I was also confused about this. do we need to store the scripts somewhere so we can rerun them?

Copy link
Author

Choose a reason for hiding this comment

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

Domi and I have been chatting about our context handling, which is a tad naiive to say the least; we aren't going to worry about it in this PR, but rather address it wholly in another.

@cweld510
Copy link

cweld510 commented Feb 8, 2024

I want to see the API used and working before hitting Approve (if at all possible)

I can try to integrate with it.

Copy link

@cweld510 cweld510 left a comment

Choose a reason for hiding this comment

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

i'll attempt to try out these changes locally.

side question, not blocking: do we assume that all of this code is running single-threaded because it is only called into from v8, we only ever have one v8 instance, and the v8 instance is single-threaded?

@@ -3043,6 +3050,10 @@ function replayEval(fn) {
}
}

function registerGlobal(name, val) {
Copy link

Choose a reason for hiding this comment

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

This is intended to be called by the preamble script, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yup--the PR description shows a hypothetical usage.

@@ -2350,6 +2355,46 @@ static void fromJsEndReplayCode(
recordreplay::ExitReplayCode();
}

// Connects replay globals that must be accessible from all contexts; MUST be called
// with AutoMarkReplayCode.
static void ConnectPreambleGlobals(LocalFrame* frame, v8::Isolate *isolate) {
Copy link

Choose a reason for hiding this comment

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

can you pass in the AutoMarkReplayCode object here to ensure that it is held?

Copy link
Author

Choose a reason for hiding this comment

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

Ehh, I waffled on this, due to recursion (it just looks distasteful/re-entrant to me); once we fix our context handling, I think we'll end up with a set of Conexts we can just safely iterate over, rather than this traversal.

@@ -2350,6 +2355,46 @@ static void fromJsEndReplayCode(
recordreplay::ExitReplayCode();
}

// Connects replay globals that must be accessible from all contexts; MUST be called
// with AutoMarkReplayCode.
static void ConnectPreambleGlobals(LocalFrame* frame, v8::Isolate *isolate) {
Copy link

Choose a reason for hiding this comment

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

nit: maybe name should be ConnectPreambleGlobalsToCurrentContexts or something like that so it's clear what we are connecting the globals to?

@@ -2502,6 +2547,10 @@ static void InitializeRecordReplayApiObjects(v8::Isolate* isolate, LocalFrame* l
DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_ARGUMENTS__",
args);

v8::Local<v8::Object> globals = v8::Object::New(isolate);
DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_GLOBALS__",
Copy link

Choose a reason for hiding this comment

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

yeah, I was also confused about this. do we need to store the scripts somewhere so we can rerun them?

@klochek
Copy link
Author

klochek commented Feb 9, 2024

i'll attempt to try out these changes locally.

side question, not blocking: do we assume that all of this code is running single-threaded because it is only called into from v8, we only ever have one v8 instance, and the v8 instance is single-threaded?

Yes, this is by V8's design.

Copy link

replay-io bot commented Feb 9, 2024

Status In Progress ↗︎
Commit 8805488
Results
3 Failed
  • breakpoints-07: rewind and seek using command bar and console messages (Replay 1, Replay 2, Replay 3)
  • breakpoints-08: should be temporarily disabled (Replay 1, Replay 2, Replay 3)
  • playwright-01: Basic Test Suites panel functionality (Replay 1, Replay 2, Replay 3)
  • ⚠️ 2 Flaky
  • authenticated/passport-03: Swiss army knife (Replay 1, Replay 2)
  • breakpoints-05: Test interaction of breakpoints with debugger statements (Replay 1, Replay 2)
  • 55 Passed
  • authenticated/comments-01: Test add, edit, and delete comment functionality
  • authenticated/comments-02: Test shared comments and replies (Replay 1, Replay 2)
  • authenticated/logpoints-01: Shared logpoints functionality (Replay 1, Replay 2)
  • authenticated/passport-01: Time travel
  • authenticated/passport-02: Infrared inspection
  • authenticated/passport-04: Multiplayer
  • breakpoints-01: Test basic breakpoint functionality
  • breakpoints-02: Test unhandled divergence while evaluating at a breakpoint
  • breakpoints-03: Test stepping forward through breakpoints when rewound before the first one
  • breakpoints-04: catch, finally, generators, and async/await
  • console_async: support console evaluations in async frames
  • console_dock: Should show the correct docking behavior for recordings with video
  • console_eval: support console evaluations
  • console_warp-01: should support warping to console messages
  • console-expressions-01: should cache input eager eval and terminal expressions per instance
  • cypress-04: Test Step buttons and menu item
  • cypress-05: Test DOM node preview on user action step hover
  • deleted-recording: Show error message for deleted recording
  • elements-search: Element panel should support basic and advanced search modes
  • fe-1875 :: verify that steps go to the right point in time
  • file-search-01: should search files
  • focus_mode-01: should filter messages as regions based on the active focus mode
  • highlighter: element highlighter works everywhere
  • inspector-elements-01: Basic DOM tree node display
  • inspector-elements-02_node-picker: element picker and iframe behavior
  • inspector-elements-05_search: element picker and iframe behavior
  • logpoints-01: log-points appear in the correct order and allow time warping
  • logpoints-02: conditional log-points
  • logpoints-03: should display event properties in the console
  • logpoints-05: should auto-complete based on log point location
  • logpoints-06: should be temporarily disabled
  • logpoints-07: should use the correct scope in auto-complete
  • logpoints-08: should support jumping directly to a hit point via the capsule input
  • logpoints-09: should support pending edits
  • logpoints-10: too-many-points-to-find UX
  • logpoints-11: too-many-points-to-run-analysis UX
  • object_preview-02: should allow objects in scope to be inspected
  • object_preview-03: Test previews when switching between frames and stepping
  • object_preview-05: Should support logging objects as values
  • object_preview-06: HTML elements
  • playwright-02: Test Step timeline behavior
  • playwright-03: Test Step interactions
  • playwright-04: Test Step buttons and menu item
  • playwright-05: Test DOM node previews on user action step hover
  • react_devtools-02: RDT integrations (Chromium)
  • react_devtools-03: process and display multiple React versions in page
  • react_devtools-04: Component selection is maintained when seeking to a new point
  • redux_devtools: Test Redux DevTools.
  • resizable-panels-01: Left side Toolbar and Video should be collapsible
  • restart debugging session
  • scopes_rerender: Test that scopes are rerendered
  • source-line-highlights: Test source line highlighting
  • sourcemap_stacktrace: Test that stacktraces are sourcemapped
  • stepping-01: Test basic step-over/back functionality
  • stepping-06: Test stepping in async frames and async call stacks
  • @@ -825,7 +827,7 @@ const char* gOnNewWindowScript = R""""(
    window.__RECORD_REPLAY__ = window.top.__RECORD_REPLAY__;
    window.__RECORD_REPLAY_ARGUMENTS__ = window.top.__RECORD_REPLAY_ARGUMENTS__;

    for (g of window.top.__RECORD_REPLAY_GLOBALS__) {
    for (const g of window.top.__RECORD_REPLAY_GLOBALS__) {
    Copy link

    Choose a reason for hiding this comment

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

    Note: My previous concern was not addressed -

    • We also need to pass down __RECORD_REPLAY_GLOBALS__ itself.
    • In fact, I'd vote to only pass down __RECORD_REPLAY_GLOBALS__, and just force any user of this API to go through that, rather than adding random globals into a global context that is shared with arbitrary user JS.

    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.

    3 participants