Bug 224045

Summary: Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebKit APIAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, darin, eric.carlson, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description youenn fablet 2021-04-01 06:09:41 PDT
Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI
Comment 1 youenn fablet 2021-04-01 06:14:26 PDT
Created attachment 424889 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-04-01 08:09:20 PDT
<rdar://problem/76104034>
Comment 3 Alex Christensen 2021-04-01 12:14:28 PDT
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?
Comment 4 youenn fablet 2021-04-02 01:03:51 PDT
(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.
Comment 5 EWS 2021-04-02 01:07:33 PDT
Committed r275407: <https://commits.webkit.org/r275407>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424889 [details].
Comment 6 Chris Dumez 2021-04-19 13:06:08 PDT
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 7 Alex Christensen 2021-04-19 13:07:35 PDT
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
Comment 8 Chris Dumez 2021-04-19 13:08:46 PDT
(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 :/
Comment 9 Alex Christensen 2021-04-19 13:10:56 PDT
Reopening to attach new patch.
Comment 10 Alex Christensen 2021-04-19 13:10:58 PDT
Created attachment 426468 [details]
Patch
Comment 11 Chris Dumez 2021-04-19 13:12:01 PDT
Comment on attachment 426468 [details]
Patch

Is it possible to API test? Clearly our current tests did not catch this.
Comment 12 Alex Christensen 2021-04-19 13:14:02 PDT
I think it could be done with some creative use of ClassMethodSwizzler
Comment 13 Darin Adler 2021-04-19 15:43:18 PDT
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?
Comment 14 Alex Christensen 2021-04-20 09:47:47 PDT
Created attachment 426559 [details]
Patch
Comment 15 EWS 2021-04-20 10:19:09 PDT
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].