Summary: | WebRTC data channel only applications require capture permissions for direct connections | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lennart Grahl <lennart.grahl> | ||||||||||||||||||||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, ashley, ben.browitt, cdumez, commit-queue, damien, db, eric.carlson, ews-watchlist, jana, jonlee, juberti, pascal, philipp.kuederli, rniwa, ryanhaddad, sebastian.schenk, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | Safari Technology Preview | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Lennart Grahl
2017-07-14 05:34:18 PDT
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. *** 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. 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. Created attachment 336643 [details]
Patch
(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 > 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.
Created attachment 336667 [details]
Patch
Created attachment 336670 [details]
Patch
Created attachment 336673 [details]
Patch
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 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
Created attachment 336685 [details]
Patch
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 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
Created attachment 336716 [details]
Patch
Created attachment 336719 [details]
Patch
Created attachment 336720 [details]
Patch
Putting under review. It seems that on iOS simulator the 250 ms MDNS timeout might be too small. 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. Created attachment 337239 [details]
Patch for landing
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.
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. Created attachment 337243 [details]
Patch for landing
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.
Comment on attachment 337243 [details] Patch for landing Clearing flags on attachment: 337243 Committed r230290: <https://trac.webkit.org/changeset/230290> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 337275 [details]
Patch
Comment on attachment 337275 [details] Patch Clearing flags on attachment: 337275 Committed r230307: <https://trac.webkit.org/changeset/230307> All reviewed patches have been landed. Closing bug. So, was the experiment a success? Would you like to pursue this further and incorporate it into the spec? *** Bug 186302 has been marked as a duplicate of this bug. *** |