Add agentInvoked and respondWith() to the `submit` event [3/N] [chromium/src : main]

0 views
Skip to first unread message

Mason Freed (Gerrit)

unread,
Jan 23, 2026, 4:49:39 PMJan 23
to Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
Attention needed from Anders Hartvoll Ruud and Dominic Farolino

Mason Freed voted and added 2 comments

Votes added by Mason Freed

Auto-Submit+1
Commit-Queue+1

2 comments

Patchset-level comments
File-level comment, Patchset 10:
Mason Freed . resolved

I'm putting both Anders and Dom on this, just to see who gets to it first. No need for two reviews.

File third_party/blink/renderer/core/html/forms/html_form_element.cc
Line 578, Patchset 3: promise.Unwrap().Then(script_state, resolved, rejected);
Mason Freed . resolved

Dom, I get this here:

```
[FATAL:third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h:94] Check failed: world_.Get() == &target_script_state->World() (0x12c00405b68 vs. 0x12c0040b8b0).
```

Mason Freed

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Anders Hartvoll Ruud
  • Dominic Farolino
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
Gerrit-Change-Number: 7467862
Gerrit-PatchSet: 10
Gerrit-Owner: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
Gerrit-Comment-Date: Fri, 23 Jan 2026 21:49:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Mason Freed <mas...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Anders Hartvoll Ruud (Gerrit)

unread,
Jan 24, 2026, 1:46:12 AMJan 24
to Mason Freed, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
Attention needed from Dominic Farolino and Mason Freed

Anders Hartvoll Ruud voted and added 4 comments

Votes added by Anders Hartvoll Ruud

Code-Review+1

4 comments

File chrome/browser/actor/tools/script_tool_browsertest.cc
Line 192, Patchset 13 (Latest): // EXPECT_EQ(action_results.at(0).result->script_tool_response->result,
// "val");
Anders Hartvoll Ruud . unresolved

nit: Is this useful? Can't we actually check the respondWidth result now?

File third_party/blink/renderer/core/html/forms/html_form_element.cc
Line 580, Patchset 13 (Latest): active_webmcp_tool_, /*resolved*/ true);
Anders Hartvoll Ruud . unresolved

nit: `/*resolved=*/` is better, since it's supported by clang-tidy.

https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html

Line 593, Patchset 13 (Latest): rejected = MakeGarbageCollected<RespondWithHandler>(
active_webmcp_tool_, /*resolved*/ false);
Anders Hartvoll Ruud . unresolved

Isn't this creating exactly the same handler already held by `rejected`? Did you mean:

```suggestion
resolved = rejected;
```

?

We should probably add a test for this branch.

File third_party/blink/renderer/core/script_tools/model_context_test.cc
Line 205, Patchset 13 (Latest): window.submit_event_fired = true;
Anders Hartvoll Ruud . unresolved

Maybe we should save `e.agentInvoked` on window and check that as well? Right now, `agentInvoked` is completely ignored by tests.

Open in Gerrit

Related details

Attention is currently required from:
  • Dominic Farolino
  • Mason Freed
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
    Gerrit-Change-Number: 7467862
    Gerrit-PatchSet: 13
    Gerrit-Owner: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-Comment-Date: Sat, 24 Jan 2026 06:45:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Dominic Farolino (Gerrit)

    unread,
    Jan 26, 2026, 9:54:18 AMJan 26
    to Mason Freed, Daniel Cheng, Anders Hartvoll Ruud, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Daniel Cheng and Mason Freed

    Dominic Farolino added 3 comments

    Patchset-level comments
    File-level comment, Patchset 13 (Latest):
    Dominic Farolino . resolved

    dcheng@ can you PTAL at my comment below?

    File third_party/blink/renderer/core/html/forms/html_form_element.cc
    Line 575, Patchset 13 (Latest): ScriptState* script_state = ToScriptStateForMainWorld(frame);
    Dominic Farolino . unresolved

    While I know that this fixes the crash we observed in the test site, this worries me.

    OK, so the flow is:

    1. A form is submitted, possibly by an extension.
    2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
    3. We construct a SubmitEvent, and fire `submit` event handlers
    4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
    5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
    6. The Promise passed into respondWith() is constructed with a script state associated with that same world
    7. We store the Promise temporarily on SubmitEvent
    8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world

    Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).

    I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.

    Line 580, Patchset 13 (Latest): active_webmcp_tool_, /*resolved*/ true);
    Anders Hartvoll Ruud . unresolved

    nit: `/*resolved=*/` is better, since it's supported by clang-tidy.

    https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html

    Dominic Farolino
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Daniel Cheng
    • Mason Freed
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
    Gerrit-Change-Number: 7467862
    Gerrit-PatchSet: 13
    Gerrit-Owner: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
    Gerrit-Comment-Date: Mon, 26 Jan 2026 14:54:10 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Anders Hartvoll Ruud <and...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Mason Freed (Gerrit)

    unread,
    Jan 26, 2026, 2:00:15 PMJan 26
    to Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Daniel Cheng and Dominic Farolino

    Mason Freed voted and added 1 comment

    Votes added by Mason Freed

    Auto-Submit+1

    1 comment

    File third_party/blink/renderer/core/html/forms/html_form_element.cc
    Line 575, Patchset 13 (Latest): ScriptState* script_state = ToScriptStateForMainWorld(frame);
    Dominic Farolino . unresolved

    While I know that this fixes the crash we observed in the test site, this worries me.

    OK, so the flow is:

    1. A form is submitted, possibly by an extension.
    2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
    3. We construct a SubmitEvent, and fire `submit` event handlers
    4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
    5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
    6. The Promise passed into respondWith() is constructed with a script state associated with that same world
    7. We store the Promise temporarily on SubmitEvent
    8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world

    Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).

    I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.

    Mason Freed

    Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.

    Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?

    Thanks for looping in dcheng@ - curious what he thinks.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Daniel Cheng
    • Dominic Farolino
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
    Gerrit-Change-Number: 7467862
    Gerrit-PatchSet: 13
    Gerrit-Owner: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
    Gerrit-Comment-Date: Mon, 26 Jan 2026 19:00:05 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Mason Freed (Gerrit)

    unread,
    Jan 26, 2026, 5:39:56 PMJan 26
    to Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Anders Hartvoll Ruud, Daniel Cheng and Dominic Farolino

    Mason Freed voted and added 5 comments

    Votes added by Mason Freed

    Auto-Submit+1

    5 comments

    File chrome/browser/actor/tools/script_tool_browsertest.cc
    Line 192, Patchset 13: // EXPECT_EQ(action_results.at(0).result->script_tool_response->result,
    // "val");
    Anders Hartvoll Ruud . resolved

    nit: Is this useful? Can't we actually check the respondWidth result now?

    Mason Freed

    I'll remove this, mostly to avoid needing another reviewer. But also because we're kind of abandoning this test in favor of the unit test.

    File third_party/blink/renderer/core/html/forms/html_form_element.cc
    Line 575, Patchset 13: ScriptState* script_state = ToScriptStateForMainWorld(frame);
    Dominic Farolino . resolved

    While I know that this fixes the crash we observed in the test site, this worries me.

    OK, so the flow is:

    1. A form is submitted, possibly by an extension.
    2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
    3. We construct a SubmitEvent, and fire `submit` event handlers
    4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
    5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
    6. The Promise passed into respondWith() is constructed with a script state associated with that same world
    7. We store the Promise temporarily on SubmitEvent
    8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world

    Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).

    I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.

    Mason Freed

    Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.

    Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?

    Thanks for looping in dcheng@ - curious what he thinks.

    Mason Freed

    I'm hoping we can land this and come back to this problem if dcheng (or you) thinks there is one. But there is a ton of code landing, and this keeps getting merge failures while it sits waiting. I'd like to land it.

    Line 580, Patchset 13: active_webmcp_tool_, /*resolved*/ true);
    Anders Hartvoll Ruud . resolved

    nit: `/*resolved=*/` is better, since it's supported by clang-tidy.

    https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html

    Dominic Farolino

    +1. It also matches the examples in https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments.

    Mason Freed

    Done

    Line 593, Patchset 13: rejected = MakeGarbageCollected<RespondWithHandler>(
    active_webmcp_tool_, /*resolved*/ false);
    Anders Hartvoll Ruud . resolved

    Isn't this creating exactly the same handler already held by `rejected`? Did you mean:

    ```suggestion
    resolved = rejected;
    ```

    ?

    We should probably add a test for this branch.

    Mason Freed
    File third_party/blink/renderer/core/script_tools/model_context_test.cc
    Line 205, Patchset 13: window.submit_event_fired = true;
    Anders Hartvoll Ruud . resolved

    Maybe we should save `e.agentInvoked` on window and check that as well? Right now, `agentInvoked` is completely ignored by tests.

    Mason Freed

    Tested in the next patchset. Look for `window.agent_invoked`.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Daniel Cheng
    • Dominic Farolino
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
    Gerrit-Change-Number: 7467862
    Gerrit-PatchSet: 16
    Gerrit-Owner: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
    Gerrit-Comment-Date: Mon, 26 Jan 2026 22:39:47 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Mason Freed <mas...@chromium.org>
    Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Mason Freed (Gerrit)

    unread,
    Jan 26, 2026, 6:24:34 PMJan 26
    to Ben Greenstein, Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Anders Hartvoll Ruud, Ben Greenstein, Daniel Cheng and Dominic Farolino

    Mason Freed voted and added 1 comment

    Votes added by Mason Freed

    Auto-Submit+1

    1 comment

    Patchset-level comments
    File-level comment, Patchset 17 (Latest):
    Mason Freed . resolved

    +bengr for timely review. I'd like to land this one since it'd gating 3 others with the rest of the functionality. I'm happy to handle any followups as followup CLs.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Ben Greenstein
    • Daniel Cheng
    • Dominic Farolino
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
    Gerrit-Change-Number: 7467862
    Gerrit-PatchSet: 17
    Gerrit-Owner: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Ben Greenstein <be...@chromium.org>
    Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Attention: Ben Greenstein <be...@chromium.org>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
    Gerrit-Comment-Date: Mon, 26 Jan 2026 23:24:27 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Ben Greenstein (Gerrit)

    unread,
    Jan 26, 2026, 7:55:44 PMJan 26
    to Mason Freed, Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
    Attention needed from Anders Hartvoll Ruud, Daniel Cheng, Dominic Farolino and Mason Freed

    Ben Greenstein voted

    Code-Review+1
    Commit-Queue+2
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Daniel Cheng
    • Dominic Farolino
    • Mason Freed
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
      Gerrit-Change-Number: 7467862
      Gerrit-PatchSet: 17
      Gerrit-Owner: Mason Freed <mas...@chromium.org>
      Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
      Gerrit-Reviewer: Ben Greenstein <be...@chromium.org>
      Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
      Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
      Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
      Gerrit-Attention: Mason Freed <mas...@chromium.org>
      Gerrit-Attention: Dominic Farolino <d...@chromium.org>
      Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
      Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
      Gerrit-Comment-Date: Tue, 27 Jan 2026 00:55:19 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Daniel Cheng (Gerrit)

      unread,
      Jan 26, 2026, 8:07:25 PMJan 26
      to Mason Freed, Ben Greenstein, Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
      Attention needed from Anders Hartvoll Ruud, Dominic Farolino and Mason Freed

      Daniel Cheng added 1 comment

      File third_party/blink/renderer/core/html/forms/html_form_element.cc
      Line 575, Patchset 13: ScriptState* script_state = ToScriptStateForMainWorld(frame);
      Dominic Farolino . unresolved

      While I know that this fixes the crash we observed in the test site, this worries me.

      OK, so the flow is:

      1. A form is submitted, possibly by an extension.
      2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
      3. We construct a SubmitEvent, and fire `submit` event handlers
      4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
      5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
      6. The Promise passed into respondWith() is constructed with a script state associated with that same world
      7. We store the Promise temporarily on SubmitEvent
      8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world

      Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).

      I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.

      Mason Freed

      Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.

      Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?

      Thanks for looping in dcheng@ - curious what he thinks.

      Mason Freed

      I'm hoping we can land this and come back to this problem if dcheng (or you) thinks there is one. But there is a ton of code landing, and this keeps getting merge failures while it sits waiting. I'd like to land it.

      Daniel Cheng

      I haven't managed to figure out exactly what's going on in this CL yet, but to be clear, a cross-world leak is a security bug.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Anders Hartvoll Ruud
      • Dominic Farolino
      • Mason Freed
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
        Gerrit-Change-Number: 7467862
        Gerrit-PatchSet: 17
        Gerrit-Owner: Mason Freed <mas...@chromium.org>
        Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
        Gerrit-Reviewer: Ben Greenstein <be...@chromium.org>
        Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
        Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
        Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
        Gerrit-Attention: Mason Freed <mas...@chromium.org>
        Gerrit-Attention: Dominic Farolino <d...@chromium.org>
        Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
        Gerrit-Comment-Date: Tue, 27 Jan 2026 01:07:11 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Dominic Farolino (Gerrit)

        unread,
        Jan 27, 2026, 10:01:31 AMJan 27
        to Mason Freed, Ben Greenstein, Daniel Cheng, Anders Hartvoll Ruud, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
        Attention needed from Anders Hartvoll Ruud, Ben Greenstein and Mason Freed

        Dominic Farolino added 1 comment

        File third_party/blink/renderer/core/html/forms/html_form_element.cc
        Line 575, Patchset 13: ScriptState* script_state = ToScriptStateForMainWorld(frame);
        Dominic Farolino . unresolved

        While I know that this fixes the crash we observed in the test site, this worries me.

        OK, so the flow is:

        1. A form is submitted, possibly by an extension.
        2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
        3. We construct a SubmitEvent, and fire `submit` event handlers
        4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
        5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
        6. The Promise passed into respondWith() is constructed with a script state associated with that same world
        7. We store the Promise temporarily on SubmitEvent
        8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world

        Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).

        I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.

        Mason Freed

        Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.

        Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?

        Thanks for looping in dcheng@ - curious what he thinks.

        Dominic Farolino

        Right, it worried me because PS11 stores a world-specific ScriptState on a world-agnostic C++ object that gets accessed by multiple worlds (because it's passed to all `submit` handlers, and those handlers can be created from any world). But maybe it's fine since we don't ever expose the ScriptState itself (or anything derived from it) directly to any handlers in those worlds...

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Anders Hartvoll Ruud
        • Ben Greenstein
        • Mason Freed
        Gerrit-Attention: Ben Greenstein <be...@chromium.org>
        Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
        Gerrit-Comment-Date: Tue, 27 Jan 2026 15:01:17 +0000
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Mason Freed (Gerrit)

        unread,
        Jan 27, 2026, 10:50:08 AMJan 27
        to Ben Greenstein, Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
        Attention needed from Anders Hartvoll Ruud, Ben Greenstein, Daniel Cheng and Dominic Farolino

        Mason Freed voted and added 1 comment

        Votes added by Mason Freed

        Auto-Submit+1
        Commit-Queue+2

        1 comment

        File third_party/blink/renderer/core/html/forms/html_form_element.cc
        Line 575, Patchset 13: ScriptState* script_state = ToScriptStateForMainWorld(frame);
        Dominic Farolino . resolved

        While I know that this fixes the crash we observed in the test site, this worries me.

        OK, so the flow is:

        1. A form is submitted, possibly by an extension.
        2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
        3. We construct a SubmitEvent, and fire `submit` event handlers
        4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
        5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
        6. The Promise passed into respondWith() is constructed with a script state associated with that same world
        7. We store the Promise temporarily on SubmitEvent
        8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world

        Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).

        I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.

        Mason Freed

        Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.

        Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?

        Thanks for looping in dcheng@ - curious what he thinks.

        Dominic Farolino

        Right, it worried me because PS11 stores a world-specific ScriptState on a world-agnostic C++ object that gets accessed by multiple worlds (because it's passed to all `submit` handlers, and those handlers can be created from any world). But maybe it's fine since we don't ever expose the ScriptState itself (or anything derived from it) directly to any handlers in those worlds...

        Mason Freed

        Alright, I've gone back to the PS11 approach of storing and returning/using the script_state from the `respondWith()` caller world. From the sounds of it, that should be safe - the promise will be unwrapped in the correct world. The only risk is future users of the promise accessor (of which there aren't likely to be any) might hold it wrong.

        I can clean up the approach and make it safer-by-default as a followup. One idea would be to create a stack allocated return value for RespondWithPromise that includes the `ScriptState::Scope` so that the promise can just be directly unwrapped.

        For now, I'd really like to land this string of patches. Please keep commenting here with suggestions and issues, and I'll tackle them as followups to this chain.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Anders Hartvoll Ruud
        • Ben Greenstein
        • Daniel Cheng
        • Dominic Farolino
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
          Gerrit-Change-Number: 7467862
          Gerrit-PatchSet: 19
          Gerrit-Owner: Mason Freed <mas...@chromium.org>
          Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
          Gerrit-Reviewer: Ben Greenstein <be...@chromium.org>
          Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
          Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
          Gerrit-Attention: Ben Greenstein <be...@chromium.org>
          Gerrit-Attention: Dominic Farolino <d...@chromium.org>
          Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
          Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
          Gerrit-Comment-Date: Tue, 27 Jan 2026 15:49:57 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Mason Freed (Gerrit)

          unread,
          Jan 27, 2026, 10:50:30 AMJan 27
          to Ben Greenstein, Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org
          Attention needed from Anders Hartvoll Ruud, Ben Greenstein, Daniel Cheng and Dominic Farolino

          Mason Freed removed a vote from this change

          Removed Commit-Queue+2 by Mason Freed <mas...@chromium.org>
          Open in Gerrit

          Related details

          Attention is currently required from:
          • Anders Hartvoll Ruud
          • Ben Greenstein
          • Daniel Cheng
          • Dominic Farolino
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: deleteVote
          satisfied_requirement
          open
          diffy

          Chromium LUCI CQ (Gerrit)

          unread,
          Jan 27, 2026, 11:43:58 AMJan 27
          to Mason Freed, Ben Greenstein, Daniel Cheng, Anders Hartvoll Ruud, Dominic Farolino, AyeAye, chromium...@chromium.org, android-web...@chromium.org, ashleynewson+w...@chromium.org, mfoltz+wa...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

          Chromium LUCI CQ submitted the change with unreviewed changes

          Unreviewed changes

          17 is the latest approved patch-set.
          The change was submitted with unreviewed changes in the following files:

          ```
          The name of the file: third_party/blink/renderer/core/html/forms/submit_event.h
          Insertions: 11, Deletions: 4.

          @@ -29,13 +29,20 @@

          bool agentInvoked() const { return agent_invoked_; }
          void respondWith(ScriptState*, ScriptPromise<IDLAny>, ExceptionState&);
          - MemberScriptPromise<IDLAny> RespondWithPromise() const {
          - return respond_with_promise_;
          - }
          +
          + // NOTE: The promise returned by this getter must be executed using the
          + // script_state also returned by the getter, and not with any other
          + // script_state. Doing so would be a cross-world security leak. E.g.,
          + // auto val = submit_event->RespondWithPromise();
          + // ScriptState::Scope scope(val->second);
          + // val->first.Unwrap()...
          + std::optional<std::pair<MemberScriptPromise<IDLAny>, Member<ScriptState>>>
          + RespondWithPromise() const;

          private:
          Member<HTMLElement> submitter_;
          - MemberScriptPromise<IDLAny> respond_with_promise_;
          + std::pair<MemberScriptPromise<IDLAny>, Member<ScriptState>>
          + respond_with_promise_;
          bool agent_invoked_ = false;
          };

          ```
          ```
          The name of the file: third_party/blink/renderer/core/html/forms/submit_event.cc
          Insertions: 10, Deletions: 1.

          @@ -41,7 +41,16 @@
          if (!IsBeingDispatched()) {
          LOG(ERROR) << "ERROR - event needs to be being dispatched.";
          }
          - respond_with_promise_ = script_promise;
          + respond_with_promise_ = {script_promise, script_state};
          +}
          +
          +std::optional<std::pair<MemberScriptPromise<IDLAny>, Member<ScriptState>>>
          +SubmitEvent::RespondWithPromise() const {
          + if (respond_with_promise_.first.IsEmpty() || !respond_with_promise_.second ||
          + !respond_with_promise_.second->ContextIsValid()) {
          + return std::nullopt;
          + }
          + return respond_with_promise_;
          }

          } // namespace blink
          ```
          ```
          The name of the file: third_party/blink/renderer/core/html/forms/html_form_element.cc
          Insertions: 4, Deletions: 6.

          @@ -585,12 +585,10 @@
          should_submit =
          DispatchEvent(*submit_event) == DispatchEventResult::kNotCanceled;
          if (declarative_webmcp_call) {
          - if (auto promise = submit_event->RespondWithPromise();
          - !promise.IsEmpty()) {
          - ScriptState* script_state = ToScriptStateForMainWorld(frame);
          - if (!script_state || !script_state->ContextIsValid()) {
          - return;
          - }
          + if (auto promise_and_script_state = submit_event->RespondWithPromise();
          + promise_and_script_state.has_value()) {
          + auto promise = promise_and_script_state->first;
          + auto script_state = promise_and_script_state->second;
          auto* resolved = MakeGarbageCollected<RespondWithHandler>(
          active_webmcp_tool_, /*resolved=*/true);
          auto* rejected = MakeGarbageCollected<RespondWithHandler>(
          ```

          Change information

          Commit message:
          Add agentInvoked and respondWith() to the `submit` event [3/N]

          For declarative WebMCP, this CL adds two attributes to the submit
          event:
          - agentInvoked: true for declarative WebMCP calls
          - respondWith(): allows the event listener to pass back a promise
          that will resolve to the tool result.

          If respondWith is called (and the event is preventDefaulted), then
          we will wait for the promise to resolve, and then pass the value
          back to the agent.

          Fixed: 475973832
          Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
          Reviewed-by: Ben Greenstein <be...@chromium.org>
          Commit-Queue: Mason Freed <mas...@chromium.org>
          Auto-Submit: Mason Freed <mas...@chromium.org>
          Cr-Commit-Position: refs/heads/main@{#1575272}
          Files:
          • M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
          • M chrome/test/data/actor/declarative_script_tool.html
          • M third_party/blink/renderer/core/html/forms/html_form_element.cc
          • M third_party/blink/renderer/core/html/forms/html_form_element.h
          • M third_party/blink/renderer/core/html/forms/html_form_mcp_tool_test.cc
          • M third_party/blink/renderer/core/html/forms/submit_event.cc
          • M third_party/blink/renderer/core/html/forms/submit_event.h
          • M third_party/blink/renderer/core/html/forms/submit_event.idl
          • M third_party/blink/renderer/core/html/forms/submit_event_init.idl
          • M third_party/blink/renderer/core/script_tools/model_context.cc
          • M third_party/blink/renderer/core/script_tools/model_context_test.cc
          • M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
          Change size: L
          Delta: 12 files changed, 243 insertions(+), 44 deletions(-)
          Branch: refs/heads/main
          Submit Requirements:
          • requirement satisfiedCode-Review: +1 by Ben Greenstein
          Open in Gerrit
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: merged
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I11f06d8d50d37c9683dcb09c326560fcf72c2e49
          Gerrit-Change-Number: 7467862
          Gerrit-PatchSet: 20
          Gerrit-Owner: Mason Freed <mas...@chromium.org>
          Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
          Gerrit-Reviewer: Ben Greenstein <be...@chromium.org>
          Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
          Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
          Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
          Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
          open
          diffy
          satisfied_requirement
          Reply all
          Reply to author
          Forward
          0 new messages