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

klochek · 2024-02-07T21:01:38Z

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.

Domiii · 2024-02-08T13:32:22Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

  window.__RECORD_REPLAY__ = window.top.__RECORD_REPLAY__;
  window.__RECORD_REPLAY_ARGUMENTS__ = window.top.__RECORD_REPLAY_ARGUMENTS__;
+
+  for (g of window.top.__RECORD_REPLAY_GLOBALS__) {


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 · 2024-02-08T13:43:24Z

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

Domiii · 2024-02-08T14:20:18Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

                 args);

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


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

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

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 · 2024-02-08T22:18:22Z

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

I can try to integrate with it.

cweld510

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?

cweld510 · 2024-02-09T13:52:02Z

replay-assets/replay_command_handlers.js

  }
 }

+function registerGlobal(name, val) {


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

Yup--the PR description shows a hypothetical usage.

cweld510 · 2024-02-09T14:00:08Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc


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


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

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.

cweld510 · 2024-02-09T14:24:34Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc


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


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

cweld510 · 2024-02-09T14:28:20Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

                 args);

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


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

klochek · 2024-02-09T15:52:04Z

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.

replay-io · 2024-02-09T16:06:07Z

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

Domiii · 2024-02-11T10:15:23Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

  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__) {


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.

Allow backend to send commands to execute preamble scripts.

7d41093

klochek requested review from Domiii and cweld510 February 7, 2024 21:01

Domiii reviewed Feb 8, 2024

View reviewed changes

Fixes.

8805488

cweld510 reviewed Feb 9, 2024

View reviewed changes

klochek force-pushed the cklochek/preamble branch from cf0ac5c to 8805488 Compare February 9, 2024 15:49

Domiii reviewed Feb 11, 2024

View reviewed changes

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

klochek commented Feb 7, 2024

Uh oh!

Domiii Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Domiii commented Feb 8, 2024

Uh oh!

Domiii Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cweld510 commented Feb 8, 2024

Uh oh!

cweld510 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klochek commented Feb 9, 2024

Uh oh!

replay-io bot commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Domiii Feb 8, 2024 •

edited

Loading

Domiii Feb 8, 2024 •

edited

Loading

replay-io bot commented Feb 9, 2024 •

edited

Loading