Bug 228649 – Attribute nw connections to the page bundle identifier

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Attribute nw connections to the page bundle identifier

[youenn fablet]

Created attachment 434645 [details]
Patch

[youenn fablet]

Created attachment 434743 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81415612>

[Eric Carlson]

Comment on attachment 434743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434743&action=review

> Source/WebKit/NetworkProcess/NetworkSession.cpp:411
> +    return m_attributedBundleIdentifierFromPageIdentifiers.get(identifier);

It is worth ASSERTing that the identifier is in the map?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:159
> +        if (auto socket = NetworkRTCUDPSocketCocoa::createUDPSocket(identifier, *this, address.value, minPort, maxPort, m_ipcConnection.copyRef(), String(attributedBundleIdentifierFromPageIdentifier(pageIdentifier)), isFirstParty, isRelayDisabled,   WTFMove(domain))) {

Nit: extra space before `WTFMove`

Is this called often enough that it would be worth keeping a pageIdentifier -> attributedBundleIdentifier map in the provider instead of doing the synchronous cross-thread hop every time?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:202
> +        if (auto socket = NetworkRTCTCPSocketCocoa::createClientTCPSocket(identifier, *this, remoteAddress.value, options, attributedBundleIdentifierFromPageIdentifier(pageIdentifier), isRelayDisabled, m_ipcConnection.copyRef())) {

Ditto.

[Alex Christensen]

Comment on attachment 434743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434743&action=review

> Source/WebKit/ChangeLog:9
> +        Use page identifier to get the bundle identifier.

attributed bundle identifier, not the bundle identifier.  The former is set via SPI in contexts such as SFSafariViewController when the view is made on behalf of another application, the latter is set for the executable.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:411
>> +    return m_attributedBundleIdentifierFromPageIdentifiers.get(identifier);
> 
> It is worth ASSERTing that the identifier is in the map?

no,  it is likely not in the map.

> Source/WebKit/NetworkProcess/NetworkSession.h:180
> +    String attributedBundleIdentifierFromPageIdentifier(WebPageProxyIdentifier) const;

This could return a const String&

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:149
> +        return value;

I feel like there should be an isolatedCopy somewhere in here.

[youenn fablet]

Created attachment 434771 [details]
Patch for landing

[youenn fablet]

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:159
> > +        if (auto socket = NetworkRTCUDPSocketCocoa::createUDPSocket(identifier, *this, address.value, minPort, maxPort, m_ipcConnection.copyRef(), String(attributedBundleIdentifierFromPageIdentifier(pageIdentifier)), isFirstParty, isRelayDisabled,   WTFMove(domain))) {
> 
> Nit: extra space before `WTFMove`

OK

> Is this called often enough that it would be worth keeping a pageIdentifier
> -> attributedBundleIdentifier map in the provider instead of doing the
> synchronous cross-thread hop every time?

Yep, this is in the patch.

> > Source/WebKit/ChangeLog:9
> > +        Use page identifier to get the bundle identifier.
> 
> attributed bundle identifier, not the bundle identifier.  The former is set
> via SPI in contexts such as SFSafariViewController when the view is made on
> behalf of another application, the latter is set for the executable.

OK


> > Source/WebKit/NetworkProcess/NetworkSession.h:180
> > +    String attributedBundleIdentifierFromPageIdentifier(WebPageProxyIdentifier) const;
> 
> This could return a const String&

OK

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:149
> > +        return value;
> 
> I feel like there should be an isolatedCopy somewhere in here.

There is one when adding the NetworkSession string to the NetworkRTCProvider map (line 147). I do not think we need an isolated copy every time we return the string:
- The map is only accessed in the background thread.
- The sockets keep a ref of the strings but the sockets are deleted in the background thread.

[youenn fablet]

> - The map is only accessed in the background thread.

I'll clear the map in the background thread when closing even though this is not strictly needed.

[youenn fablet]

Created attachment 434776 [details]
Patch for landing

[youenn fablet]

Created attachment 434778 [details]
Patch for landing

[EWS]

Committed r280556 (240180@main): <https://commits.webkit.org/240180@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434778 [details].