Add SSM support to Blink MulticastController and browser integration [chromium/src : main]
Vlad Krot (Gerrit)
Vlad Krot voted and added 3 comments![]( "Open in Gerrit")
Votes added by Vlad Krot
| Commit-Queue | +1 |
3 comments
String ToString() const {Won't it be easier to directly use MembershipKey as a HashMap key?
SourceSpecificMulticastInDirectSocketsEnabled());Add a testcase for that please, it is not clear that this is actually unreachable.
[RuntimeEnabled=SourceSpecificMulticastInDirectSockets]Inconsistent that here sourceAddress is RuntimeEnabled, but above is not
Related details
- Omar Ramadan
- Reilly Grant
- Vlad Krot
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. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
Thanks for the great job. The code looks good. Note, that this feature would benefit from devtools addition (it is there for direct sockets and multicast) however, it is up to devtools owners to see if this is blocking or not.
Related details
- Omar Ramadan
- Reilly Grant
Omar Ramadan (Gerrit)
Message from Omar Ramadan![]( "Open in Gerrit")
PS10 summary for reviewers: (1) unified the controller's membership bookkeeping into one map of (group, optional source) -> {joining|joined} state, replacing the separate joined_groups_ map plus a string-prefix scan over pending join keys — the ASM/SSM mixing check (committed + in-flight) is now a single structural loop; (2) fixed a runtime-feature gating bug: depends_on: [DirectSockets] checks the static flag, but DirectSockets is enabled per-context for IWAs, so the SSM feature (and the sourceAddress dictionary member) was silently disabled exactly where it matters — caught by the previously-never-run SSM browsertests, which now pass; (3) trimmed test scaffolding (shared join helper, single DeriveSsmSourceAddresses() test API, removed dead fake-socket members) — net -104 lines. Verified: 39/39 blink unittests, 67/67 DirectSockets content_browsertests (Linux-equivalent config on mac-x64), 12/12 IWA multicast browser_tests on macOS x64.
Related details
- Reilly Grant
- Vlad Krot
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. |
Omar Ramadan (Gerrit)
Message from Omar Ramadan![]( "Open in Gerrit")
The PS9 CQ dry-run failures (linux-chromeos-rel: JoinGroupSSMInvalidSource / JoinGroupSSMSameGroupDifferentSources / LeaveGroupSSMMustMatchSource / MulticastCannotMixASMAndSSM / MulticastJoinGroupSSMSameGroupDifferentSources; mac-rel: JoinGroupSSMInvalidSource) are all one root cause, fixed in PS10: the runtime feature had depends_on: [DirectSockets], which gates on the static flag, while the browsertest/IWA fixtures enable DirectSockets per-context — so SourceSpecificMulticastInDirectSockets stayed disabled and the sourceAddress dictionary member was dropped at IDL conversion, silently turning SSM joins into ASM joins. The failing tests are precisely the ones whose outcome differs under that distortion. Reproduced locally on PS9 (same failure set) and verified green on PS10 (15/15 DirectSocketsBoundUdp content_browsertests, 12/12 IWA multicast browser_tests). PS10 should be ready for another dry run.
Omar Ramadan (Gerrit)
Omar Ramadan added 3 comments![]( "Open in Gerrit")
Message
PS11 addresses the review comments: IDL-gating unit tests for the sourceAddress CHECK, and RuntimeEnabled on MulticastMembership.sourceAddress for consistency. All 27 multicast blink_unittests pass locally, including the two new ones. PS10 additionally fixed the depends_on static-gating bug that caused the PS9 dry-run failures (mac-rel JoinGroupSSMInvalidSource and the linux-chromeos-rel content_browsertests) — a fresh CQ dry run on PS11 should come back green.
3 comments
String group_string = String::FromUtf8(group.ToString());I considered that, but kept the String: the serialized form doubles as the correlation token bound into the mojo JoinGroup/LeaveGroup callbacks, and using MembershipKey directly as the HashMap key would need custom WTF HashTraits — including empty/deleted sentinel values for net::IPAddress — which is more machinery than the one ToString() it would save. Happy to switch if you feel strongly.
"sourceAddress and ipAddress must be the same IP version");Done in PS11 — added MulticastControllerTest.IdlDropsSourceAddressWhenFeatureDisabled (and the ...Enabled counterpart), which push a script object carrying sourceAddress through the generated V8MulticastGroupOptions converter. With the feature disabled the member is dropped at conversion, so options built from bindings can never carry a source address and this CHECK is unreachable from script. (C++ callers can still construct such options directly, which is exactly what the CHECK guards.)
dictionary MulticastGroupOptions {Done in PS11 — MulticastMembership.sourceAddress is now also [RuntimeEnabled=SourceSpecificMulticastInDirectSockets]. No behavior change (SSM memberships cannot exist when the feature is off, so the output member was never emitted), but the gating now reads consistently on both dictionaries.
Related details
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Code-Review+1![]( "Open in Gerrit")
- Omar Ramadan
- Reilly Grant
Omar Ramadan (Gerrit)
Omar Ramadan voted Commit-Queue+1![]( "Open in Gerrit")
PS12: rebased onto tip-of-tree. The PS11 CQ dry run failed at patch-application (chrome/test/BUILD.gn conflict), not on the code. Both of the CL's BUILD.gn edits — adding //chrome/browser/direct_sockets:browser_tests to deps and dropping the moved direct_sockets_apitest.cc source line — have since landed upstream independently, so that file is now a no-op in this CL and the tree applies cleanly. Re-running the dry run. Thanks for the Code-Review+1.
| Commit-Queue | +1 |
Related details
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan voted Commit-Queue+1![]( "Open in Gerrit")
Omar Ramadan (Gerrit)
Message from Omar Ramadan![]( "Open in Gerrit")
PS12 is rebased onto tip-of-tree and now mergeable. The PS11 dry run failed at patch-application only (chrome/test/BUILD.gn conflict), not on the code — and both of this CL's edits to that file (adding //chrome/browser/direct_sockets:browser_tests to deps, dropping the moved direct_sockets_apitest.cc source line) have since landed upstream independently, so the file is now a no-op here and the tree applies clean. 8027359 is rebased on top as well. I don't have CQ access, so when you have a moment could you re-run the dry run on PS12 (and CQ+2 to land if it's green)? Thanks for the CR+1.
Vlad Krot (Gerrit)
Vlad Krot voted Code-Review+1![]( "Open in Gerrit")
- Omar Ramadan
- Reilly Grant
Omar Ramadan (Gerrit)
Message from Omar Ramadan![]( "Open in Gerrit")
Added owners for the files outside vkrot@'s direct_sockets coverage (CQ dry run passed on PS12):
- tkent@: third_party/blink/renderer/platform/runtime_enabled_features.json5 (Blink API_OWNERS) and third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py (blinkpy OWNERS).
- avi@: content/public/test/OWNERS (per-file redirect added for the direct_sockets test helpers).
- PTAL — thanks!
Related details
- Avi Drissman
- Kent Tamura
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
- Kent Tamura
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 14 comments![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
14 comments
return group_string + "@" + String::FromUtf8(source->ToString());`operator+` for `blink::String` is discouraged. Use `StrCat()` instead.
class MulticastGroupOptions;nit: add blank lines before and after this line for better readability.
// multicast_controller.idl:IMO we shouldn't remove this comment.
// multicast_controller.idl:IMO we shouldn't remove this comment.
const char* param_name,Use `StringView` instead of `const char*`.
exception_state.ThrowTypeError(String(param_name) +Use `StrCat()` instead.
const char* param_name,Use StringView instead of const char*.
exception_state.ThrowTypeError(String(param_name) +Use `StrCat()` instead.
const char* param_name,Use StringView instead of const char*.
String(param_name) + " must not be the zero address (0.0.0.0 or ::)");Use `StrCat()` instead.
class MulticastControllerSSMTest : public testing::Test {`SSMTest` should `SsmTest`.
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> When an abbreviation is used, prefer to capitalize it as a single "word", e.g., StartRpc() rather than StartRPC().
TEST_F(MulticastControllerSSMTest, CannotLeaveASMGroupWithSourceAddress) {`ASM` should be `Asm`.
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
TEST_F(MulticastControllerSSMTest, TOCTOU_PendingASMBlocksSSMJoin) {`Toctou_PendingAsmBlocksSssmJoin`
Related details
- Omar Ramadan
- Reilly Grant
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. |
Omar Ramadan (Gerrit)
Message from Omar Ramadan![]( "Open in Gerrit")
PS13 addresses tkent@'s review:
- multicast_controller.cc: const char* param_name -> StringView; String(param_name)+"..." -> StrCat({param_name, "..."}) in all four validators.
- multicast_controller.h: MembershipKey::ToString now uses StrCat instead of operator+; restored the two `// multicast_controller.idl:` comments above joinGroup/leaveGroup; added a blank line before the MulticastGroupOptions forward-decl.
- unittest: MulticastControllerSSMTest -> ...SsmTest; ASM/SSM -> Asm/Ssm in test names; TOCTOU_PendingASMBlocksSSMJoin -> PendingAsmBlocksSsmJoin.
vkrot@'s three earlier threads were handled in PS11 and carry forward: cc:144 the CHECK is exercised by IdlDropsSourceAddressWhenFeatureDisabled/...Enabled; idl:7 MulticastMembership.sourceAddress is now [RuntimeEnabled]; h:78 kept String key (mojo-callback correlation token; a MembershipKey HashMap key needs custom HashTraits) - happy to switch if preferred. PTAL.
Related details
- Avi Drissman
- Kent Tamura
- Reilly Grant
- Vlad Krot
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. |
Omar Ramadan (Gerrit)
Omar Ramadan added 3 comments![]( "Open in Gerrit")
net::IPAddress group;Kept the String key: its serialized form doubles as the mojo JoinGroup/LeaveGroup callback correlation token, and a MembershipKey HashMap key would need custom WTF HashTraits (empty/deleted sentinels for net::IPAddress) — more machinery than the one ToString() it saves. Happy to switch if you prefer.
return std::nullopt;Done — the CHECK is now exercised by MulticastControllerTest.IdlDropsSourceAddressWhenFeatureDisabled and ...KeepsSourceAddressWhenFeatureEnabled (PS11+), which drive a script object through the V8 dictionary converter under each feature state.
dictionary MulticastGroupOptions {Done in PS11 — MulticastMembership.sourceAddress is now also [RuntimeEnabled=SourceSpecificMulticastInDirectSockets], consistent with MulticastGroupOptions.sourceAddress.
Omar Ramadan (Gerrit)
Message from Omar Ramadan![]( "Open in Gerrit")
PS14 is clang-format-corrected + compile-verified (multicast_controller.o + multicast_controller_unittest.o build clean). PS13 to PS14 is formatting-only (StrCat line-wraps + include ordering) with no logic change vs the reviewed PS13, so it should be a quick re-stamp.
@vkrot could you re-approve on PS14 and set CQ+1 for a dry run? The dry run gives us a native blink_unittests run on the bots. I was unable to complete a clean local/devbox run to exercise the MulticastController* tests directly - the frozen devbox tree failed in the devtools optimize_images resource step, unrelated to this change. omar@ has no CQ rights, hence the ask.
Related details
- Avi Drissman
- Kent Tamura
- Reilly Grant
- Vlad Krot
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. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
StringView param_name,nit: Unlike `std::string_view`, `StringView` should be passed as `const StringView&`.
Related details
- Avi Drissman
- Omar Ramadan
- Reilly Grant
- Vlad Krot
Omar Ramadan (Gerrit)
Omar Ramadan added 1 comment![]( "Open in Gerrit")
Message
Done in PS15 - all three validators now take const StringView&. Verified: git cl format clean + multicast_controller.o recompiles clean.
1 comment
Related details
- Avi Drissman
- Kent Tamura
- Reilly Grant
- Vlad Krot
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. |
Omar Ramadan (Gerrit)
Omar Ramadan added 14 comments![]( "Open in Gerrit")
Message
Resolving remaining review threads - all were addressed in code across PS13-PS15 (StringView/const-ref params, StrCat over operator+, restored .idl comments, blank-line nit, Ssm/Asm/Toctou test renames). Marking the threads resolved now; apologies they were left open. Verified: git cl format clean, multicast_controller.o compiles clean, and blink_unittests MulticastController* = 27/27 pass natively.
14 comments
Done in PS15.
uint64_t inspector_id);Done in PS15.
DEFINE_WRAPPERTYPEINFO();Done in PS15.
namespace blink {Done in PS15.
std::optional<net::IPAddress> ParseAndValidateIPAddress(Done in PS15.
net::IPAddress::FromIPLiteral(ip_string.Ascii());Done in PS15.
std::optional<net::IPAddress> ParseAndValidateMulticastAddress(Done in PS15.
Done in PS15.
std::optional<net::IPAddress> ParseAndValidateUnicastAddress(Done in PS15.
if (parsed_ip->IsMulticast()) {Done in PS15.
exception_state.ThrowTypeError(Done in PS15.
class MulticastControllerSsmTest : public testing::Test {Done in PS15.
TEST_F(MulticastControllerSsmTest, CannotLeaveAsmGroupWithSourceAddress) {Done in PS15.
TEST_F(MulticastControllerSsmTest, PendingAsmBlocksSsmJoin) {Done in PS15.
Related details
- Avi Drissman
- Kent Tamura
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Kent Tamura (Gerrit)
Kent Tamura voted Code-Review+1![]( "Open in Gerrit")
- Avi Drissman
- Omar Ramadan
- Reilly Grant
- Vlad Krot
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Omar Ramadan
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Omar Ramadan
- Reilly Grant
Reilly Grant (Gerrit)
Reilly Grant added 4 comments![]( "Open in Gerrit")
return ('joinGroupSSM failed: ' + error);```suggestion
return 'joinGroupSSM failed: ' + error;
```
return 'joinGroupSSMTwiceWithSameSource succeeded.';Check the error name?
return 'joinGroupSSMInvalidSource succeeded.';Check the error name?
This file is missing a copyright header.
Related details
- Omar Ramadan
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. |
Omar Ramadan (Gerrit)
Omar Ramadan added 4 comments![]( "Open in Gerrit")
Message
PS16 addresses all four comments: error-name assertions in joinGroupSSMTwiceWithSameSource (InvalidStateError) and joinGroupSSMInvalidSource (TypeError), dropped the redundant parens, and added the copyright header to multicast_controller.idl. JS syntax-checked; C++ EXPECT_EQ success strings unchanged.
4 comments
return 'joinGroupSSM failed: ' + error;Done in PS16.
if (e.name !== 'InvalidStateError') {Done in PS16 - now checks e.name === 'InvalidStateError' (what the controller throws for a duplicate group/source join), matching the idiom in cannotMixASMAndSSM.
// If we reach here, test failedDone in PS16 - now checks e.name === 'TypeError' (thrown by address validation for a non-parseable source).
// Copyright 2025 The Chromium AuthorsDone in PS16 - added the standard copyright header.
Related details
- Avi Drissman
- Kent Tamura
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |