CrashReportStorage API uses shared memory instead of sync IPCs [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
void set(const String&, const String&, ExceptionState&);This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Please add parameter names to the `set` and `remove` function declarations. The purpose of the `String` parameters (e.g., `key`, `value`) is not obvious from the type alone. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
auto json_object = std::make_unique<JSONObject>();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Please use `WTF::MakeUnique` instead of `std::make_unique` to align with Blink's preference for WTF types over STL types. (Blink Style Guide: Prefer WTF types over STL and base types)
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Related details
- Dominic Farolino
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
ScriptPromise<IDLUndefined> create(ScriptState*,This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
For clarity, please add parameter names to the declaration of `create`. The roles of `ScriptState*` and `ExceptionState&` are not obvious from the type names alone. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Related details
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
auto json_object = std::make_unique<JSONObject>();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Prefer WTF types over STL and base types. Please use `WTF::MakeUnique` instead of `std::make_unique` for consistency within the Blink codebase.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
base::CheckedNumeric<size_t> total_size = utf8.size();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Prefer WTF types over STL and base types. The guide specifies to 'use wtf_size_t instead of size_t' when interacting with WTF types. Please use `wtf_size_t` for `total_size` as it's derived from `StringUtf8Adaptor`.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Related details
- Dominic Farolino
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
4 comments
This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
For clarity, please add parameter names to the declaration of `create`. The roles of `ScriptState*` and `ExceptionState&` are not obvious from the type names alone. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Done
void set(const String&, const String&, ExceptionState&);This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Please add parameter names to the `set` and `remove` function declarations. The purpose of the `String` parameters (e.g., `key`, `value`) is not obvious from the type alone. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Done
auto json_object = std::make_unique<JSONObject>();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Please use `WTF::MakeUnique` instead of `std::make_unique` to align with Blink's preference for WTF types over STL types. (Blink Style Guide: Prefer WTF types over STL and base types)
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Won't fix: wrong.
auto json_object = std::make_unique<JSONObject>();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Prefer WTF types over STL and base types. Please use `WTF::MakeUnique` instead of `std::make_unique` for consistency within the Blink codebase.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Won't fix: wrong.
Related details
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
auto json_object = std::make_unique<JSONObject>();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Prefer WTF types over STL and base types. Please use `WTF::MakeUnique` to create a `WTF::unique_ptr` instead of `std::make_unique`.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
base::CheckedNumeric<size_t> total_size = utf8.size();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Prefer WTF types over STL and base types. The guide states to 'use `wtf_size_t` instead of `size_t`' when interacting with WTF types. Please use `wtf_size_t` for `total_size` as it's derived from `StringUtf8Adaptor`, a WTF type.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Related details
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
3 comments
nasko@: For //content/browser
dcheng@: To weigh in on the comments I've left, and double-checks some of the shared memory buffer handling in blink::CrashReportStorage
nidhijaju@: For reporting_browsertest.cc
const uint32_t data_size = *mapping.GetMemoryAs<uint32_t>();I believe this—where the browser process starts to read from shared memory—is where we discussed the possibility of needing to copy the entire buffer before JSON parsing it, since we can't really parse its bytes atomically.
But I don't think I see a path for the renderer to still be writing to the buffer (or pass a writable/unsafe end of the buffer to another still-living renderer) such that the renderer can write to the browser while the browser is reading it.
Do you think we should still copy here, or can we get away without it? I'm asking just because the max size is 5mb, which feels non-trivial to me to maintain another copy of. But maybe it's not a big deal 🤷♂️
// The `if` statement above ensures the destination buffer is large enough.dcheng@: How do you feel about this? I feel like you're the best person to sign off on this kind of change and justification.
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
- Daniel Cheng
- Dominic Farolino
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 3 comments![]( "Open in Gerrit")
base::WritableSharedMemoryMapping mapping =Can't we just create a regular memory buffer instead of shared memory?
const uint32_t data_size = *mapping.GetMemoryAs<uint32_t>();I believe this—where the browser process starts to read from shared memory—is where we discussed the possibility of needing to copy the entire buffer before JSON parsing it, since we can't really parse its bytes atomically.
But I don't think I see a path for the renderer to still be writing to the buffer (or pass a writable/unsafe end of the buffer to another still-living renderer) such that the renderer can write to the browser while the browser is reading it.
Do you think we should still copy here, or can we get away without it? I'm asking just because the max size is 5mb, which feels non-trivial to me to maintain another copy of. But maybe it's not a big deal 🤷♂️
Let's make a copy. It is a lot easier to reason about, does not vary in semantics per platform (like shared memory does), and it is not a performance critical path.
Every time we try to ask "can we get away with it" ... inevitably someone comes later on to demonstrate why we can't : ). I'd rather we be defensive than optimistic.
if (mapping.size() >= sizeof(data_size) + data_size) {Shouldn't this be strict equality check?
Related details
- Daniel Cheng
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 8 comments![]( "Open in Gerrit")
Have you discussed OS-specific limits around shmem? How was the limit of 5MB determined? Are there any platform-specific limits we need to be concerned about? Can a renderer easily use up all allocatable shmem regions on a machine just with JS calls, whereas maybe it was harder before?
// The renderer is not trusted, so we don't trust it to write in bounds of the
// valid memory. Therefore, this region is created as unsafe.To be clear, "unsafe" in this context means "we cannot vend out a version of the region which allows readonly mappings"; it is not about spatial memory safety.
base::WritableSharedMemoryMapping mapping =Can't we just create a regular memory buffer instead of shared memory?
An unsafe shmem region can only be mapped as writable. Strictly speaking, I guess we could implement a method `MapReadOnly()` that returns a `ReadOnlySharedMemoryRegion`... but it would be read-only in name only (as the underlying bytes could still be modified).
So when we refactored the API, we chose to make this explicitly visible in the type system.
if (mapping.IsValid() && mapping.size() >= sizeof(uint32_t)) {I suggest using SpanReader, which has helpers for reading integers as well as larger spans.
const uint32_t data_size = *mapping.GetMemoryAs<uint32_t>();Nasko OskovI believe this—where the browser process starts to read from shared memory—is where we discussed the possibility of needing to copy the entire buffer before JSON parsing it, since we can't really parse its bytes atomically.
But I don't think I see a path for the renderer to still be writing to the buffer (or pass a writable/unsafe end of the buffer to another still-living renderer) such that the renderer can write to the browser while the browser is reading it.
Do you think we should still copy here, or can we get away without it? I'm asking just because the max size is 5mb, which feels non-trivial to me to maintain another copy of. But maybe it's not a big deal 🤷♂️
Let's make a copy. It is a lot easier to reason about, does not vary in semantics per platform (like shared memory does), and it is not a performance critical path.
Every time we try to ask "can we get away with it" ... inevitably someone comes later on to demonstrate why we can't : ). I'd rather we be defensive than optimistic.
We need to copy. The renderer can collaborate with another malicious renderer and send it a clone of the region, and the second malicious renderer could modify the bytes while we try to read here.
body.Set(it.first, it.second.GetString());Is it OK that this might overwrite keys that we've already set? Perhaps that's intentional though?
length,Do we anticipate using the async bits of this for other stuff in the future?
It feels a bit hard to use TBH, though I guess if we're going through promises, it's already async so maybe it doesn't matter.
// The `if` statement above ensures the destination buffer is large enough.dcheng@: How do you feel about this? I feel like you're the best person to sign off on this kind of change and justification.
HOw about `base::as_bytes(utf8.AsStringView())`?
`SpanWriter` might be a nice option as well here.
Related details
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 7 comments![]( "Open in Gerrit")
// The renderer is not trusted, so we don't trust it to write in bounds of the
// valid memory. Therefore, this region is created as unsafe.To be clear, "unsafe" in this context means "we cannot vend out a version of the region which allows readonly mappings"; it is not about spatial memory safety.
Yeah, I think I learned this after this comment. I'll just remove this comment since I don't think it adds anything.
Daniel ChengCan't we just create a regular memory buffer instead of shared memory?
An unsafe shmem region can only be mapped as writable. Strictly speaking, I guess we could implement a method `MapReadOnly()` that returns a `ReadOnlySharedMemoryRegion`... but it would be read-only in name only (as the underlying bytes could still be modified).
So when we refactored the API, we chose to make this explicitly visible in the type system.
This is as I expected. Given that limitation and the fact that this CL is using the API as intended, I think I'm OK to resolve this.
if (mapping.IsValid() && mapping.size() >= sizeof(uint32_t)) {I suggest using SpanReader, which has helpers for reading integers as well as larger spans.
I'll work on this now.
if (mapping.size() >= sizeof(data_size) + data_size) {Shouldn't this be strict equality check?
No because it's possible that the renderer wrote less bytes than it actually requested, not completely filling up the buffer. In that case, `data_size` can be quite low. As long as `data_size` (plus the size of `data_size` itself) is `<=` the maximum size that the renderer requested, we can proceed to read.
Is it OK that this might overwrite keys that we've already set? Perhaps that's intentional though?
Correct, the thought is that whatever the developer decides to supply here should override any other defaults. This could change in the future if we have some keys in the report that we decide should be untouchable by the web-exposed API, but no such keys exist yet. I will document this in the specification text.
length,Do we anticipate using the async bits of this for other stuff in the future?
It feels a bit hard to use TBH, though I guess if we're going through promises, it's already async so maybe it doesn't matter.
What do you mean "use the async bits in the future"? I'm just default to async IPC + Promise exposed through Web IDL because sync IPCs are so expensive, and the async-ness here gives us the ability to change the backing implementation to anything we want in the future, or make other non-web-observable optimizations at a later time, if we choose to do so. Does that answer your question?
// The `if` statement above ensures the destination buffer is large enough.Daniel Chengdcheng@: How do you feel about this? I feel like you're the best person to sign off on this kind of change and justification.
HOw about `base::as_bytes(utf8.AsStringView())`?
`SpanWriter` might be a nice option as well here.
`base::SpanWriter` worked great here! This is lovely, thanks a lot.
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
2 comments
if (mapping.IsValid() && mapping.size() >= sizeof(uint32_t)) {Dominic FarolinoI suggest using SpanReader, which has helpers for reading integers as well as larger spans.
I'll work on this now.
Done. I'm reading the bytes into a span and making a copy via std::string, and parsing the std::string as JSON now.
const uint32_t data_size = *mapping.GetMemoryAs<uint32_t>();Nasko OskovI believe this—where the browser process starts to read from shared memory—is where we discussed the possibility of needing to copy the entire buffer before JSON parsing it, since we can't really parse its bytes atomically.
But I don't think I see a path for the renderer to still be writing to the buffer (or pass a writable/unsafe end of the buffer to another still-living renderer) such that the renderer can write to the browser while the browser is reading it.
Do you think we should still copy here, or can we get away without it? I'm asking just because the max size is 5mb, which feels non-trivial to me to maintain another copy of. But maybe it's not a big deal 🤷♂️
Daniel ChengLet's make a copy. It is a lot easier to reason about, does not vary in semantics per platform (like shared memory does), and it is not a performance critical path.
Every time we try to ask "can we get away with it" ... inevitably someone comes later on to demonstrate why we can't : ). I'd rather we be defensive than optimistic.
We need to copy. The renderer can collaborate with another malicious renderer and send it a clone of the region, and the second malicious renderer could modify the bytes while we try to read here.
Copy it is! I've made a std::string copy out of the span produced from `base::SpanReader::ReadInto()`. That seemed to be the best way of doing it, but let me know if you know a better way of holding the API, dcheng@. For now I'll resolve this.
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
fabian athayde (Gerrit)
fabian athayde added 1 comment![]( "Open in Gerrit")
Message
Ok Dominic can I interest you in a punch bowl ticket
On Fri, Aug 1, 2025, 2:10 PM Dominic Farolino (Gerrit) <noreply-gerritcodereview+MGSawfHioM8TkEuRPWcghA==@> wrote: chromium.org
1 comment
Ok Dominic can I interest you in a punch bowl ticket
On Fri, Aug 1, 2025, 2:10 PM Dominic Farolino (Gerrit) <noreply-gerritcodereview+MGSawfHioM8TkEuRPWcghA==@> wrote: chromium.org
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
Also, did you have a chance to reach out to security about the shmem questions? Mostly it'd just be good to get some eyes on it that are familiar with any platform-specific quirks/limitations.
std::string body_copy(body_span.begin(), body_span.end());Add a comment here like:
```
// Take a copy to snapshot the data so that a malicious writer can't
// change the data while it's being parsed.
```
// data; see `CreateCrashReportStorage()`.Nit: explain some of the rationale behind this. I'm assuming that the actual limit isn't specified; will we have sufficient freedom to change this in the future if needed?
length,Dominic FarolinoDo we anticipate using the async bits of this for other stuff in the future?
It feels a bit hard to use TBH, though I guess if we're going through promises, it's already async so maybe it doesn't matter.
What do you mean "use the async bits in the future"? I'm just default to async IPC + Promise exposed through Web IDL because sync IPCs are so expensive, and the async-ness here gives us the ability to change the backing implementation to anything we want in the future, or make other non-web-observable optimizations at a later time, if we choose to do so. Does that answer your question?
As long as you're OK with the general shape of the API, and it fits the usual web platform patterns, I'm fine with it.
(It didn't seem like we actually needed this to be async, but if it makes it easier to extend in the future/is more consistent with other web platform APIs, I'm 100% fine with it)
Related details
- Dominic Farolino
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Also, did you have a chance to reach out to security about the shmem questions? Mostly it'd just be good to get some eyes on it that are familiar with any platform-specific quirks/limitations.
Yep, I haven't forgotten about this. I've got an email drafted and will send very soon so we can unblock this. But this CL isn't strictly necessary for the Origin Trial in M140.
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
1 comment
- Daniel Cheng
- Dominic Farolino
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Removing myself from reviewers since the files I own are no longer in the latest patchset
Related details
- Daniel Cheng
- Dominic Farolino
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
base::WritableSharedMemoryMapping shm_mapping_;This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent) \
To opt out permanently, please join [Blink AIAgent Optout](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)
Blink Style Guide: Prefer blink:: types over STL and base types. `base::WritableSharedMemoryMapping` is a `base` type used as a member variable. While necessary for shared memory interoperability, the style guide indicates that such exceptions should be explicitly annotated. Please add `ALLOW_DISCOURAGED_TYPE("For shared memory interop with the browser process")` after the member variable.
For a quick and terse communication, please consider using one of these options: \
Done | b/<bug_id> | Invalid: reason | Won't fix: reason<br/>
NOTE: AI reviews can give wrong results. \
We apologize in advance for any inconvenience and would love to take your feedback to make this better. \
[file a bug](http://go/blink-c++-code-review-agent-feedback) or [ping us on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4)
auto json_object = std::make_unique<JSONObject>();This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent) \
To opt out permanently, please join [Blink AIAgent Optout](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)
Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. `JSONObject` is a `WTF::RefCounted` object that provides a `JSONObject::Create()` factory method. Please use `auto json_object = JSONObject::Create();` instead of `std::make_unique` to follow the idiomatic creation pattern for this Blink platform object.
For a quick and terse communication, please consider using one of these options: \
Done | b/<bug_id> | Invalid: reason | Won't fix: reason<br/>
NOTE: AI reviews can give wrong results. \
We apologize in advance for any inconvenience and would love to take your feedback to make this better. \
[file a bug](http://go/blink-c++-code-review-agent-feedback) or [ping us on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4)
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 6 comments![]( "Open in Gerrit")
Have you discussed OS-specific limits around shmem? How was the limit of 5MB determined? Are there any platform-specific limits we need to be concerned about? Can a renderer easily use up all allocatable shmem regions on a machine just with JS calls, whereas maybe it was harder before?
I'm hoping the security review will help flesh some of this out, specifically around the OS-specific and platform-specific limits. I don't know enough about what to be concerned with there, outside of what you mentioned regarding shared memory being a scarce resource on some platforms. I suspect we may end up with some limits around how many times this API can be called within a tab, or how many bytes can be allocated in a tab that has platform-specific constraints or so, but I don't know enough about the platform limitations myself, so I'd like to hear more from security reviewers.
Dominic FarolinoAlso, did you have a chance to reach out to security about the shmem questions? Mostly it'd just be good to get some eyes on it that are familiar with any platform-specific quirks/limitations.
Yep, I haven't forgotten about this. I've got an email drafted and will send very soon so we can unblock this. But this CL isn't strictly necessary for the Origin Trial in M140.
Resolving this, since it's currently under the extra security review in http://g/chrome-security-reviews/c/ug0-uatcaFE
std::string body_copy(body_span.begin(), body_span.end());Add a comment here like:
```
// Take a copy to snapshot the data so that a malicious writer can't
// change the data while it's being parsed.
```
Done
Nit: explain some of the rationale behind this. I'm assuming that the actual limit isn't specified; will we have sufficient freedom to change this in the future if needed?
We've talked about this offline, but I've added comments describing what we discussed, and mentioning the fact that this value can grow in the future.
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent) \
To opt out permanently, please join [Blink AIAgent Optout](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)Blink Style Guide: Prefer blink:: types over STL and base types. `base::WritableSharedMemoryMapping` is a `base` type used as a member variable. While necessary for shared memory interoperability, the style guide indicates that such exceptions should be explicitly annotated. Please add `ALLOW_DISCOURAGED_TYPE("For shared memory interop with the browser process")` after the member variable.
For a quick and terse communication, please consider using one of these options: \
Done | b/<bug_id> | Invalid: reason | Won't fix: reason<br/>NOTE: AI reviews can give wrong results. \
We apologize in advance for any inconvenience and would love to take your feedback to make this better. \
[file a bug](http://go/blink-c++-code-review-agent-feedback) or [ping us on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4)
Invalid: does not make sense.
auto json_object = std::make_unique<JSONObject>();This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent) \
To opt out permanently, please join [Blink AIAgent Optout](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. `JSONObject` is a `WTF::RefCounted` object that provides a `JSONObject::Create()` factory method. Please use `auto json_object = JSONObject::Create();` instead of `std::make_unique` to follow the idiomatic creation pattern for this Blink platform object.
For a quick and terse communication, please consider using one of these options: \
Done | b/<bug_id> | Invalid: reason | Won't fix: reason<br/>NOTE: AI reviews can give wrong results. \
We apologize in advance for any inconvenience and would love to take your feedback to make this better. \
[file a bug](http://go/blink-c++-code-review-agent-feedback) or [ping us on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4)
Invalid: Doesn't seem relevant since this is not how the object is created throughout the code base, from what I can tell from a cursory search.
Related details
- Daniel Cheng
- Nasko Oskov
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Related details
- Daniel Cheng
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
reader.ReadU32LittleEndian(data_size);Not sure whether LittleEndian or NativeEndian is the right thing here, will defer to dcheng@ on that one. It seems to me that NativeEndian is better choice, as we never persist this anywhere, but the warning on the API header is worth another opinion.
// parsed.The malicious renderer still has opportunity for changing the data between the read of the size int and reading the data. I don't know whether a clever attack can be performed by manipulating the actual data once we have read its size ... but the alternatives are likely more complicated to implement. So mostly sharing my thoughts, not sure if there is an action item at this point.
Related details
- Daniel Cheng
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 2 comments![]( "Open in Gerrit")
Not sure whether LittleEndian or NativeEndian is the right thing here, will defer to dcheng@ on that one. It seems to me that NativeEndian is better choice, as we never persist this anywhere, but the warning on the API header is worth another opinion.
Yep, I think you're right, this at least seems the most general option, and I don't think we need to make a non-native decision one way or another, so let's defer to the platform. Fixed this here, and the "write" counterpart in the renderer.
The malicious renderer still has opportunity for changing the data between the read of the size int and reading the data. I don't know whether a clever attack can be performed by manipulating the actual data once we have read its size ... but the alternatives are likely more complicated to implement. So mostly sharing my thoughts, not sure if there is an action item at this point.
Is the malicious renderer in this case, one that the crashed process handed the handle of writable memory to? So there are three processes:
- Browser process
- R1: Malicious renderer that crashes
- R2: Malicious renderer that colludes with the crashing process
The scenario you envision is:
1. R1 initializes crash reporting storage
1. R1 hands off its writable shared memory handle to R2, a different renderer
1. R1 crashes
1. Browser process reads the size int from the shared memory
1. R2 mutates the contents of the shared memory
1. Browser process reads the remaining body contents of the shared memory, which has been mutated since it last read the first "size" int parts of the memory
Just to be clear, this is the scenario you had in mind, right?
I agree, I don't think there's any action that needs to be taken right now, but I've captured the concern in documentation here. So I'll resolve this.
Related details
- Daniel Cheng
- Nasko Oskov
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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Clearing the attention set since I still need to resolve some of the questions on the internal security review thread.
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
Dominic FarolinoHave you discussed OS-specific limits around shmem? How was the limit of 5MB determined? Are there any platform-specific limits we need to be concerned about? Can a renderer easily use up all allocatable shmem regions on a machine just with JS calls, whereas maybe it was harder before?
I'm hoping the security review will help flesh some of this out, specifically around the OS-specific and platform-specific limits. I don't know enough about what to be concerned with there, outside of what you mentioned regarding shared memory being a scarce resource on some platforms. I suspect we may end up with some limits around how many times this API can be called within a tab, or how many bytes can be allocated in a tab that has platform-specific constraints or so, but I don't know enough about the platform limitations myself, so I'd like to hear more from security reviewers.
I'll resolve this since we're discussing the security implications of this CL in https://groups.google.com/a/google.com/g/chrome-security-reviews/c/ug0-uatcaFE
nasko@ PTAL at //content/browser
dcheng@ PTAL at the parsing/serialization code in render_frame_host_impl.cc, as well as the renderer-side changes
Note that this feature is still under security review, but I'm hopeful we can land this CL now, and implement any restrictions that we decide on in https://groups.google.com/a/google.com/g/chrome-security-reviews/c/ug0-uatcaFE in a separate CL. We of course cannot ship the API until security signs off, so I think it should be OK to land this CL while this is under a flag.
Related details
- Daniel Cheng
- Nasko Oskov
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
1 comment
dcheng@: I've updated the shared memory size limit after Friday's resolution. PTAL!
Related details
- Daniel Cheng
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 7 comments![]( "Open in Gerrit")
base::UnsafeSharedMemoryRegion crash_report_storage_region_;Do we need this here too?
if (length > blink::mojom::kMaxCrashReportStorageSize) {I would like to adjust this limit before we land this.
if (mapping.size() >= sizeof(data_size) + data_size) {Should we report bad message and early return on general principle here?
base::span<const uint8_t> body_span;
// Take a copy of the entire crash report API memory, to snapshot the
// data so that a malicious writer can't change it while it's being
// parsed. Note that technically malicious writer could technically
// change the data that we're about to snapshot, after we've read
// `data_size`, but changing the contents or format of this data after
// we snapshot the body below shouldn't matter; the worst that can
// happen is the data is no longer valid JSON, and does not get added to
// the report.
DCHECK(reader.ReadInto(data_size, body_span));
std::string body_copy(body_span.begin(), body_span.end());Would it make sense to use `SpanReader::ReadCopy()` here?
body.Set("crash_report_api", std::move(crash_report_api_body));Or is it important to still do these steps even if the preconditions above are violated?
base::CheckedNumeric<size_t> total_size = utf8.size();
total_size += sizeof(uint32_t);
if (!base::IsValueInRangeForNumericType<uint32_t>(utf8.size()) ||
total_size.ValueOrDie() > shm_mapping_.size()) {```suggestion
if (base::CheckedAdd(utf8.size(), sizeof(uint32_t)).IsInvalidOr(
[this] (uint32_t val) { return val > shm_mapping_.size(); }) {
```
Related details
- Dominic Farolino
- Nasko Oskov
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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 6 comments![]( "Open in Gerrit")
base::UnsafeSharedMemoryRegion crash_report_storage_region_;Do we need this here too?
Nope, I missed this while rebasing, sorry. Removed.
if (length > blink::mojom::kMaxCrashReportStorageSize) {I would like to adjust this limit before we land this.
It has been adjusted already in `frame.mojom` in this PS, to `65536` (i.e., 64KiB as we discussed). So I think we're OK to resolve this? (Sorry, I'm assuming that you missed the change, but if *I'm* actually missing something, lmk.
if (mapping.size() >= sizeof(data_size) + data_size) {Should we report bad message and early return on general principle here?
We don't want to early-return. Just because a compromised renderer abused the `window.crashReport` API, I don't think the origin should be deprived of the other trusted data that the browser is equipped to supply in the crash report. As for bad messaging... I'm not sure it's useful if the renderer process has already crashed—does bad messaging do anything else useful to us, besides crashing? (Maybe some statistics are reported internally?).
I'm happy leaving this alone here, but lmk if you'd liek something different.
base::span<const uint8_t> body_span;
// Take a copy of the entire crash report API memory, to snapshot the
// data so that a malicious writer can't change it while it's being
// parsed. Note that technically malicious writer could technically
// change the data that we're about to snapshot, after we've read
// `data_size`, but changing the contents or format of this data after
// we snapshot the body below shouldn't matter; the worst that can
// happen is the data is no longer valid JSON, and does not get added to
// the report.
DCHECK(reader.ReadInto(data_size, body_span));
std::string body_copy(body_span.begin(), body_span.end());Would it make sense to use `SpanReader::ReadCopy()` here?
Can you clarify what you envision here? I tried the following:
```
std::string body_copy(data_size, '\0');
base::span<const uint8_t> body_span = base::byte_span_from_cstring(body_copy.c_str());
DCHECK(reader.ReadCopy(body_span));
```
In order to get a byte span from the string with appropriate size, so we could read the copy into *that* byte span, but that didn't work. The compiler didn't like it:
```
../../content/browser/renderer_host/render_frame_host_impl.cc:16366:47: error: no matching function for call to 'byte_span_from_cstring'
16366 | base::span<const uint8_t> body_span = base::byte_span_from_cstring(body_copy.c_str());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/containers/span.h:1651:16: note: candidate template ignored: could not match 'CharT[Extent]' against 'const value_type *' (aka 'const char *')
1651 | constexpr auto byte_span_from_cstring(const CharT (&str LIFETIME_BOUND)[Extent])
| ^
../../content/browser/renderer_host/render_frame_host_impl.cc:16367:32: error: no viable conversion from 'span<const uint8_t>' to 'span<std::remove_const_t<const unsigned char>>'
16367 | DCHECK(reader.ReadCopy(body_span));
```
Do you have a different way in mind, of getting the byte span to read the copy into? Or am I just going about this wrong.
body.Set("crash_report_api", std::move(crash_report_api_body));Or is it important to still do these steps even if the preconditions above are violated?
Correct, as discussed above (so I'll resolve this).
#include "base/bits.h"Dominic FarolinoIs this needed?
No, removed.
Related details
- Daniel Cheng
- Nasko Oskov
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. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
// Read the key/value data from the `crash_report_storage_region_` sharedThis should be updated to be the document associated data, not the private member of RFH, based on the code below.
// parsed. Note that technically malicious writer could technicallynit: used technically twice technically : )
Related details
- Daniel Cheng
- 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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 5 comments![]( "Open in Gerrit")
base::WritableSharedMemoryMapping mapping =I would suggest pulling this entire block out into a helper function.
if (mapping.IsValid() && mapping.size() >= sizeof(uint32_t)) {Because then we can just early return when things are invalid. And we don't need the sizeof check
reader.ReadU32NativeEndian(data_size);Because we can just use the return value here to determine if the span was big enough.
if (mapping.size() >= sizeof(data_size) + data_size) {Dominic FarolinoShould we report bad message and early return on general principle here?
We don't want to early-return. Just because a compromised renderer abused the `window.crashReport` API, I don't think the origin should be deprived of the other trusted data that the browser is equipped to supply in the crash report. As for bad messaging... I'm not sure it's useful if the renderer process has already crashed—does bad messaging do anything else useful to us, besides crashing? (Maybe some statistics are reported internally?).
I'm happy leaving this alone here, but lmk if you'd liek something different.
That's fine. But I think we should pull this logic out to make it less deeply-nested/a bit easier to read.
base::span<const uint8_t> body_span;
// Take a copy of the entire crash report API memory, to snapshot the
// data so that a malicious writer can't change it while it's being
// parsed. Note that technically malicious writer could technically
// change the data that we're about to snapshot, after we've read
// `data_size`, but changing the contents or format of this data after
// we snapshot the body below shouldn't matter; the worst that can
// happen is the data is no longer valid JSON, and does not get added to
// the report.
DCHECK(reader.ReadInto(data_size, body_span));
std::string body_copy(body_span.begin(), body_span.end());Dominic FarolinoWould it make sense to use `SpanReader::ReadCopy()` here?
Can you clarify what you envision here? I tried the following:
```
std::string body_copy(data_size, '\0');
base::span<const uint8_t> body_span = base::byte_span_from_cstring(body_copy.c_str());
DCHECK(reader.ReadCopy(body_span));
```In order to get a byte span from the string with appropriate size, so we could read the copy into *that* byte span, but that didn't work. The compiler didn't like it:
```
../../content/browser/renderer_host/render_frame_host_impl.cc:16366:47: error: no matching function for call to 'byte_span_from_cstring'
16366 | base::span<const uint8_t> body_span = base::byte_span_from_cstring(body_copy.c_str());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/containers/span.h:1651:16: note: candidate template ignored: could not match 'CharT[Extent]' against 'const value_type *' (aka 'const char *')
1651 | constexpr auto byte_span_from_cstring(const CharT (&str LIFETIME_BOUND)[Extent])
| ^
../../content/browser/renderer_host/render_frame_host_impl.cc:16367:32: error: no viable conversion from 'span<const uint8_t>' to 'span<std::remove_const_t<const unsigned char>>'
16367 | DCHECK(reader.ReadCopy(body_span));
```Do you have a different way in mind, of getting the byte span to read the copy into? Or am I just going about this wrong.
I think there are several possible approaches here. The simplest is probably:
```
std::string body_copy(data_size, '\0');
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 8 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
8 comments
I think this is ready to go! PTAL!
// Read the key/value data from the `crash_report_storage_region_` sharedThis should be updated to be the document associated data, not the private member of RFH, based on the code below.
Done
base::WritableSharedMemoryMapping mapping =I would suggest pulling this entire block out into a helper function.
Done
if (mapping.IsValid() && mapping.size() >= sizeof(uint32_t)) {Because then we can just early return when things are invalid. And we don't need the sizeof check
Done
reader.ReadU32NativeEndian(data_size);Because we can just use the return value here to determine if the span was big enough.
Done
// parsed. Note that technically malicious writer could technicallynit: used technically twice technically : )
Eek, nice catch. Done!
base::span<const uint8_t> body_span;
// Take a copy of the entire crash report API memory, to snapshot the
// data so that a malicious writer can't change it while it's being
// parsed. Note that technically malicious writer could technically
// change the data that we're about to snapshot, after we've read
// `data_size`, but changing the contents or format of this data after
// we snapshot the body below shouldn't matter; the worst that can
// happen is the data is no longer valid JSON, and does not get added to
// the report.
DCHECK(reader.ReadInto(data_size, body_span));
std::string body_copy(body_span.begin(), body_span.end());Dominic FarolinoWould it make sense to use `SpanReader::ReadCopy()` here?
Daniel ChengCan you clarify what you envision here? I tried the following:
```
std::string body_copy(data_size, '\0');
base::span<const uint8_t> body_span = base::byte_span_from_cstring(body_copy.c_str());
DCHECK(reader.ReadCopy(body_span));
```In order to get a byte span from the string with appropriate size, so we could read the copy into *that* byte span, but that didn't work. The compiler didn't like it:
```
../../content/browser/renderer_host/render_frame_host_impl.cc:16366:47: error: no matching function for call to 'byte_span_from_cstring'
16366 | base::span<const uint8_t> body_span = base::byte_span_from_cstring(body_copy.c_str());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/containers/span.h:1651:16: note: candidate template ignored: could not match 'CharT[Extent]' against 'const value_type *' (aka 'const char *')
1651 | constexpr auto byte_span_from_cstring(const CharT (&str LIFETIME_BOUND)[Extent])
| ^
../../content/browser/renderer_host/render_frame_host_impl.cc:16367:32: error: no viable conversion from 'span<const uint8_t>' to 'span<std::remove_const_t<const unsigned char>>'
16367 | DCHECK(reader.ReadCopy(body_span));
```Do you have a different way in mind, of getting the byte span to read the copy into? Or am I just going about this wrong.
I think there are several possible approaches here. The simplest is probably:
```
std::string body_copy(data_size, '\0');
DCHECK(reader.ReadCopy(base::as_bytes(base::span(body_copy))));
```
I needed `as_writable_bytes`, but looks like that did the trick. So close. Thanks.
base::CheckedNumeric<size_t> total_size = utf8.size();
total_size += sizeof(uint32_t);
if (!base::IsValueInRangeForNumericType<uint32_t>(utf8.size()) ||
total_size.ValueOrDie() > shm_mapping_.size()) {```suggestion
if (base::CheckedAdd(utf8.size(), sizeof(uint32_t)).IsInvalidOr(
[this] (uint32_t val) { return val > shm_mapping_.size(); }) {
```
- Daniel Cheng
- Nasko Oskov
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +0 |
| Commit-Queue | +1 |
1 comment
(oops)
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
LGTM
void ReadCrashReportAPIBody(base::Value::Dict&);Nit: return a `base::Value::Dict` here rather than using an out param
Related details
- Dominic Farolino
- Nasko Oskov
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
2 comments
Adding mmenke@ for reporting_browsertest.cc
Nit: return a `base::Value::Dict` here rather than using an out param
- Nasko Oskov
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke voted and added 3 comments![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
3 comments
LGTM. Comments are optional.
// page. In this case, just a custom key that gets written to twice, to verify
// that the most recent write gets recorded and comes out on the other end of
// the crash report.Not a sentence - needs a verb (pre-existing issue). `use it to write a custom key twice, to...` maybe?
// Verify the contents of the report that we received.
const base::Value::Dict& report = request.begin()->GetDict();
const std::string* type = report.FindString("type");
const std::string* url = report.FindString("url");
const base::Value::Dict* body = report.FindDict("body");
const std::string* reason = body->FindString("reason");
const base::Value::Dict* crash_report_api_body =
body->FindDict("crash_report_api");
const std::string* custom_key =
crash_report_api_body->FindString("custom_key");optional: Suggest using values_test_util.
```
EXPECT_THAT(report, base::test::IsJson(<multi-line-json-string>));
(IsJson supports std::strings in addition to Values/Lists/Dicts)
```
If you don't want to do an exact match, there's also IsSupersetOfValue(). I believe its superset logic is recursive.
Given that we hard-code the length of the JSON object, though, I think exact match is fine, since we'd need to update the test anyways if that changes?
Related details
- Dominic Farolino
- Nasko Oskov
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. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
// Read the key/value data from the document associated data' shared memorynit: Unnecessary?
DCHECK(reader.ReadCopy(base::as_writable_bytes(base::span(body_copy))));If DCHECKs are disabled, wouldn't this result in the `ReadCopy` call being skipped entirely? It will then operate on empty body and not attach anything to the actual report.
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
3 comments
PTAL! (will look at mmenke@'s review later, since it won't impact the CR statuses).
// Read the key/value data from the document associated data' shared memoryDominic Farolinonit: Unnecessary?
Yep. Removed.
DCHECK(reader.ReadCopy(base::as_writable_bytes(base::span(body_copy))));If DCHECKs are disabled, wouldn't this result in the `ReadCopy` call being skipped entirely? It will then operate on empty body and not attach anything to the actual report.
Yep (and we talked about this offline). Good catch, done.
Related details
- Nasko Oskov
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. |
Nasko Oskov (Gerrit)
Nasko Oskov voted Code-Review+1![]( "Open in Gerrit")
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +2 |
2 comments
// page. In this case, just a custom key that gets written to twice, to verify
// that the most recent write gets recorded and comes out on the other end of
// the crash report.Not a sentence - needs a verb (pre-existing issue). `use it to write a custom key twice, to...` maybe?
Fixed this differently, more more to what I was trying to achieve in the first place. Thanks for catching.
// Verify the contents of the report that we received.
const base::Value::Dict& report = request.begin()->GetDict();
const std::string* type = report.FindString("type");
const std::string* url = report.FindString("url");
const base::Value::Dict* body = report.FindDict("body");
const std::string* reason = body->FindString("reason");
const base::Value::Dict* crash_report_api_body =
body->FindDict("crash_report_api");
const std::string* custom_key =
crash_report_api_body->FindString("custom_key");optional: Suggest using values_test_util.
```
EXPECT_THAT(report, base::test::IsJson(<multi-line-json-string>));
(IsJson supports std::strings in addition to Values/Lists/Dicts)
```If you don't want to do an exact match, there's also IsSupersetOfValue(). I believe its superset logic is recursive.
Given that we hard-code the length of the JSON object, though, I think exact match is fine, since we'd need to update the test anyways if that changes?
I think that's reasonable, but yeah since everything is hardcoded and this matches the other tests, I will opt to leave it be.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
33 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/net/reporting_browsertest.cc
Insertions: 3, Deletions: 3.
@@ -800,9 +800,9 @@
EXPECT_TRUE(NavigateToURL(contents, main_url));
// Use the crash reporting storage API, to collect some data about the current
- // page. In this case, just a custom key that gets written to twice, to verify
- // that the most recent write gets recorded and comes out on the other end of
- // the crash report.
+ // page. In this case, a custom key gets written to twice, to verify that the
+ // most recent write gets recorded and comes out on the other end of the crash
+ // report.
EXPECT_TRUE(content::EvalJs(contents->GetPrimaryMainFrame(), R"(
(async () => {
await crashReport.initialize(100);
```
Change information
CrashReportStorage API uses shared memory instead of sync IPCs
This CL refactors the CrashReportStorage API to remove the synchronous
set()/remove() IPCs in favor of shared memory that is directly
written to by the renderer. The crash report shared memory is read from
the browser process after a renderer crash, in
RenderFrameHostImpl::MaybeGenerateCrashReport(). The motivation for this
change is that the sync IPCs were found to be prohibitively expensive
for sites to deploy this API without putting an excessive amount of
thought into the paths that this this API was being used on. This cuts
against the intent of the API, which is meant to be a drop-in crash
debugging assist tool. See
https://github.com/WICG/crash-reporting/blob/gh-pages/crashReport-explainer.md.
This CL makes the CrashReportStorage#initialize() Web IDL method send
its requested size, in bytes, to the browser process, which vends a
writable shared memory buffer handle to the renderer.
Note that the shared memory buffer that the browser vends is exactly N
bytes in size, where N is:
- The developer's requested length in bytes (enforced to be less than
`kMaxCrashReportStorageSize`); plus
- `sizeof(unt32_t)`
This is so that the renderer can write a leading integer at the front
of the buffer, recording the number of bytes used in the buffer after
every set()/remove() call. The browser reads this leading integer when
parsing and serializing the bytes into JSON, to be attached to the
web-exposed crash report.
R=dcheng, nasko
- M chrome/browser/net/reporting_browsertest.cc
- M content/browser/renderer_host/document_associated_data.cc
- M content/browser/renderer_host/document_associated_data.h
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/renderer_host/render_frame_host_impl.h
- M third_party/blink/public/mojom/frame/frame.mojom
- M third_party/blink/renderer/core/frame/DEPS
- M third_party/blink/renderer/core/frame/crash_report_storage.cc
- M third_party/blink/renderer/core/frame/crash_report_storage.h
- M third_party/blink/renderer/core/testing/fake_local_frame_host.cc
- M third_party/blink/renderer/core/testing/fake_local_frame_host.h
Code-Review: +1 by mmenke, +1 by Nasko Oskov, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |