Bug 174096 - [MediaStream] Protect request and web view during gUM client callback
Summary: [MediaStream] Protect request and web view during gUM client callback
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-03 07:59 PDT by Eric Carlson
Modified: 2017-07-05 10:41 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch. (19.84 KB, patch)
2017-07-03 09:03 PDT, Eric Carlson
youennf: review+
Details | Formatted Diff | Diff
Proposed patch. (19.84 KB, patch)
2017-07-03 11:01 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Fix test. (2.75 KB, patch)
2017-07-03 15:02 PDT, Eric Carlson
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fix test. (20.01 KB, patch)
2017-07-05 09:10 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-07-03 07:59:35 PDT
Protect request and web view during gUM client callback to prevent crash if navigation happens during callback.
Comment 1 Eric Carlson 2017-07-03 09:00:46 PDT
<rdar://problem/32833102>
Comment 2 Eric Carlson 2017-07-03 09:03:12 PDT
Created attachment 314487 [details]
Proposed patch.
Comment 3 youenn fablet 2017-07-03 10:20:40 PDT
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?
Comment 4 Eric Carlson 2017-07-03 10:50:08 PDT
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.
Comment 5 Eric Carlson 2017-07-03 11:01:25 PDT
Created attachment 314498 [details]
Proposed patch.
Comment 6 WebKit Commit Bot 2017-07-03 11:55:04 PDT
Comment on attachment 314498 [details]
Proposed patch.

Clearing flags on attachment: 314498

Committed r219083: <http://trac.webkit.org/changeset/219083>
Comment 8 Matt Lewis 2017-07-03 14:38:47 PDT
Reverted r219083 for reason:

The revision caused an API failure on all testing platforms.

Committed r219094: <http://trac.webkit.org/changeset/219094>
Comment 9 Eric Carlson 2017-07-03 15:02:52 PDT
Created attachment 314524 [details]
Fix test.
Comment 10 WebKit Commit Bot 2017-07-03 15:04:20 PDT
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
Comment 11 Eric Carlson 2017-07-05 09:10:43 PDT
Created attachment 314609 [details]
Fix test.
Comment 12 WebKit Commit Bot 2017-07-05 10:41:51 PDT
Comment on attachment 314609 [details]
Fix test.

Clearing flags on attachment: 314609

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