Bug 224045 – Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[youenn fablet]

Pass the main frame URL to requestGeolocationAuthorizationForURL delegate SPI

[youenn fablet]

Created attachment 424889 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76104034>

[Alex Christensen]

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?

[youenn fablet]

(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.

[EWS]

Committed r275407: <https://commits.webkit.org/r275407>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424889 [details].

[Chris Dumez]

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 ?

[Alex Christensen]

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

[Chris Dumez]

(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 :/

[Alex Christensen]

Reopening to attach new patch.

[Alex Christensen]

Created attachment 426468 [details]
Patch

[Chris Dumez]

Comment on attachment 426468 [details]
Patch

Is it possible to API test? Clearly our current tests did not catch this.

[Alex Christensen]

I think it could be done with some creative use of ClassMethodSwizzler

[Darin Adler]

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?

[Alex Christensen]

Created attachment 426559 [details]
Patch

[EWS]

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].