Bug 174096 – [MediaStream] Protect request and web view during gUM client callback

• Component: Media
• Status: ASSIGNED

[Eric Carlson]

Protect request and web view during gUM client callback to prevent crash if navigation happens during callback.

[Eric Carlson]

<rdar://problem/32833102>

[Eric Carlson]

Created attachment 314487 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 314487 [details]
Proposed patch.

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

> Source/WebKit2/ChangeLog:9
> +        Retain the message and WebView during asynchronous calls so they won't be 

s/message/request/?
It is not totally clear to me why there is a need to retain both request and WebView. Can you detail why?

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:430
> +    [delegate _webView:protectedWebView.get() requestUserMediaAuthorizationForDevices:devices url:requestFrameURL mainFrameURL:mainFrameURL decisionHandler:decisionHandler.get()];

I am not very familiar with ObjC and BlockPtr.
I guess there is no destruction issue but just in case, I guess BlockPtr is different from unique_ptr for instance.
why are we doing decisionHandler.get() here?

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:449
> +    auto requestCameraAuthorization = BlockPtr<void()>::fromCallable([this, &frame, protectedRequest = makeRef(request), webView = RetainPtr<WKWebView>(m_uiDelegate.m_webView)]() {

requestCameraAuthorization is used in cases where this could be called synchronously.
Maybe we could make the inside of requestCameraAuthorization a helper method so that we would capture the request and the web view only in the case we need to call the helper method asynchronously?

[Eric Carlson]

Comment on attachment 314487 [details]
Proposed patch.

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

>> Source/WebKit2/ChangeLog:9
>> +        Retain the message and WebView during asynchronous calls so they won't be 
> 
> s/message/request/?
> It is not totally clear to me why there is a need to retain both request and WebView. Can you detail why?

If the page navigates, WebKit's cleanup code will release the request so request.allow() or request.deny() will crash. The WebView can be released during the callback, as is done in my API test case.

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:430
>> +    [delegate _webView:protectedWebView.get() requestUserMediaAuthorizationForDevices:devices url:requestFrameURL mainFrameURL:mainFrameURL decisionHandler:decisionHandler.get()];
> 
> I am not very familiar with ObjC and BlockPtr.
> I guess there is no destruction issue but just in case, I guess BlockPtr is different from unique_ptr for instance.
> why are we doing decisionHandler.get() here?

The decisionHandler parameter to this method is an ObjC block, but it isn't possible to cast from a lambda to a block.  BlockPtr was created for just this.

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:449
>> +    auto requestCameraAuthorization = BlockPtr<void()>::fromCallable([this, &frame, protectedRequest = makeRef(request), webView = RetainPtr<WKWebView>(m_uiDelegate.m_webView)]() {
> 
> requestCameraAuthorization is used in cases where this could be called synchronously.
> Maybe we could make the inside of requestCameraAuthorization a helper method so that we would capture the request and the web view only in the case we need to call the helper method asynchronously?

The code is iOS only, and avoiding an extra retain in the one case where it is called synchronously doesn't seem worth adding an iOS only method.

[Eric Carlson]

Created attachment 314498 [details]
Proposed patch.

[WebKit Commit Bot]

Comment on attachment 314498 [details]
Proposed patch.

Clearing flags on attachment: 314498

Committed r219083: <http://trac.webkit.org/changeset/219083>

[Matt Lewis]

This patch caused the API test WebKit2.NavigateDuringGetUserMediaPrompt to fail on all testers.

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/2712
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/2712/steps/run-api-tests/logs/stdio

[Matt Lewis]

Reverted r219083 for reason:

The revision caused an API failure on all testing platforms.

Committed r219094: <http://trac.webkit.org/changeset/219094>

[Eric Carlson]

Created attachment 314524 [details]
Fix test.

[WebKit Commit Bot]

Comment on attachment 314524 [details]
Fix test.

Rejecting attachment 314524 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 314524, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
s/TestWebKitAPI/Tests/WebKit2/GetUserMediaNavigation.mm
|===================================================================
|--- Tools/TestWebKitAPI/Tests/WebKit2/GetUserMediaNavigation.mm	(revision 219093)
|+++ Tools/TestWebKitAPI/Tests/WebKit2/GetUserMediaNavigation.mm	(working copy)
--------------------------
No file to patch.  Skipping patch.
3 out of 3 hunks ignored

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4047065

[Eric Carlson]

Created attachment 314609 [details]
Fix test.

[WebKit Commit Bot]

Comment on attachment 314609 [details]
Fix test.

Clearing flags on attachment: 314609

Committed r219135: <http://trac.webkit.org/changeset/219135>