Bug 167430

Summary: Implement <iframe allow="camera; microphone">
Product: WebKit Reporter: Simon Pieters (:zcorpan) <zcorpan>
Component: DOMAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, cdumez, commit-queue, dccollie, eric.carlson, ews-watchlist, jonlee, marc, mike.stark, rniwa, szmydadam, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=183300
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Patch
none
Patch for landing
none
Patch for landing
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Patch for landing with fixed message
none
Patch for landing with fixed message
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews114 for mac-elcapitan none

Description Simon Pieters (:zcorpan) 2017-01-25 13:47:58 PST
+++ This bug was initially created as a clone of Bug #167417 +++

I guess WebKit doesn't implement getUserMedia() yet(?), but filing this anyway...

<iframe allowusermedia> controls if an iframe can access the getUserMedia() API.

On the spec side:

* When a document is initialized (https://html.spec.whatwg.org/#creating-a-new-browsing-context and https://html.spec.whatwg.org/#initialise-the-document-object ), https://html.spec.whatwg.org/#set-the-allow*-flags is run, which can set "allowusemedia flag" on Document.
* getUserMedia also call allowed to use: https://github.com/w3c/mediacapture-main/pull/427

Relevant whatwg/html PRs:

https://github.com/whatwg/html/pull/2195
https://github.com/whatwg/html/pull/2217

Relevant w3c/web-platform-tests issue to add tests:

https://github.com/w3c/web-platform-tests/issues/4625
Comment 1 Dave Collie 2017-10-04 12:29:02 PDT
The recent release of Safari 11 adds support for WebRTC.  This opens up an opportunity to finally replace Flash based camera recording.  However, the following checks prevent calling getUserMedia from an iframe.

https://github.com/WebKit/webkit/blob/master/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp#L116

https://github.com/WebKit/webkit/blob/master/Source/WebCore/page/SecurityOrigin.cpp#L563

If 'allowusermedia' was implemented it would allow different webrtc camera recording implementations that are iframed into client sites to function.  Currently, they have to fallback to flash.
Comment 2 Jon Lee 2017-10-04 17:04:25 PDT
(In reply to Dave Collie from comment #1)
> The recent release of Safari 11 adds support for WebRTC.  This opens up an
> opportunity to finally replace Flash based camera recording.  However, the
> following checks prevent calling getUserMedia from an iframe.
> 
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/Modules/
> mediastream/UserMediaRequest.cpp#L116
> 
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/page/
> SecurityOrigin.cpp#L563
> 
> If 'allowusermedia' was implemented it would allow different webrtc camera
> recording implementations that are iframed into client sites to function. 
> Currently, they have to fallback to flash.

What different implementations are out there that have wide deployment?
Comment 3 Dave Collie 2017-10-05 05:51:14 PDT
Cameratag (https://cameratag.com/demo) currently doesn't work with Safari 11 unless flash fallback is forced.

It doesn't require iframing but there are use cases where it's easier to implement cameratag on a subdomain and embed in other services.  

The company I work for is using the same strategy for our own recording platform and partner ecosystem.  We were hoping with Safari 11 that we could finally relegate flash camera recording to IE11 only.

For Chrome we use the 'allow' attribute https://wicg.github.io/feature-policy/#iframe-allow-attribute to permit accessing the camera and microphone.  This doesn't appear to be implemented in Firefox.
Comment 4 youenn fablet 2017-10-07 17:13:01 PDT
A mechanism to allow iframes to access camera/mic on behalf of the top document makes sense to me.
That said, allowusermedia might be removed from the spec and replaced by something like https://wicg.github.io/feature-policy/
Comment 5 youenn fablet 2017-10-09 08:25:52 PDT
Created attachment 323176 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2017-10-09 09:42:55 PDT
<rdar://problem/34887226>
Comment 7 Build Bot 2017-10-09 09:50:57 PDT
Comment on attachment 323176 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-MediaElement-srcObject.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-audio-only.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-id-manual.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-idl.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStreamTrack-MediaElement-disabled-audio-is-silence.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStreamTrack-getSettings.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.sub.html
imported/w3c/web-platform-tests/mediacapture-streams/GUM-trivial-constraint.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-enumerateDevices.html
imported/w3c/web-platform-tests/mediacapture-streams/GUM-deny.https.html
imported/w3c/web-platform-tests/mediacapture-streams/GUM-impossible-constraint.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-video-only.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-MediaElement-preload-none.https.html
imported/w3c/web-platform-tests/mediacapture-streams/GUM-optional-constraint.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-finished-add.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-removetrack.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-add-audio-track.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStreamTrack-init.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStreamTrack-id.https.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-gettrackid.https.html
Comment 8 Build Bot 2017-10-09 09:50:58 PDT
Created attachment 323179 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 youenn fablet 2017-10-09 10:13:00 PDT
Created attachment 323185 [details]
Patch
Comment 10 Jon Lee 2017-10-09 12:12:40 PDT
Comment on attachment 323185 [details]
Patch

We need to discuss this change in policy. Third-party frames are disallowed for a reason.
Comment 11 Adam Szmyd 2017-11-06 05:06:51 PST
Is there any progress on this one? We're implementing a voice client that uses iFrames to allow users to plug our embedded iFrame on their side and allow communication. 

Current Safari11 gives us this error "Trying to call getUserMedia from a document with a different security origin than its top-level frame." 

Can someone give me an answer if capturing microphone from cross-domain iFrame (both host and child iFrame on HTTPS of course) will be possible?
Comment 12 youenn fablet 2017-11-08 09:41:32 PST
(In reply to Adam Szmyd from comment #11)
> Is there any progress on this one? We're implementing a voice client that
> uses iFrames to allow users to plug our embedded iFrame on their side and
> allow communication. 
> 
> Current Safari11 gives us this error "Trying to call getUserMedia from a
> document with a different security origin than its top-level frame." 
> 
> Can someone give me an answer if capturing microphone from cross-domain
> iFrame (both host and child iFrame on HTTPS of course) will be possible?

It was discussed at W3C TPAC and getUserMedia spec should be updated to support feature policy. That should help adoption here.
Comment 13 Eric Carlson 2017-11-09 08:59:03 PST
(In reply to youenn fablet from comment #12)
> 
> It was discussed at W3C TPAC and getUserMedia spec should be updated to
> support feature policy. That should help adoption here.

https://github.com/w3c/mediacapture-main/pull/440
Comment 14 Eric Carlson 2017-11-09 09:40:20 PST
Comment on attachment 323185 [details]
Patch

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:132
> +    for (auto allowItem : StringView { allow }.split(';')) {
> +        auto item = allowItem.stripLeadingAndTrailingMatchedCharacters(isHTMLSpace<UChar>);
> +        if (requiresAudio && item == "microphone")
> +            requiresAudio = false;
> +        if (requiresVideo && item == "camera")
> +            requiresVideo = false;
> +    }
> +    return !requiresVideo && !requiresAudio;

Nit: I think using local variable names like "allowCameraAccess" and "allowMicrophoneAccess" and revising this logic would make it easier to follow.
Comment 15 youenn fablet 2017-12-14 16:30:13 PST
Created attachment 329418 [details]
Patch for landing
Comment 16 youenn fablet 2017-12-14 16:33:06 PST
Created attachment 329419 [details]
Patch for landing
Comment 17 Jon Lee 2017-12-14 16:51:37 PST
Comment on attachment 329419 [details]
Patch for landing

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
> +static bool isAllowedToUse(Document& document, Document& topDocument, bool requiresAudio, bool requiresVideo)

Don't we normally eschew bool parameters and go for enums instead?

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:153
> +                return false;

Is there an appropriate errorMessage we can return here?

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:161
>                  errorMessage = "Trying to call getUserMedia from a document with a different security origin than its top-level frame.";

This doesn't seem like the right message now. It should be something along the lines of "The top-level frame has prevented a document with a different security origin to call getUserMedia."
Comment 18 youenn fablet 2017-12-14 17:28:46 PST
Comment on attachment 329419 [details]
Patch for landing

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

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
>> +static bool isAllowedToUse(Document& document, Document& topDocument, bool requiresAudio, bool requiresVideo)
> 
> Don't we normally eschew bool parameters and go for enums instead?

We usually do that for class methods or helper functions defined in headers.
Since this is a static inline here and given the fact that we would need to compute the enum value at the call site, it does not seem very attractive.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:153
>> +                return false;
> 
> Is there an appropriate errorMessage we can return here?

I'll dig into it, I was thinking this could be some disconnected frame cases but I should check further.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:161
>>                  errorMessage = "Trying to call getUserMedia from a document with a different security origin than its top-level frame.";
> 
> This doesn't seem like the right message now. It should be something along the lines of "The top-level frame has prevented a document with a different security origin to call getUserMedia."

ok, will update the message.
Comment 19 EWS Watchlist 2017-12-14 17:45:31 PST
Comment on attachment 329419 [details]
Patch for landing

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-default-feature-policy.sub.html
Comment 20 EWS Watchlist 2017-12-14 17:45:33 PST
Created attachment 329429 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 youenn fablet 2017-12-14 17:46:13 PST
Created attachment 329430 [details]
Patch for landing with fixed message
Comment 22 youenn fablet 2017-12-14 17:49:08 PST
Created attachment 329431 [details]
Patch for landing with fixed message
Comment 23 youenn fablet 2017-12-14 17:50:06 PST
(In reply to youenn fablet from comment #22)
> Created attachment 329431 [details]
> Patch for landing with fixed message

Skipped xhr test which is timing out.
Time out is triggered by the fact that the test is now going further due to added support for allow attribute.
Comment 24 youenn fablet 2017-12-14 17:51:56 PST
> > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:153
> > +                return false;
> 
> Is there an appropriate errorMessage we can return here?

I removed this additional check.
This is already in the code base and I saw another place which follows the same pattern without checking document.
There may be a bug though since for a disconnected frame, we might have a document but no parent document. Maybe we should check early for a frame.
This should be fixed in a follow-up anyway.
Comment 25 EWS Watchlist 2017-12-14 19:14:03 PST
Comment on attachment 329431 [details]
Patch for landing with fixed message

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

New failing tests:
http/tests/ssl/media-stream/get-user-media-nested.html
http/tests/ssl/media-stream/get-user-media-different-host.html
Comment 26 EWS Watchlist 2017-12-14 19:14:04 PST
Created attachment 329438 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 27 EWS Watchlist 2017-12-14 19:15:49 PST
Comment on attachment 329431 [details]
Patch for landing with fixed message

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

New failing tests:
http/tests/ssl/media-stream/get-user-media-nested.html
http/tests/ssl/media-stream/get-user-media-different-host.html
Comment 28 EWS Watchlist 2017-12-14 19:15:51 PST
Created attachment 329440 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 29 youenn fablet 2017-12-14 19:26:44 PST
Created attachment 329444 [details]
Patch
Comment 30 EWS Watchlist 2017-12-14 21:22:12 PST
Comment on attachment 329444 [details]
Patch

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

New failing tests:
webgl/1.0.2/conformance/context/context-release-upon-reload.html
Comment 31 EWS Watchlist 2017-12-14 21:22:13 PST
Created attachment 329454 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 32 WebKit Commit Bot 2017-12-14 21:32:58 PST
Comment on attachment 329444 [details]
Patch

Clearing flags on attachment: 329444

Committed r225963: <https://trac.webkit.org/changeset/225963>
Comment 33 WebKit Commit Bot 2017-12-14 21:33:00 PST
All reviewed patches have been landed.  Closing bug.