Multicast support in Direct Sockets #3 [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
ExceptionState& exception_state);nit: The name for the `exception_state` parameter is obvious from its type and can be omitted from the function declaration in the header file. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
***
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **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
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
ExceptionState& exception_state);nit: The name for the `exception_state` parameter is obvious from its type and can be omitted from the function declaration in the header file. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
***_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **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)_
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 8 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(DirectSocketsBoundUdpBrowserTest, JoinGroup) {Can you also add tests to `chrome/browser/direct_sockets/direct_sockets_apitest.cc` to make sure stuff really works with IWAs?
std::move(callback).Run(net::ERR_ACCESS_DENIED);IIUC there's no need to invoke a callback after ReportBadMessage.
I'd rather call this `has_multicast_permission_` and structure this as
```
if (!has_multicast_permission_) {
// Calling this method without the permission means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("...");
return;
}
udp_socket_->JoinGroup(...)
```
Member<UDPSocketMojoRemote> udp_socket_;nit: `const Member`
BindOnce(&MulticastController::OnJoinedGroup, WrapPersistent(this),Remove the base/ include -- it's misleading!
*parsed_ip_opt,maybe restructure the promise hashsets to be a map<ip, promise>? This way it'd be possible to catch repeated calls early.
OnJoinLeftGroupInternal(resolver, net_error, join_group_promises_);Frankly, this function adds more code than value (also note that `Contains()` exists!). How about
```
join_group_promises_.erase(resolver);
if (net_error == net::OK) {
if (!joined_groups_.Contains(normalized_ip_address)) {
joined_groups_.push_back(std::move(normalized_ip_address));
}
resolver->Resolve();
return;
}
resolver->Reject(...);
```
ditto below.
if (index != kNotFound) {`Erase(joined_groups_, normalized_ip_address)` should just [work](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/vector.h;drc=c31ed6ad77ee0d2e8b21ec551264ba80b2e96a0c;l=2601).
Please fix this WARNING reported by Check Contents: Please remove the trailing whitespace.
To rerun the analyzer locally, run: `ali...
Please remove the trailing whitespace.
To rerun the analyzer locally, run: `alint -- -c CheckContents`
For more information, see go/alint.
Related details
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 8 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(DirectSocketsBoundUdpBrowserTest, JoinGroup) {Can you also add tests to `chrome/browser/direct_sockets/direct_sockets_apitest.cc` to make sure stuff really works with IWAs?
Done
std::move(callback).Run(net::ERR_ACCESS_DENIED);IIUC there's no need to invoke a callback after ReportBadMessage.
I'd rather call this `has_multicast_permission_` and structure this as
```
if (!has_multicast_permission_) {
// Calling this method without the permission means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("...");
return;
}udp_socket_->JoinGroup(...)
```
Done
Member<UDPSocketMojoRemote> udp_socket_;Vlad Krotnit: `const Member`
Done
BindOnce(&MulticastController::OnJoinedGroup, WrapPersistent(this),Remove the base/ include -- it's misleading!
Done
*parsed_ip_opt,maybe restructure the promise hashsets to be a map<ip, promise>? This way it'd be possible to catch repeated calls early.
Good idea.
OnJoinLeftGroupInternal(resolver, net_error, join_group_promises_);Frankly, this function adds more code than value (also note that `Contains()` exists!). How about
```
join_group_promises_.erase(resolver);
if (net_error == net::OK) {
if (!joined_groups_.Contains(normalized_ip_address)) {
joined_groups_.push_back(std::move(normalized_ip_address));
}
resolver->Resolve();
return;
}
resolver->Reject(...);
```ditto below.
Done
if (index != kNotFound) {`Erase(joined_groups_, normalized_ip_address)` should just [work](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/vector.h;drc=c31ed6ad77ee0d2e8b21ec551264ba80b2e96a0c;l=2601).
Done
Please fix this WARNING reported by Check Contents: Please remove the trailing whitespace.
To rerun the analyzer locally, run: `ali...
Please remove the trailing whitespace.
To rerun the analyzer locally, run: `alint -- -c CheckContents`
For more information, see go/alint.
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Commit-Queue+1![]( "Open in Gerrit")
- Andrew Rayskiy
- Chromium IPC Reviews
- Maks Orlovich
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
| 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: kin...@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): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Andrew Rayskiy
- Kinuko Yasuda
- Maks Orlovich
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
1 comment
content::JsReplace(script, kBindAllAddress, kMulticastAddress)),`net::IPAddress::IPv4AllZeros()`
Related details
- Kinuko Yasuda
- Maks Orlovich
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vlad Krot
| Commit-Queue | +1 |
1 comment
content::JsReplace(script, kBindAllAddress, kMulticastAddress)),Vlad Krot`net::IPAddress::IPv4AllZeros()`
Done
Related details
- Kinuko Yasuda
- Maks Orlovich
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
// The original network.mojom.UDPSocket allows the caller to perform a second
// round of Bind() or Connect() calls after Close() which is highly undesirable
// in an untrusted process like the renderer. This restricted version implementsAre there any similar concerns or restrictions on the multicast join/leave groups?
Related details
- Kinuko Yasuda
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
// The original network.mojom.UDPSocket allows the caller to perform a second
// round of Bind() or Connect() calls after Close() which is highly undesirable
// in an untrusted process like the renderer. This restricted version implementsAre there any similar concerns or restrictions on the multicast join/leave groups?
This is only a concern for Bind(), Connect() after Close(). joinMulticast() won't work on closed socket, and because it is a RestrictedUDPSocket, opening it again won't work.
Related details
- Kinuko Yasuda
- Maks Orlovich
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |