Do not send DOM model to renderer on model update [chromium/src : main]
Seung Jae Lim (Gerrit)
Seung Jae Lim added 1 comment![]( "Open in Gerrit")
Related details
- Xinghui Lu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xinghui Lu (Gerrit)
Xinghui Lu voted and added 4 comments![]( "Open in Gerrit")
Votes added by Xinghui Lu
| Code-Review | +1 |
4 comments
base::MappedReadOnlyRegion mapped_region_We might be able to get one step further by not copying the model to `mapped_region_` to save more memory. What's the plan for this?
// version. Once the metadata has been migrated to internal
// services, the fields will be derived through that instead of theI don't understand what internal services mean. Could you elaborate on this?
std::unique_ptr<Scorer> Scorer::Create(base::File visual_tflite_model) {Nit: I think it is less error-prone to pass in the width and height explicitly in this function rather than relying on external callers to call SetClassificationDimensions
Related details
- Seung Jae Lim
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. |
Seung Jae Lim (Gerrit)
Seung Jae Lim voted and added 4 comments![]( "Open in Gerrit")
Votes added by Seung Jae Lim
| Commit-Queue | +1 |
Adding Chromium IPC reviewer for safe_browsing.mojom change.
We might be able to get one step further by not copying the model to `mapped_region_` to save more memory. What's the plan for this?
Yes, this will be part 3 of the deprecation where we no longer rely on the flatbuffer model. Sadly, we still need it as part of this change in order to retrieve the thresholds for image classification, and dimensions for embedding and classification.
// version. Once the metadata has been migrated to internal
// services, the fields will be derived through that instead of theI don't understand what internal services mean. Could you elaborate on this?
This is for the internal service that distributes the model, aka optimization guide. I can clarify that here.
std::unique_ptr<Scorer> Scorer::Create(base::File visual_tflite_model) {Nit: I think it is less error-prone to pass in the width and height explicitly in this function rather than relying on external callers to call SetClassificationDimensions
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
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: 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
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
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
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Please add lots more documentation about what this CL does and how it does it, to help me review.
Related details
- Seung Jae Lim
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. |
Seung Jae Lim (Gerrit)
Seung Jae Lim added 1 comment![]( "Open in Gerrit")
Please add lots more documentation about what this CL does and how it does it, to help me review.
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
I am very sorry, but I just will not have any time to review this change this week, I am very swamped trying to catch up on a number of time-sensitive things. you might want to find another security reviewer for this. Sorry, again.
Related details
- Seung Jae Lim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seung Jae Lim (Gerrit)
Seung Jae Lim voted and added 2 comments![]( "Open in Gerrit")
Votes added by Seung Jae Lim
| Commit-Queue | +1 |
I am very sorry, but I just will not have any time to review this change this week, I am very swamped trying to catch up on a number of time-sensitive things. you might want to find another security reviewer for this. Sorry, again.
No worries, thank you for letting me know!
Hi @chrome-ip...@google.com, would you be able to take a look at `safe_browsing.mojom`? Thanks!
Related details
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
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: joenot...@google.com
📎 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): joenot...@google.com
Reviewer source(s):
joenot...@google.com is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 5 comments![]( "Open in Gerrit")
// embedding model is converted from a TfLite file to a string in the browserCan you make this comment more clear? I can't tell when the conversion to a string happens, or how it affects this method, since none of the arguments are strings. Is the model converted to a string and then written to a file, and this method gets a handle to the file?
(I see the comment is copied from SetImageEmbeddingAndPhishingFlatBufferModel, and it doesn't make much sense to me there either. Is it out of date?)
// match. Input width and height for image embedding are included in theNit: can you document what happens if the versions don't match?
scorer->AttachImageEmbeddingModel(image_embedding_input_width,The mojo comment says that this method attaches the image embedding model IF it matches the version of the scorer. But I can't find any version check. Is the check done on the browser side? (That's safer in any case, since a compromised renderer could lie about the model version.)
std::unique_ptr<Scorer> Scorer::Create(int classification_input_width,These width/height parameters aren't used in the two new Create functions. Is that intentional?
if (scorer && !scorer->image_embedding_model_.Initialize(Minor nit: I don't see any way `scorer` can be null, since if the earlier initialize fails it early returns.
Related details
- Seung Jae Lim
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. |
Seung Jae Lim (Gerrit)
Seung Jae Lim voted and added 5 comments![]( "Open in Gerrit")
Votes added by Seung Jae Lim
| Commit-Queue | +1 |
5 comments
// embedding model is converted from a TfLite file to a string in the browserCan you make this comment more clear? I can't tell when the conversion to a string happens, or how it affects this method, since none of the arguments are strings. Is the model converted to a string and then written to a file, and this method gets a handle to the file?
(I see the comment is copied from SetImageEmbeddingAndPhishingFlatBufferModel, and it doesn't make much sense to me there either. Is it out of date?)
Good point, and yes I did copy some sections from the comment from the other function. We have never converted this in the browser process, so I can't recall why this comment was stated this way, so it's removed. Changed the original as well as the new one. Please let me know how it looks now.
// match. Input width and height for image embedding are included in theNit: can you document what happens if the versions don't match?
Done. I added a comment moreso that this function should not be called if the versions don't match.
scorer->AttachImageEmbeddingModel(image_embedding_input_width,The mojo comment says that this method attaches the image embedding model IF it matches the version of the scorer. But I can't find any version check. Is the check done on the browser side? (That's safer in any case, since a compromised renderer could lie about the model version.)
Yes, this is checked [here](https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/browser/client_side_detection_service.cc;l=639;drc=16b7171f8e2cd5bb41279a7975dc9cf1f8c98b5d) on the browser side!
std::unique_ptr<Scorer> Scorer::Create(int classification_input_width,These width/height parameters aren't used in the two new Create functions. Is that intentional?
Great find. Thank you so much for this catch!
if (scorer && !scorer->image_embedding_model_.Initialize(Minor nit: I don't see any way `scorer` can be null, since if the earlier initialize fails it early returns.
- Joe Mason
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. |