Fix unsafe buffer usage in literal_buffer.h [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
// To avoid Integer OverflowBlink Style Guide: Prefer blink:: types over STL and base types. Use 'blink::wtf_size_t' instead of 'size_t' for 'count' and the 'base::CheckedNumeric' template argument to match the rest of the file.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
// To avoid Integer OverflowBlink Style Guide: Prefer blink:: types over STL and base types. Use 'blink::wtf_size_t' instead of 'size_t' for 'count' and the 'base::CheckedNumeric' template argument to match the rest of the file.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Will create a new bug to address this since this is not fixing memory safety.
Related details
- Arthur Sonzogni
- Kouhei Ueno
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
// To avoid Integer OverflowAditi PageBlink Style Guide: Prefer blink:: types over STL and base types. Use 'blink::wtf_size_t' instead of 'size_t' for 'count' and the 'base::CheckedNumeric' template argument to match the rest of the file.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Will create a new bug to address this since this is not fixing memory safety.
Done
Related details
- Arthur Sonzogni
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno added 1 comment![]( "Open in Gerrit")
Mason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Related details
- Aditi Page
- Arthur Sonzogni
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Mason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Yeah, I'm definitely concerned about the same thing. This adds a ton of `CHECK` calls that likely slow down the parser significantly.
I'll try to kick some off.
Related details
- Aditi Page
- Arthur Sonzogni
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Mason FreedMason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Yeah, I'm definitely concerned about the same thing. This adds a ton of `CHECK` calls that likely slow down the parser significantly.
I'll try to kick some off.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m4-mini-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12e6bfdf310000
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | -1 |
1 comment
Hi Aditi,
Looking at the code, it doesn't compile for multiple reasons, including `-WUnsafe-buffer-usage`. In the future, could you please:
- Compile the code locally.
- Press the Commit-queue: +1 button and wait for the commit queue results before requesting a code review.
About this patch, I don't think it would be trivial to use AI to fix the UNSAFE_TODO here. Mostly likely, they will be replaced by `UNSAFE_BUFFERS` and a comment explaining why this is safe.
BTW, this isn't the original [AI patch](https://chromium-review.googlesource.com/c/chromium/src/+/7090591) that was proposed for the rotation.
Related details
- Aditi Page
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
Mason FreedMason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Mason FreedYeah, I'm definitely concerned about the same thing. This adds a ton of `CHECK` calls that likely slow down the parser significantly.
I'll try to kick some off.
https://pinpoint-dot-chromeperf.appspot.com/job/12e6bfdf310000
- Kouhei Ueno
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
This looks good to me! Thanks for the detailed (and correct-looking) new SAFETY comments.
It's a bit unfortunate that Arthur has marked this `-1` and then gone OOO. I'm afraid that'll make it unable to land without him removing his -1. But you have my LGTM for whenever that happens.
this->Grow(new_size);nit: you don't need `this->`.
Related details
- Aditi Page
- Kouhei Ueno
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
this->Grow(new_size);nit: you don't need `this->`.
- Kouhei Ueno
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kouhei Ueno
| Code-Review | +1 |
1 comment
- Aditi Page
- Nate Chapin
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
This looks good to me! Thanks for the detailed (and correct-looking) new SAFETY comments.
It's a bit unfortunate that Arthur has marked this `-1` and then gone OOO. I'm afraid that'll make it unable to land without him removing his -1. But you have my LGTM for whenever that happens.
Thank you for the review Mason and giving me more context on the performance aspect of this code change.
Related details
- Nate Chapin
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
LGTM to unblock. Do we have the pinpoint run?
I'll trigger it. Thanks!