Bug 174500 – WebRTC data channel only applications require capture permissions for direct connections

• Component: WebRTC
• Status: RESOLVED FIXED

[Lennart Grahl]

Currently, ICE candidates of type 'host' will not be gathered/handed out to the application in case no capture permission has been granted. While this may be a perfect solution for audio/video, it leaves data channel only applications with either srflx/relay only or with the need for an unfitting permission request.

The biggest issue with this approach is: How do you explain to the user that you need capture permissions for a direct peer-to-peer connection? That smells fishy and I understand everyone who will decline such a request for this exact reason.

So, if the user rejects this permission request, secondary issues are that you
a) now always need to provide a STUN server, and
b) always need to provide a TURN server (which itself is rather expensive in terms of operating costs and may not be feasible for small open source applications for this reason) because there are plenty of routers who are not capable of doing NAT loopback (yep, even models from 2017). Thus, one cannot rely on server reflexive candidates.

My proposal would be that the application can request a permission to "establish a direct connection" (maybe also displaying some hints on what this means towards privacy, etc.) which enables the use of host candidates.

(P.S.: You should probably add a WebRTC component to your Bugzilla.)

[Radar WebKit Bug Importer]

<rdar://problem/34134281>

[Ben]

Another use case is a viewer in a webinar that only receive audio/video tracks but doesn't transmit. Requesting mic/cam permission might confuse a user that is only intended to be a viewer.

In addition to the proposed additional permission request type I suggest that if the website is saved to the home screen in iOS the restrictions will be relaxed.

[Jon Lee]

*** Bug 176921 has been marked as a duplicate of this bug. ***

In general I wouldn't assume that a WebRTC connection is always bi-directional. In my use case I stream multiple live videos from a server to the client. In this scenario I'm using VPN without any NATs. I also would like to avoid using a TURN server.

As @Lennart Grahl already mentioned, asking the user for cam/mic permission without even really using it is definitely not a good work around.

[Jana]

As @Ben noted, the important use case of streaming video to a user via WebRTC doesn't fit with the permissions request required. To tell a user that they have to grant access to their camera/mic doesn't make any sense when all they are trying to do is view a video without sending any media back. No user will understand why they have to give permission in this scenario. Basically, getUserMedia shouldn't require camera/mic permissions when the data is not bidirectional. Developers should be able to specify that the data is streaming to the user only, and therefore doesn't require permission. If you decide that it's necessary to request permission, the permission should be fit for the purpose rather than requiring them to grant access to permissions for components that aren't being used.

[youenn fablet]

Created attachment 336643 [details]
Patch

[Lennart Grahl]

(In reply to youenn fablet from comment #6)
> Created attachment 336643 [details]
> Patch

Youenn,

using mDNS with UUIDs seems like a clever approach to me and is definitely a step forward which I appreciate. If you plan on suggesting to specify this in ICE (or as an extension to ICE) - great! If not, then this is a proprietary solution limited to Webkit.

Cheers
Lennart

[youenn fablet]

> using mDNS with UUIDs seems like a clever approach to me and is definitely a
> step forward which I appreciate. If you plan on suggesting to specify this
> in ICE (or as an extension to ICE) - great! If not, then this is a
> proprietary solution limited to Webkit.

This is an experiment. If proved to be successful, we hope it could be made interoperable with other browsers.

[youenn fablet]

Created attachment 336667 [details]
Patch

[youenn fablet]

Created attachment 336670 [details]
Patch

[youenn fablet]

Created attachment 336673 [details]
Patch

[EWS Watchlist]

Comment on attachment 336673 [details]
Patch

Attachment 336673 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7126070

New failing tests:
webrtc/datachannel/mdns-ice-candidates.html

[EWS Watchlist]

Created attachment 336681 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 336685 [details]
Patch

[EWS Watchlist]

Comment on attachment 336685 [details]
Patch

Attachment 336685 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7127141

New failing tests:
webrtc/datachannel/mdns-ice-candidates.html

[EWS Watchlist]

Created attachment 336709 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 336716 [details]
Patch

[youenn fablet]

Created attachment 336719 [details]
Patch

[youenn fablet]

Created attachment 336720 [details]
Patch

[youenn fablet]

Putting under review.
It seems that on iOS simulator the 250 ms MDNS timeout might be too small.

[Eric Carlson]

Comment on attachment 336720 [details]
Patch

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

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:289
> +            provider.resolveMDNSName(name, [peerConnection = makeRef(m_peerConnection), this, name, iceCandidate = makeRef(*iceCandidate), promise = WTFMove(promise)] (auto&& result) mutable {

Nit: I think "auto" is not a good choice here because the only way for someone that isn't already familiar with the code to figure out "result"'s type is to dig through the libWebRTCProvider header.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:295
> +                    // FIXME: Log error.

It is probably worth logging the error to the console in this patch, even if that isn't the long term solution.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:435
> +        // FIXME: We might need to clear al pending candidates when setting again local description.

Nits: "al" => "all"
"when setting again local description" => "when setting local description again"

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:467
> +    provider.registerMDNSName(document, ipAddress, [peerConnection = makeRef(m_peerConnection), this, ipAddress] (auto&& result) {

Ditto the "auto" comment above.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:473
> +            // FIXME: Log error.

Ditto the comment above about logging the error.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:78
> +    virtual void unregisterMDNSNames(Document&) { }
> +
> +    virtual void registerMDNSName(Document& document, const String& ipAddress, CompletionHandler<void(MDNSNameOrError&&)>&& callback)

It is a layering violation to use Document here. Can you just use the identifier?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:80
> +        request->connection->send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Why not log the error here, or pass the error value to be logged in the web process?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:92
> +            m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:96
> +            m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:104
> +    String baseName = createCanonicalUUIDString();

Nit: is the local variable necessary?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:109
> +    if (ip == ( in_addr_t)(-1)) {

Nit: unnecessary space after the "(".

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:110
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto the comment above about logging or passing a meaningful error to the web process.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:129
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:154
> +        connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:173
> +        request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:181
> +        request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:193
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:207
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:214
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:88
> +        finishedRegisteringMDNSName(m_pendingRequestsIdentifier, makeUnexpected(MDNSRegisterError::General));

Ditto.

> Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:97
> +        finishedResolvingMDNSName(m_pendingRequestsIdentifier, makeUnexpected(MDNSRegisterError::General));

Ditto.

[youenn fablet]

Created attachment 337239 [details]
Patch for landing

[EWS Watchlist]

Attachment 337239 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Network/webrtc/LibWebRTCProvider.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:60:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:95:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Thanks for the review, I took it all.
Next step is integrating the MDNS resolution at some lower level so that we do not add this unnecessary delay, ideally at libwebrtc level, otherwise at the WebCore/libwebrtc binding level.

[youenn fablet]

Created attachment 337243 [details]
Patch for landing

[EWS Watchlist]

Attachment 337243 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Network/webrtc/LibWebRTCProvider.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:60:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 337243 [details]
Patch for landing

Clearing flags on attachment: 337243

Committed r230290: <https://trac.webkit.org/changeset/230290>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 337275 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337275 [details]
Patch

Clearing flags on attachment: 337275

Committed r230307: <https://trac.webkit.org/changeset/230307>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lennart Grahl]

So, was the experiment a success? Would you like to pursue this further and incorporate it into the spec?

[youenn fablet]

*** Bug 186302 has been marked as a duplicate of this bug. ***