Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI
Created attachment 424889 [details] Patch
<rdar://problem/76104034>
Comment on attachment 424889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424889&action=review > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:-165 > -- (BOOL)_webView:(WKWebView *)webView shouldRequestGeolocationAuthorizationForURL:(NSURL *)url isMainFrame:(BOOL)isMainFrame mainFrameURL:(NSURL *)mainFrameURL; Are we sure nobody uses this?
(In reply to Alex Christensen from comment #3) > Comment on attachment 424889 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424889&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:-165 > > -- (BOOL)_webView:(WKWebView *)webView shouldRequestGeolocationAuthorizationForURL:(NSURL *)url isMainFrame:(BOOL)isMainFrame mainFrameURL:(NSURL *)mainFrameURL; > > Are we sure nobody uses this? I have not found any use through search tools.
Committed r275407: <https://commits.webkit.org/r275407> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424889 [details].
Comment on attachment 424889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424889&action=review I think this may have caused rdar://76791065 > Source/WebKit/UIProcess/ios/WKGeolocationProviderIOS.mm:182 > + Function<void(bool)> decisionHandler = [completionHandler = WTFMove(request.completionHandler), protectedSelf = retainPtr(self)](bool result) { request.completionHandler gets moved here. > Source/WebKit/UIProcess/ios/WKGeolocationProviderIOS.mm:200 > + auto policyListener = adoptNS([[WKWebAllowDenyPolicyListener alloc] initWithCompletionHandler:WTFMove(request.completionHandler)]); This looks like a use-after-move of request.completionHandler ?
Comment on attachment 424889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424889&action=review >> Source/WebKit/UIProcess/ios/WKGeolocationProviderIOS.mm:200 >> + auto policyListener = adoptNS([[WKWebAllowDenyPolicyListener alloc] initWithCompletionHandler:WTFMove(request.completionHandler)]); > > This looks like a use-after-move of request.completionHandler ? Indeed, request.completionHandler should be replaced with decisionHandler
(In reply to Alex Christensen from comment #7) > Comment on attachment 424889 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424889&action=review > > >> Source/WebKit/UIProcess/ios/WKGeolocationProviderIOS.mm:200 > >> + auto policyListener = adoptNS([[WKWebAllowDenyPolicyListener alloc] initWithCompletionHandler:WTFMove(request.completionHandler)]); > > > > This looks like a use-after-move of request.completionHandler ? > > Indeed, request.completionHandler should be replaced with decisionHandler Could one of you please follow-up? I am busy with GPUProcess stuff :/
Reopening to attach new patch.
Created attachment 426468 [details] Patch
Comment on attachment 426468 [details] Patch Is it possible to API test? Clearly our current tests did not catch this.
I think it could be done with some creative use of ClassMethodSwizzler
Comment on attachment 426468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426468&action=review > Source/WebKit/UIProcess/ios/WKGeolocationProviderIOS.mm:200 > - auto policyListener = adoptNS([[WKWebAllowDenyPolicyListener alloc] initWithCompletionHandler:WTFMove(request.completionHandler)]); > + auto policyListener = adoptNS([[WKWebAllowDenyPolicyListener alloc] initWithCompletionHandler:WTFMove(decisionHandler)]); This does more than just fix a use-after-move. It also adds a call to the geolocationAuthorizationGranted method, which I hope is also a progression. Is there a way we can construct a test that is sensitive to both of these?
Created attachment 426559 [details] Patch
Committed r276311 (236793@main): <https://commits.webkit.org/236793@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426559 [details].