[Extensions] Make Message move-only and add Clone() [chromium/src : main]
38 views
Skip to first unread message
Justin Lulejian (Gerrit)
Feb 4, 2026, 11:56:00 AM (7 days ago) Feb 4
to Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Justin Lulejian added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 22 (Latest):
Justin Lulejian . resolved
Hi Solomon! Had a pre-refactor for you in the structured clone messaging you reviewed before. This is to support sending Blobs over extension messaging. This change seems out of place at first glance, but makes more sense in the context of the blob support downstream change: [\[Extensions\] Add Blob support to extension messaging. (7311207)](https://crrev.com/c/7311207)
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. |
Justin Lulejian (Gerrit)
Feb 4, 2026, 7:07:17 PM (7 days ago) Feb 4
to Solomon Kinard, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Solomon Kinard
Justin Lulejian added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Justin Lulejian . resolved
Whoops! 😮 Actually adding Solomon now.
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. |
Solomon Kinard (Gerrit)
Feb 5, 2026, 3:41:13 PM (6 days ago) Feb 5
to Justin Lulejian, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Justin Lulejian
Solomon Kinard added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Solomon Kinard . resolved
A quick question.
File extensions/browser/api/messaging/extension_message_port.cc
Line 622, Patchset 22 (Latest):
[](Message* message, mojom::MessagePort* port) {Solomon Kinard . unresolved
Restore? See line 625 for more. cref() enforces constness and prevents mutation in callback.
```suggestion
[](const Message* message, mojom::MessagePort* port) {
```
Line 625, Patchset 22 (Latest):
&message));Solomon Kinard . unresolved
Should `std::cref(message)` be preferred over `&message` to make the lifetime assumptions explicit, given the risk of a dangling reference if this ever becomes async?
```suggestion
std::cref(message)));
```
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. |
Justin Lulejian (Gerrit)
Feb 5, 2026, 4:28:16 PM (6 days ago) Feb 5
to Chromium IPC Reviews, Solomon Kinard, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Chromium IPC Reviews
Justin Lulejian added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 23 (Latest):
Justin Lulejian . resolved
Hi Chromium IPC Reviews! Could you review the //extensions/common/mojom.* changes?
File extensions/browser/api/messaging/extension_message_port.cc
Restore? See line 625 for more. cref() enforces constness and prevents mutation in callback.
```suggestion
[](const Message* message, mojom::MessagePort* port) {
```
Justin Lulejian
Done
Should `std::cref(message)` be preferred over `&message` to make the lifetime assumptions explicit, given the risk of a dangling reference if this ever becomes async?
```suggestion
std::cref(message)));
```
Attention is currently required from:
- Chromium IPC Reviews
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Feb 5, 2026, 4:31:10 PM (6 days ago) Feb 5
to Justin Lulejian, Chromium IPC Reviews, Giovanni Ortuno Urquidi, Solomon Kinard, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Giovanni Ortuno Urquidi and Justin Lulejian
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
Attention is currently required from:
- Giovanni Ortuno Urquidi
- 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. |
Giovanni Ortuno Urquidi (Gerrit)
Feb 6, 2026, 10:59:48 AM (5 days ago) Feb 6
to Justin Lulejian, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Solomon Kinard, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Justin Lulejian and Solomon Kinard
Giovanni Ortuno Urquidi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Ortuno Urquidi
| Code-Review | +1 |
1 comment
Patchset-level comments
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. |
Solomon Kinard (Gerrit)
Feb 6, 2026, 2:51:57 PM (5 days ago) Feb 6
to Justin Lulejian, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Justin Lulejian
Solomon Kinard voted and added 1 comment![]( "Open in Gerrit")
Votes added by Solomon Kinard
| Code-Review | +1 |
1 comment
File extensions/browser/api/messaging/extension_message_port.cc
Line 618, Patchset 23 (Latest):
// We use a raw pointer to `message` here because `SendToPort` executesSolomon Kinard . unresolved
Was the raw pointer reverted?
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. |
Justin Lulejian (Gerrit)
Feb 9, 2026, 3:54:26 PM (2 days ago) Feb 9
to Solomon Kinard, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Justin Lulejian added 1 comment![]( "Open in Gerrit")
File extensions/browser/api/messaging/extension_message_port.cc
Line 618, Patchset 23:
// We use a raw pointer to `message` here because `SendToPort` executesSolomon Kinard . resolved
Was the raw pointer reverted?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Feb 9, 2026, 9:17:07 PM (2 days ago) Feb 9
to Solomon Kinard, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Raphael Kubo da Costa, Kentaro Hara, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Justin Lulejian voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Feb 9, 2026, 9:22:32 PM (2 days ago) Feb 9
to Justin Lulejian, Solomon Kinard, Giovanni Ortuno Urquidi, Chromium IPC Reviews, Raphael Kubo da Costa, Kentaro Hara, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jbroma...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
23 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: extensions/browser/api/messaging/extension_message_port.cc
Insertions: 0, Deletions: 4.
@@ -614,10 +614,6 @@
// Since we are now receiving a message, we can mark any asynchronous reply
// that may have been pending for this port as no longer pending.
asynchronous_reply_pending_ = false;
-
- // We use a raw pointer to `message` here because `SendToPort` executes
- // synchronously (iterating over `frames_` and `service_workers_`), ensuring
- // `message` remains valid for the duration of the broadcast.
SendToPort(base::BindRepeating(
[](const Message& message, mojom::MessagePort* port) {
port->DeliverMessage(message.Clone());
```
Change information
Commit message:
[Extensions] Make Message move-only and add Clone()
Refactor extensions::Message to be a move-only type and add an explicit
Clone() method. This is a prerequisite for supporting JS blobs in
extension messages, which will require switching the underlying storage
from mojo_base::BigBuffer to blink::CloneableMessage (which contains
move-only payload types like Mojo remote handles).
Explicit copying via Clone() is moved to call sites where it is strictly
necessary, such as when broadcasting a message to multiple listener
contexts (e.g., ExtensionMessagePort::DispatchOnMessage).
Key changes:
- Make Message move-only; add Clone().
- Pass Message by value (using std::move) in routing logic (e.g.,
MessageService, MessagePort) instead of by const reference.
- Change std::unique_ptr<Message> return type of MessageFromV8 to
std::optional<Message> to reduce allocation overhead.
- Update MessageService::PendingMessage to be a struct instead of a
typedef for std::pair (per the style guide (preferring named members
over std::pair/tuples)
- Update tests to accommodate move-only semantics and use
testing::Property.
This change should be architectural-only and maintains the existing
JSON(string)/StructureCloned(BigBuffer) serialization logic.
Low-Coverage-Reason: COVERAGE_UNDERREPORTED message_util.cc missing coverage is tested by browser_tests matching "*MessageSerialization*".
Bug: 40321352
Change-Id: I84d32b6dd851b50250e3f9b9be5e71a8da432ff3
Commit-Queue: Justin Lulejian <jlul...@chromium.org>
Reviewed-by: Giovanni Ortuno Urquidi <ort...@chromium.org>
Reviewed-by: Solomon Kinard <solomo...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1582204}
Files:
- M chrome/renderer/extensions/api/extension_hooks_delegate.cc
- M chrome/renderer/extensions/api/tabs_hooks_delegate.cc
- M extensions/browser/api/messaging/extension_message_port.cc
- M extensions/browser/api/messaging/extension_message_port.h
- M extensions/browser/api/messaging/message_port.cc
- M extensions/browser/api/messaging/message_port.h
- M extensions/browser/api/messaging/message_service.cc
- M extensions/browser/api/messaging/message_service.h
- M extensions/browser/api/messaging/native_message_port.cc
- M extensions/browser/api/messaging/native_message_port.h
- M extensions/common/api/messaging/message.cc
- M extensions/common/api/messaging/message.h
- M extensions/common/mojom/message_port_mojom_traits.cc
- M extensions/common/mojom/message_port_mojom_traits.h
- M extensions/renderer/api/messaging/gin_port.cc
- M extensions/renderer/api/messaging/gin_port.h
- M extensions/renderer/api/messaging/gin_port_unittest.cc
- M extensions/renderer/api/messaging/messaging_util.cc
- M extensions/renderer/api/messaging/messaging_util.h
- M extensions/renderer/api/messaging/messaging_util_unittest.cc
- M extensions/renderer/api/messaging/native_renderer_messaging_service.cc
- M extensions/renderer/api/messaging/native_renderer_messaging_service.h
- M extensions/renderer/api/messaging/native_renderer_messaging_service_unittest.cc
- M extensions/renderer/api/messaging/one_time_message_handler.cc
- M extensions/renderer/api/messaging/one_time_message_handler.h
- M extensions/renderer/api/messaging/one_time_message_handler_unittest.cc
- M extensions/renderer/api/messaging/send_message_tester.cc
- M extensions/renderer/api/runtime_hooks_delegate.cc
Change size: L
Delta: 28 files changed, 283 insertions(+), 222 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Solomon Kinard, +1 by Giovanni Ortuno Urquidi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages