Protect request and web view during gUM client callback to prevent crash if navigation happens during callback.
<rdar://problem/32833102>
Created attachment 314487 [details] Proposed patch.
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 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.
Created attachment 314498 [details] Proposed patch.
Comment on attachment 314498 [details] Proposed patch. Clearing flags on attachment: 314498 Committed r219083: <http://trac.webkit.org/changeset/219083>
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
Reverted r219083 for reason: The revision caused an API failure on all testing platforms. Committed r219094: <http://trac.webkit.org/changeset/219094>
Created attachment 314524 [details] Fix test.
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
Created attachment 314609 [details] Fix test.
Comment on attachment 314609 [details] Fix test. Clearing flags on attachment: 314609 Committed r219135: <http://trac.webkit.org/changeset/219135>