Justin Lulejian (Gerrit)

unread,

Mar 3, 2026, 3:28:26 PM (9 days ago) Mar 3

to Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Solomon Kinard

Justin Lulejian voted and added 1 comment

Votes added by Justin Lulejian

Auto-Submit

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Justin Lulejian . resolved

Hi Solomon! Could you review the extensions/.* changes please?

Open in Gerrit

Related details

Attention is currently required from:

Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Solomon Kinard (Gerrit)

unread,

Mar 5, 2026, 11:59:35 PM (7 days ago) Mar 5

to Justin Lulejian, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Justin Lulejian

Solomon Kinard voted and added 4 comments

Votes added by Solomon Kinard

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Solomon Kinard . resolved

extensions lgtm from a high level, with a few questions for my own understanding.

File extensions/renderer/api/messaging/messaging_util.cc

Line 158, Patchset 4 (Latest): return StructuredCloneMessageData();

Solomon Kinard . unresolved

Is it ok to return raw message data on error or would an optional SCMD be beneficial?

Line 260, Patchset 4 (Latest): return v8::Local<v8::Value>();

Solomon Kinard . unresolved

Is it ok to return an empty handle or is an explicit null ok `v8::Null(&isolate);` ?

File extensions/test/data/api_test/messaging/serialization/serialization_common_tests.js

Line 448, Patchset 4 (Latest): await new Promise(resolve => setTimeout(resolve, 100));

Solomon Kinard . unresolved

OOC, is there an event based way to handle this? E.g. could 100 become flaky?

Open in Gerrit

Related details

Attention is currently required from:

Justin Lulejian

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

Mar 6, 2026, 5:42:38 PM (6 days ago) Mar 6

to Dave Tapuska, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Dave Tapuska

Justin Lulejian voted and added 4 comments

Votes added by Justin Lulejian

Auto-Submit

+1

4 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Justin Lulejian . resolved

Hi Dave! Could you review the third_party/blink/.* changes?

File extensions/renderer/api/messaging/messaging_util.cc

Line 158, Patchset 4: return StructuredCloneMessageData();

Solomon Kinard . resolved

Is it ok to return raw message data on error or would an optional SCMD be beneficial?

Justin Lulejian

I think a std::optional would be clearer but out of scope for this change -- added a TODO.

Line 260, Patchset 4: return v8::Local<v8::Value>();

Solomon Kinard . resolved

Is it ok to return an empty handle or is an explicit null ok `v8::Null(&isolate);` ?

Justin Lulejian

`v8::Null(&isolate);` wouldn't be optimal because `null` is a valid JS value so it would be impossible to tell if deserialized value was actually `null` or indicating an error. Added a TODO to make this an optional as well to be more clear since it seems callers check the return or the error which isn't great...

File extensions/test/data/api_test/messaging/serialization/serialization_common_tests.js

Line 448, Patchset 4: await new Promise(resolve => setTimeout(resolve, 100));

Solomon Kinard . resolved

OOC, is there an event based way to handle this? E.g. could 100 become flaky?

Justin Lulejian

Great point! Changed to a promise that resolves when the `onDisconnected` handler fires with the deserialization failure error so it should not be flaky.

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

Mar 6, 2026, 8:28:55 PM (6 days ago) Mar 6

to Dave Tapuska, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Dave Tapuska and Solomon Kinard

Justin Lulejian voted Auto-Submit+0

Auto-Submit

+0

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

Mar 6, 2026, 8:33:25 PM (6 days ago) Mar 6

to Dave Tapuska, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Dave Tapuska and Solomon Kinard

Justin Lulejian added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Justin Lulejian . resolved

Hi Solomon! Unfortunately I realized after your +1 that I was changing messaging behavior outside of structured clone serialization with the changes I was making to gin_port.cc and one_time_message_handler.cc. Those changes cause the sender to be notified of a failed deserialization which is a behavior change for JSON serialization too.

I'll think more about whether sending the error back to the sender makes sense later, but for now I've reverted that and adjusted the tests so they don't rely on the sender getting an error to prove that the SAB fails to deserialize in the renderer. You can see the diff between patchset 4 and patchset 6.

Also since I'm removing code and confirmed tests pass locally I think we can treat this as if the trybots have passed so it is ready for re-review.

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Mar 9, 2026, 7:10:50 PM (3 days ago) Mar 9

to Justin Lulejian, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Justin Lulejian and Solomon Kinard

Dave Tapuska added 1 comment

File third_party/blink/public/web/web_serialized_script_value.h

Line 84, Patchset 7 (Latest): v8::Local<v8::Value> Deserialize(v8::Isolate*, bool* did_fail = nullptr);

Dave Tapuska . unresolved

Is it possible to change this signature to std::expected<v8::Local<v8::Value>, DeserializationError> ?

where maybe DeserializationError is a
```
enum class DeserializationError {
  kDefaultFailure,
};
```

Open in Gerrit

Related details

Attention is currently required from:

Justin Lulejian
Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

Mar 10, 2026, 1:37:12 PM (2 days ago) Mar 10

to Dave Tapuska, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Dave Tapuska and Solomon Kinard

Justin Lulejian voted and added 1 comment

Votes added by Justin Lulejian

Auto-Submit

+1

1 comment

File third_party/blink/public/web/web_serialized_script_value.h

Line 84, Patchset 7: v8::Local<v8::Value> Deserialize(v8::Isolate*, bool* did_fail = nullptr);

Dave Tapuska . resolved

Is it possible to change this signature to std::expected<v8::Local<v8::Value>, DeserializationError> ?
where maybe DeserializationError is a
```
enum class DeserializationError {
  kDefaultFailure,
};
```

Justin Lulejian

Done

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Mar 10, 2026, 1:42:23 PM (2 days ago) Mar 10

to Justin Lulejian, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Justin Lulejian and Solomon Kinard

Dave Tapuska added 1 comment

File third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_deserializer.h

Line 134, Patchset 8 (Latest): bool* did_fail_ = nullptr;

Dave Tapuska . unresolved

Let's change this to just a bool has_error_ = false;

Then add an API that can ask for HasError()...

Deserialize doesn't support multiple calls.

Open in Gerrit

Related details

Attention is currently required from:

Justin Lulejian
Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

Mar 10, 2026, 7:09:21 PM (2 days ago) Mar 10

to Jerome Jiang, Mirko Bonadei, AyeAye, Dave Tapuska, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, net-r...@chromium.org, chrome-intelligence-te...@google.com, titoua...@chromium.org, devtools...@chromium.org, mar...@chromium.org, ortuno...@chromium.org, fgal...@chromium.org, cblume...@chromium.org, jz...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Dave Tapuska and Solomon Kinard

Justin Lulejian voted and added 1 comment

Votes added by Justin Lulejian

Auto-Submit

+1

1 comment

File third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_deserializer.h

Line 134, Patchset 8: bool* did_fail_ = nullptr;

Dave Tapuska . resolved

Let's change this to just a bool has_error_ = false;
Then add an API that can ask for HasError()...
Deserialize doesn't support multiple calls.

Justin Lulejian

Done

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Mar 11, 2026, 10:27:59 PM (yesterday) Mar 11

to Justin Lulejian, Jerome Jiang, Mirko Bonadei, AyeAye, Solomon Kinard, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, net-r...@chromium.org, chrome-intelligence-te...@google.com, titoua...@chromium.org, devtools...@chromium.org, mar...@chromium.org, ortuno...@chromium.org, fgal...@chromium.org, cblume...@chromium.org, jz...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Justin Lulejian and Solomon Kinard

Dave Tapuska voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Justin Lulejian
Solomon Kinard

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Solomon Kinard (Gerrit)

unread,

10:13 AM (14 hours ago) 10:13 AM

to Justin Lulejian, Dave Tapuska, Jerome Jiang, Mirko Bonadei, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, net-r...@chromium.org, chrome-intelligence-te...@google.com, titoua...@chromium.org, devtools...@chromium.org, mar...@chromium.org, ortuno...@chromium.org, fgal...@chromium.org, cblume...@chromium.org, jz...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Attention needed from Dave Tapuska and Justin Lulejian

Solomon Kinard voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Justin Lulejian

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

10:17 AM (14 hours ago) 10:17 AM

to Solomon Kinard, Dave Tapuska, Jerome Jiang, Mirko Bonadei, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, net-r...@chromium.org, chrome-intelligence-te...@google.com, titoua...@chromium.org, devtools...@chromium.org, mar...@chromium.org, ortuno...@chromium.org, fgal...@chromium.org, cblume...@chromium.org, jz...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Justin Lulejian voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Justin Lulejian (Gerrit)

unread,

10:42 AM (13 hours ago) 10:42 AM

to Solomon Kinard, Dave Tapuska, Jerome Jiang, Mirko Bonadei, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, net-r...@chromium.org, chrome-intelligence-te...@google.com, titoua...@chromium.org, devtools...@chromium.org, mar...@chromium.org, ortuno...@chromium.org, fgal...@chromium.org, cblume...@chromium.org, jz...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Justin Lulejian voted

Auto-Submit	+1
Commit-Queue	+0

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

11:15 AM (13 hours ago) 11:15 AM

to Justin Lulejian, Solomon Kinard, Dave Tapuska, Jerome Jiang, Mirko Bonadei, AyeAye, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, net-r...@chromium.org, chrome-intelligence-te...@google.com, titoua...@chromium.org, devtools...@chromium.org, mar...@chromium.org, ortuno...@chromium.org, fgal...@chromium.org, cblume...@chromium.org, jz...@chromium.org, penghuan...@chromium.org, chrome-intell...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, jbroma...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[Extensions] Error on SharedArrayBuffer's in extension messaging

Extension messaging transports structured clone payloads via the
`blink.mojom.CloneableMessage` IPC struct, which drops SharedArrayBuffer
(SAB) memory handles over IPC and therefore means they can't be sent
with extension messaging.

Previously:
* For top-level SABs: `sendMessage(<SharedArrayBuffer>)`
   * Sending a SAB would appear to succeed serialization in the sender,
   but silently resolve to `null` on the receiver side when
   deserialization failed to find the dropped handle.
* For embedded SABs: `sendMessage({'key': <SharedArrayBuffer>})`
   * Any structured clone deserialization failure in Blink was caught
   and swallowed by `V8ScriptValueDeserializer`, simply returning
   `v8::Null()`. Extension messaging could not distinguish this failure
   from an intentional `sendMessage(null)`.

This CL updates the architecture to correctly handle and report these
errors:
* For top-level SABs:
   * `MessageToV8UsingStructuredClone()` now explicitly rejects
   top-level SABs before serialization, surfacing a synchronous error
   to the sender.
* For embedded SABs:
   * `V8ScriptValueDeserializer` and `WebSerializedScriptValue` now
   expose an optional `did_fail` out-parameter.
   * The extension messaging receiver renderer uses `did_fail` to detect
   the deserialization error that occurs for embedded SABs. It logs a
   console message of the failure, rather than serialize it as JS
   `null`. We don't close the port/channel when this occurs. To do so
   would change all messaging behavior. and surface a "Could not
   deserialize message" error to inform the receiver of the error.

Low-Coverage-Reason: COVERAGE_UNDERREPORTED browsertests are not reported all "uncovered" lines have test cases in messaging_apitest.cc

Bug: 40321352

Change-Id: Ib07f8a4eab97bd544e612f8590eed8d4082a640a

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7624387

Auto-Submit: Justin Lulejian <jlul...@chromium.org>

Reviewed-by: Solomon Kinard <solomo...@chromium.org>

Commit-Queue: Justin Lulejian <jlul...@chromium.org>

Reviewed-by: Dave Tapuska <dtap...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1598442}

Files:

M extensions/renderer/api/messaging/messaging_util.cc
M extensions/renderer/api/messaging/messaging_util_unittest.cc
M extensions/renderer/utils_native_handler.cc
M extensions/test/data/api_test/messaging/serialization/serialization_common_tests.js
M extensions/test/data/api_test/messaging/serialization_sab/test_receiver.js
M extensions/test/data/api_test/messaging/serialization_sab/test_sender.js
M third_party/blink/public/web/web_serialized_script_value.h
M third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.h
M third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value_factory.cc
M third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_deserializer.cc
M third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_deserializer.h
M third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_serializer_test.cc
M third_party/blink/renderer/bindings/modules/v8/serialization/serialized_script_value_for_modules_factory.cc
M third_party/blink/renderer/core/exported/web_serialized_script_value.cc
M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py

Change size: L

Delta: 15 files changed, 220 insertions(+), 33 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Dave Tapuska, +1 by Solomon Kinard

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[Extensions] Error on SharedArrayBuffer's in extension messaging [chromium/src : main]

Justin Lulejian (Gerrit)

Justin Lulejian voted and added 1 comment

Votes added by Justin Lulejian

1 comment

Related details

Solomon Kinard (Gerrit)

Solomon Kinard voted and added 4 comments

Votes added by Solomon Kinard

4 comments

Related details

Justin Lulejian (Gerrit)

Justin Lulejian voted and added 4 comments

Votes added by Justin Lulejian

4 comments

Related details

Justin Lulejian (Gerrit)

Justin Lulejian voted Auto-Submit+0

Related details

Justin Lulejian (Gerrit)

Justin Lulejian added 1 comment

Dave Tapuska (Gerrit)

Dave Tapuska added 1 comment

Related details

Justin Lulejian (Gerrit)

Justin Lulejian voted and added 1 comment

Votes added by Justin Lulejian

1 comment

Related details

Dave Tapuska (Gerrit)

Dave Tapuska added 1 comment

Related details

Justin Lulejian (Gerrit)

Justin Lulejian voted and added 1 comment

Votes added by Justin Lulejian

1 comment

Related details

Dave Tapuska (Gerrit)

Dave Tapuska voted

Related details

Solomon Kinard (Gerrit)

Solomon Kinard voted Code-Review+1

Related details

Justin Lulejian (Gerrit)

Justin Lulejian voted Commit-Queue+2

Related details

Justin Lulejian (Gerrit)

Justin Lulejian voted

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information