Multicast support in Direct Sockets #1 [chromium/src : main]
Vlad Krot (Gerrit)
Vlad Krot voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vlad Krot
| Commit-Queue | +1 |
1 comment
Please review:
active_script_wrappable_creation_key.h, DEPS
Related details
- Andrew Rayskiy
- Andrey Kosyakov
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: cl...@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): cl...@chromium.org
Reviewer source(s):
cl...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Andrew Rayskiy
- Andrey Kosyakov
- Camille Lamy
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 5 comments![]( "Open in Gerrit")
// TODO(@vkrot) check the necessary permission policyThe standard TODO format is (here and elsewhere):
`// TODO(crbug.com/<...>): ...`
Window MulticastInDirectSockets,Since the flag is the same, you can simply do
```
RuntimeEnabled=MulticastInDirectSockets,
Exposed=(Window, DedicatedWorker)
```
MulticastController multicastController;hide this behind runtime enabled?
boolean multicastAllowAddressSharing;Hide these behind runtime enabled
WeakMember<MulticastController> weak_multicast_controller_;I'd make this a normal `Member`; this way you won't need to mark the controller as ActiveWrappable (I presume).
You might also be able to get this from `openInfo`?
Related details
- Andrey Kosyakov
- Camille Lamy
- 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 5 comments![]( "Open in Gerrit")
// TODO(@vkrot) check the necessary permission policyThe standard TODO format is (here and elsewhere):
`// TODO(crbug.com/<...>): ...`
Done
Since the flag is the same, you can simply do
```
RuntimeEnabled=MulticastInDirectSockets,
Exposed=(Window, DedicatedWorker)
```
Done
hide this behind runtime enabled?
Yes, good idea
Hide these behind runtime enabled
Done
WeakMember<MulticastController> weak_multicast_controller_;I'd make this a normal `Member`; this way you won't need to mark the controller as ActiveWrappable (I presume).
You might also be able to get this from `openInfo`?
ActiveScriptWrappable is necessary to prevent collection of this object while joining/leaving groups is in process, to fire callback for the user when operations ended, because what if socket and multicast controller are both collected.
Technically, nothing bad will happen, because the socket resources will be released, but the user won't receive callback.
Related details
- Andrew Rayskiy
- Andrey Kosyakov
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
WeakMember<MulticastController> weak_multicast_controller_;Vlad KrotI'd make this a normal `Member`; this way you won't need to mark the controller as ActiveWrappable (I presume).
You might also be able to get this from `openInfo`?
ActiveScriptWrappable is necessary to prevent collection of this object while joining/leaving groups is in process, to fire callback for the user when operations ended, because what if socket and multicast controller are both collected.
Technically, nothing bad will happen, because the socket resources will be released, but the user won't receive callback.
I'd argue that if the socket object is already dead, then the multicast pipe will most likely also be dead, nah?
Related details
- Andrey Kosyakov
- Camille Lamy
- Vlad Krot
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
WeakMember<MulticastController> weak_multicast_controller_;Vlad KrotI'd make this a normal `Member`; this way you won't need to mark the controller as ActiveWrappable (I presume).
You might also be able to get this from `openInfo`?
Andrew RayskiyActiveScriptWrappable is necessary to prevent collection of this object while joining/leaving groups is in process, to fire callback for the user when operations ended, because what if socket and multicast controller are both collected.
Technically, nothing bad will happen, because the socket resources will be released, but the user won't receive callback.
I'd argue that if the socket object is already dead, then the multicast pipe will most likely also be dead, nah?
- Andrew Rayskiy
- Andrey Kosyakov
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 4 comments![]( "Open in Gerrit")
if (options->multicast_allow_address_sharing.has_value()) {Sanity check: does it make sense for any of these options to be present while the others aren't? If not, would it make sense to package all the multicast options as a single struct to have this struct be the optional memebers in the mojom struct, instead of having a list of individual optionals?
// Default is false.nit: this seems a bit weird for an optional value. Do you mean something like by default this is not present unless the user of the API wants to use multicast. In this case we will have a value, and this value will be false, unless the user of the API uses special configuration option. If so, maybe update the comment.
Similar nit for multicast_loopback below.
// When the object dies, it does not leave groups, because the socket closureCould you precise which object you are referring to? It's not clear whether it is the MulticastController, or the object that requested to join groups. It would also be nice to reference the exact name of the class and function you mean by "the socket closure will do it".
Also, the comment seems to suggest that the MulticastController is only kept alive by having pending join/leave requests, and when it doesn't have them anymore it is destroyed. Except that it is a member of UDPSocket, so its lifetime should be the UDPSocket lifetime instead (which makes more sense). Could you update the comment so that it is clearer?
Related details
- Andrew Rayskiy
- Andrey Kosyakov
- 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 3 comments![]( "Open in Gerrit")
if (options->multicast_allow_address_sharing.has_value()) {Sanity check: does it make sense for any of these options to be present while the others aren't? If not, would it make sense to package all the multicast options as a single struct to have this struct be the optional memebers in the mojom struct, instead of having a list of individual optionals?
does it make sense for any of these options to be present while the others aren't?
- Yes, makes sense. Every option can be present independently. For example, multicast_time_to_live is used for sending datagrams.
- allow_address_sharing_for_multicast - to be able to bind to the same port multiple times. They are independent.
nit: this seems a bit weird for an optional value. Do you mean something like by default this is not present unless the user of the API wants to use multicast. In this case we will have a value, and this value will be false, unless the user of the API uses special configuration option. If so, maybe update the comment.
Similar nit for multicast_loopback below.
changed wording
// When the object dies, it does not leave groups, because the socket closureCould you precise which object you are referring to? It's not clear whether it is the MulticastController, or the object that requested to join groups. It would also be nice to reference the exact name of the class and function you mean by "the socket closure will do it".
Also, the comment seems to suggest that the MulticastController is only kept alive by having pending join/leave requests, and when it doesn't have them anymore it is destroyed. Except that it is a member of UDPSocket, so its lifetime should be the UDPSocket lifetime instead (which makes more sense). Could you update the comment so that it is clearer?
Thanks for the observation. I have changed lifetime of MulticastController, but forgot to update the comment.
Related details
- Andrew Rayskiy
- Andrey Kosyakov
- Camille Lamy
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")
if (options->multicast_allow_address_sharing.has_value()) {Vlad KrotSanity check: does it make sense for any of these options to be present while the others aren't? If not, would it make sense to package all the multicast options as a single struct to have this struct be the optional memebers in the mojom struct, instead of having a list of individual optionals?
does it make sense for any of these options to be present while the others aren't?
- Yes, makes sense. Every option can be present independently. For example, multicast_time_to_live is used for sending datagrams.
- allow_address_sharing_for_multicast - to be able to bind to the same port multiple times. They are independent.
Acknowledged
Related details
- Andrew Rayskiy
- Andrey Kosyakov
- Camille Lamy
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 3 comments![]( "Open in Gerrit")
std::optional<net::IPAddress> CreateAndCheckIpAddress(I'd move this to a `.cc` anon namespace
void OnJoinedGroup(ScriptPromiseResolver<IDLUndefined>* resolver,These three methods do not make sense without a proper impl imo. Shall we move them to a follow-up?
Same for promises
enum class State { kOpen, kClosed, kAborted };Do we really care about the aborted/closed states?
Related details
- Andrey Kosyakov
- Camille Lamy
- 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 3 comments![]( "Open in Gerrit")
std::optional<net::IPAddress> CreateAndCheckIpAddress(I'd move this to a `.cc` anon namespace
Good idea!
void OnJoinedGroup(ScriptPromiseResolver<IDLUndefined>* resolver,These three methods do not make sense without a proper impl imo. Shall we move them to a follow-up?
Same for promises
Yea, we can do that.
Do we really care about the aborted/closed states?
- Andrew Rayskiy
- Andrey Kosyakov
- Camille Lamy
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 Code-Review+1![]( "Open in Gerrit")
- Andrey Kosyakov
- Camille Lamy
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
1 comment
Related details
- Andrey Kosyakov
- Vlad Krot
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+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Andrey Kosyakov
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Multicast support in Direct Sockets #1
This CL adds IDL files for Multicast and necessary parameters to
UDPSocketOptions. Not everything is in place yet. cover: 1. Permission
Policy 2. Communication with real socket via mojo 3. Usage histograms 4.
Workers support 5. WPT tests (possibly).
Explainer
https://github.com/explainers-by-googlers/multicast-in-direct-sockets.
Chrome Entry https://chromestatus.com/feature/5073740211814400 Intent to
prototype
https://groups.google.com/a/chromium.org/g/blink-dev/c/ADV4FZtN4nE
The functionality is gated behind a blink flag MulticastInDirectSockets.
- M content/browser/direct_sockets/direct_sockets_service_impl.cc
- M content/browser/direct_sockets/direct_sockets_udp_browsertest.cc
- M third_party/blink/public/mojom/direct_sockets/direct_sockets.mojom
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/bindings/idl_in_modules.gni
- M third_party/blink/renderer/modules/DEPS
- M third_party/blink/renderer/modules/direct_sockets/BUILD.gn
- M third_party/blink/renderer/modules/direct_sockets/README.md
- A third_party/blink/renderer/modules/direct_sockets/multicast_controller.cc
- A third_party/blink/renderer/modules/direct_sockets/multicast_controller.h
- A third_party/blink/renderer/modules/direct_sockets/multicast_controller.idl
- M third_party/blink/renderer/modules/direct_sockets/socket_connection.idl
- M third_party/blink/renderer/modules/direct_sockets/socket_options.idl
- M third_party/blink/renderer/modules/direct_sockets/udp_socket.cc
- M third_party/blink/renderer/modules/direct_sockets/udp_socket.h
Code-Review: +1 by Camille Lamy, +1 by Andrew Rayskiy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yifan Luo (Gerrit)
Yifan Luo has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |