[Extensions] Add Blob support to extension messaging. [chromium/src : main]
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
Hi Solomon (again 😄)! Here's the actual Blob support impl now that we can use CloneableMessage.
Related details
- Solomon Kinard
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)
Solomon Kinard added 7 comments![]( "Open in Gerrit")
A few quick questions.
// (UUID, content type, and size) to determine identity, rather thanAny risks to comparing for identity without checking for content equality?
using StructuredCloneMessageWireData = mojo_base::BigBuffer;OOC, why drop Wire?
if (this_msg.blobs[i]->uuid != other_msg.blobs[i]->uuid ||
this_msg.blobs[i]->content_type !=
other_msg.blobs[i]->content_type ||
this_msg.blobs[i]->size != other_msg.blobs[i]->size) {
return false;
}Ok to avoid relying on the possibility of CSE (Common Subexpression Elimination), or `this_msg_blob` with `other_msg_blob`, in case it's easier to read?
```suggestion
const auto& blob_a = *this_msg.blobs[i];
const auto& blob_b = *other_msg.blobs[i];
if (blob_a.uuid != blob_b.uuid ||
blob_a.content_type != blob_b.content_type ||
blob_a.size != blob_b.size) {
return false;
}
```
"uuid", "content_type", 10, std::move(blob_remote)));? here and elsewhere?
```suggestion
"uuid", "content_type", /*size=*/10, std::move(blob_remote)));
```
if (one_time_message_handler_.HasPort(script_context, target_port_id)) {Is it possible/likely to reach this point where the context does not have a port with with this specified id?
let blobMessagePromise = sendMessage(blobMessage);
let blobResponse = await blobMessagePromise;?
```suggestion
let blobResponse = await sendMessage(blobMessage);
```
Related details
- Justin Lulejian
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)
Justin Lulejian added 6 comments![]( "Open in Gerrit")
// (UUID, content type, and size) to determine identity, rather thanAny risks to comparing for identity without checking for content equality?
Actually I assumed UUID was a unguessable token, but it is not. it's a [base::UUID but stored as a string](https://source.chromium.org/chromium/chromium/src/+/main:base/uuid.h;l=31-32;drc=8393737e4619e84795f4c565912e1b044fef2a82) so I suspect there could be collisions at some point. Thankfully we actually don't use this operator (except in tests) so I'm going to create a testing method and delete the operator to avoid any confusion.
using StructuredCloneMessageWireData = mojo_base::BigBuffer;Justin LulejianOOC, why drop Wire?
It changed it because the underlying type changed from mojo_base::BigBuffer to blink::CloneableMessage. CloneableMessage is a higher-level object that can contain mojo remotes for Blobs, so the suffix 'Wire'(format) felt less accurate and data is more consistent with data() and message_data().
if (this_msg.blobs[i]->uuid != other_msg.blobs[i]->uuid ||
this_msg.blobs[i]->content_type !=
other_msg.blobs[i]->content_type ||
this_msg.blobs[i]->size != other_msg.blobs[i]->size) {
return false;
}Ok to avoid relying on the possibility of CSE (Common Subexpression Elimination), or `this_msg_blob` with `other_msg_blob`, in case it's easier to read?
```suggestion
const auto& blob_a = *this_msg.blobs[i];
const auto& blob_b = *other_msg.blobs[i];if (blob_a.uuid != blob_b.uuid ||
blob_a.content_type != blob_b.content_type ||
blob_a.size != blob_b.size) {
return false;
}
```
Done
"uuid", "content_type", 10, std::move(blob_remote)));? here and elsewhere?
```suggestion
"uuid", "content_type", /*size=*/10, std::move(blob_remote)));
```
Done
if (one_time_message_handler_.HasPort(script_context, target_port_id)) {Is it possible/likely to reach this point where the context does not have a port with with this specified id?
No, due to the earlier check in [DeliverMessageToScriptContext](https://source.chromium.org/chromium/chromium/src/+/main:extensions/renderer/api/messaging/native_renderer_messaging_service.cc;l=498;drc=8393737e4619e84795f4c565912e1b044fef2a82) that returns early if there's no one-time message or long lived port. So one of them will have it, but this function assumes it could be either.
let blobMessagePromise = sendMessage(blobMessage);
let blobResponse = await blobMessagePromise;?
```suggestion
let blobResponse = await sendMessage(blobMessage);
```
- Solomon Kinard
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)
Solomon Kinard voted and added 1 comment![]( "Open in Gerrit")
Votes added by Solomon Kinard
| Code-Review | +1 |
1 comment
// `message` no longer has valid message data!OOC, why? optional: add a few words to the comment e.g. ` because ...`
Related details
- Justin Lulejian
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)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
Hi Chromium IPC Reviews! Could you review
extensions/common/mojom/.* please?
Related details
- Chromium IPC Reviews
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: d...@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): d...@chromium.org
Reviewer source(s):
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dominic Farolino
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)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
OOC, why? optional: add a few words to the comment e.g. ` because ...`
The goal here was to avoid copying the `extensions::Message` `data_` field, which can be significant in size; so TakeStructuredMessage() `std::move`s `data_`. After the std::move `data_` no longer points to valid data. Appended the explanation to the comment.
Related details
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |