Bug 197851

Summary: getUserMedia sandbox extensions should not be revoked when a getUserMedia allowed request is being processed
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, eric.carlson, ews-watchlist, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Patch for landing none

Description youenn fablet 2019-05-13 13:26:43 PDT
getUserMedia sandbox should not be revoked when a getUserMedia allowed request is being processed
Comment 1 youenn fablet 2019-05-13 15:21:13 PDT
Created attachment 369788 [details]
Patch
Comment 2 EWS Watchlist 2019-05-14 08:58:44 PDT
Comment on attachment 369788 [details]
Patch

Attachment 369788 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12188112

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html
Comment 3 EWS Watchlist 2019-05-14 08:58:47 PDT
Created attachment 369855 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 4 youenn fablet 2019-05-14 14:20:10 PDT
Created attachment 369891 [details]
Patch
Comment 5 EWS Watchlist 2019-05-14 14:22:13 PDT
Attachment 369891 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebProcess.cpp:1881:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/WebProcess/WebProcess.cpp:1883:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Radar WebKit Bug Importer 2019-05-14 15:50:25 PDT
<rdar://problem/50786818>
Comment 7 Eric Carlson 2019-05-15 09:00:21 PDT
Comment on attachment 369891 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This is done to ensure that, should capture is failing, the UIProcess

Nit: s/should capture is failing/should capture failing/

> Source/WebKit/ChangeLog:19
> +        Covered by added test by adding a debug assertion that will be hit in case we revoke extensions while a document is capturing.

Nit: s/by added test by adding/by added test and by adding/

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:160
> -    process.send(Messages::WebProcess::RevokeUserMediaDeviceSandboxExtensions(params), proxy.page().pageID());
> +    process.send(Messages::WebProcess::RevokeUserMediaDeviceSandboxExtensions(params), 0);

Why is this change necessary?
Comment 8 youenn fablet 2019-05-15 09:04:12 PDT
Thanks for the review.

(In reply to Eric Carlson from comment #7)
> Comment on attachment 369891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369891&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        This is done to ensure that, should capture is failing, the UIProcess
> 
> Nit: s/should capture is failing/should capture failing/

OK

> > Source/WebKit/ChangeLog:19
> > +        Covered by added test by adding a debug assertion that will be hit in case we revoke extensions while a document is capturing.
> 
> Nit: s/by added test by adding/by added test and by adding/

OK

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:160
> > -    process.send(Messages::WebProcess::RevokeUserMediaDeviceSandboxExtensions(params), proxy.page().pageID());
> > +    process.send(Messages::WebProcess::RevokeUserMediaDeviceSandboxExtensions(params), 0);
> 
> Why is this change necessary?

There is no need to pass a pageID since the target is the WebProcess not the WebPage.
Maybe we were passing the pageID at some point as it was a WebPage message.
I'll change it for WebProcess::GrantUserMediaDeviceSandboxExtensions as well.
Comment 9 youenn fablet 2019-05-15 10:28:50 PDT
Created attachment 369967 [details]
Patch for landing
Comment 10 EWS Watchlist 2019-05-15 10:31:34 PDT
Attachment 369967 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebProcess.cpp:1881:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/WebProcess/WebProcess.cpp:1883:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Commit Bot 2019-05-15 11:44:42 PDT
Comment on attachment 369967 [details]
Patch for landing

Clearing flags on attachment: 369967

Committed r245335: <https://trac.webkit.org/changeset/245335>
Comment 12 WebKit Commit Bot 2019-05-15 11:44:44 PDT
All reviewed patches have been landed.  Closing bug.