Bug 224045 - Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI
Summary: Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-01 06:09 PDT by youenn fablet
Modified: 2021-04-20 10:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.46 KB, patch)
2021-04-01 06:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2021-04-19 13:10 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (7.80 KB, patch)
2021-04-20 09:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].