Bug 170362 – Add SPI for handling geolocation authorization requests

• Component: WebKit2
• Status: RESOLVED FIXED

[David Quesada]

rdar://problem/17508627

[David Quesada]

Created attachment 306015 [details]
Patch

[Build Bot]

Attachment 306015 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:188:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Quesada]

Comment on attachment 306015 [details]
Patch

>commit 8d19071e1f4ffcbe354b901238e104b85728994a
>Author: David Quesada <[e-mail]>
>Date:   Fri Mar 31 14:53:01 2017 -0700
>
>    Add SPI for handling geolocation authorization requests
>    https://bugs.webkit.org/show_bug.cgi?id=170362
>    rdar://problem/17508627
>    
>    Reviewed by NOBODY (OOPS!).
>    
>    * UIProcess/API/Cocoa/WKUIDelegatePrivate.h:
>    * UIProcess/ios/WKGeolocationProviderIOS.mm:
>    (-[WKGeolocationProviderIOS geolocationAuthorizationGranted]):
>
>diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
>index 9ca40daa58b..5b969402e17 100644
>--- a/Source/WebKit2/ChangeLog
>+++ b/Source/WebKit2/ChangeLog
>@@ -1,3 +1,15 @@
>[phone]  David Quesada  <[e-mail]>
>+
>+        Add SPI for handling geolocation authorization requests
>+        https://bugs.webkit.org/show_bug.cgi?id=170362
>+        rdar://problem/17508627
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UIProcess/API/Cocoa/WKUIDelegatePrivate.h:
>+        * UIProcess/ios/WKGeolocationProviderIOS.mm:
>+        (-[WKGeolocationProviderIOS geolocationAuthorizationGranted]):
>+
> 2017-03-31  Csaba Osztrogonác  <[e-mail]>
> 
>         Mac cmake buildfix after r214403
>diff --git a/Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h b/Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h
>index 05a1096fb8c..4e3614eaa69 100644
>--- a/Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h
>+++ b/Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h
>@@ -35,6 +35,7 @@
> @class UIItemProvider;
> @class UIScrollView;
> @class UIViewController;
>+@class WKFrameInfo;
> @class _WKContextMenuElementInfo;
> @class _WKActivatedElementInfo;
> @class _WKElementAction;
>@@ -72,6 +73,7 @@ struct UIEdgeInsets;
> - (NSArray *)_webView:(WKWebView *)webView actionsForElement:(_WKActivatedElementInfo *)element defaultActions:(NSArray<_WKElementAction *> *)defaultActions;
> - (void)_webView:(WKWebView *)webView didNotHandleTapAsClickAtPoint:(CGPoint)point;
> - (BOOL)_webView:(WKWebView *)webView shouldRequestGeolocationAuthorizationForURL:(NSURL *)url isMainFrame:(BOOL)isMainFrame mainFrameURL:(NSURL *)mainFrameURL;
>+- (void)_webView:(WKWebView *)webView requestGeolocationAuthorizationForURL:(NSURL *)url frame:(WKFrameInfo *)frame decisionHandler:(void (^)(BOOL authorized))decisionHandler WK_API_AVAILABLE(ios(WK_IOS_TBA));
> - (UIViewController *)_webView:(WKWebView *)webView previewViewControllerForURL:(NSURL *)url WK_API_AVAILABLE(ios(9.0));
> - (void)_webView:(WKWebView *)webView commitPreviewedViewController:(UIViewController *)previewedViewController WK_API_AVAILABLE(ios(9.0));
> - (void)_webView:(WKWebView *)webView willPreviewImageWithURL:(NSURL *)imageURL WK_API_AVAILABLE(ios(9.0));
>diff --git a/Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm b/Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm
>index 24781044816..839939d4c5a 100644
>--- a/Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm
>+++ b/Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm
>@@ -28,8 +28,11 @@
> 
> #if PLATFORM(IOS)
> 
>+#import "APIFrameInfo.h"
> #import "APISecurityOrigin.h"
>+#import "CompletionHandlerCallChecker.h"
> #import "GeolocationPermissionRequestProxy.h"
>+#import "WKFrameInfoInternal.h"
> #import "WKUIDelegatePrivate.h"
> #import "WKWebView.h"
> #import "WebGeolocationManagerProxy.h"
>@@ -38,6 +41,7 @@
> #import <WebCore/URL.h>
> #import <WebGeolocationPosition.h>
> #import <wtf/Assertions.h>
>+#import <wtf/BlockPtr.h>
> #import <wtf/HashSet.h>
> #import <wtf/PassRefPtr.h>
> #import <wtf/RefPtr.h>
>@@ -177,6 +181,22 @@ - (void)geolocationAuthorizationGranted
>         bool requiresUserAuthorization = true;
> 
>         id<WKUIDelegatePrivate> uiDelegate = static_cast<id <WKUIDelegatePrivate>>([request.view UIDelegate]);
>+        if ([uiDelegate respondsToSelector:@selector(_webView:requestGeolocationAuthorizationForURL:frame:decisionHandler:)]) {
>+            URL requestFrameURL(URL(), request.frame->url());
>+            RetainPtr<WKFrameInfo> frameInfo = wrapper(API::FrameInfo::create(*request.frame.get(), *request.origin.get()));
>+            RefPtr<CompletionHandlerCallChecker> checker = CompletionHandlerCallChecker::create(uiDelegate, @selector(_webView:requestGeolocationAuthorizationForURL:frame:decisionHandler:));
>+            [uiDelegate _webView:request.view.get() requestGeolocationAuthorizationForURL:requestFrameURL frame:frameInfo.get() decisionHandler:BlockPtr<void(BOOL)>::fromCallable([request, checker = WTFMove(checker)](BOOL authorized) {
>+                if (checker->completionHandlerHasBeenCalled())
>+                    return;
>+                if (authorized)
>+                    request.permissionRequest->allow();
>+                else
>+                    request.permissionRequest->deny();
>+                checker->didCallCompletionHandler();
>+            }).get()];
>+            return;
>+        }
>+
>         if ([uiDelegate respondsToSelector:@selector(_webView:shouldRequestGeolocationAuthorizationForURL:isMainFrame:mainFrameURL:)]) {
>             const WebFrameProxy* mainFrame = request.frame->page()->mainFrame();
>             bool isMainFrame = request.frame == mainFrame;

[Build Bot]

Comment on attachment 306015 [details]
Patch

Attachment 306015 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3451495

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-status.html

[Build Bot]

Created attachment 306025 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alex Christensen]

Comment on attachment 306015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306015&action=review

Test failure seems unrelated.  This looks good.  It seems like we had a synchronous callback and this adds an asynchronous one.
You should make an API test that opens a page that requests geolocation data and asynchronously call the decisionHandler with a callOnMainThread in the delegate callback that calls the decisionHandler.  Then we can do things like test the behavior if the delegate callback isn't there, test the behavior if we call the callback twice, make sure there are no use-after-free bugs on the asan bots, make sure the expected behavior happens in JavaScript when a request is accepted and denied, etc.

> Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:185
> +            URL requestFrameURL(URL(), request.frame->url());

Can't we just call the copy constructor here?

[David Quesada]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 306015 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306015&action=review
> 
> Test failure seems unrelated.  This looks good.  It seems like we had a
> synchronous callback and this adds an asynchronous one.

As far as I know, nothing uses the synchronous method (specifically, it is not being used for the purpose for which it was added), so I would like to get rid of it. But removing SPI probably isn't something I can just bundle in with another change.

> You should make an API test that opens a page that requests geolocation data
> and asynchronously call the decisionHandler with a callOnMainThread in the
> delegate callback that calls the decisionHandler.  Then we can do things
> like test the behavior if the delegate callback isn't there, test the
> behavior if we call the callback twice, make sure there are no
> use-after-free bugs on the asan bots, make sure the expected behavior
> happens in JavaScript when a request is accepted and denied, etc.

I can look into doing this. I wasn't sure API tests would work out-of-the-box for this, since we would possibly have to either (1) entitle TestWebKitAPI for location services access or (2) expose more about WKGeolocationProviderIOS and WebGeolocationCoreLocationProvider so the API tests can configure WKGeolocationProviderIOS to go through its motions without actually touching CoreLocation.

If we didn't make either of these changes, the API tests would fail or get stuck at the system location prompt when TestWebKitAPI tries to request location permission. If we only did (1), there may be flakiness when running the tests if geolocation isn’t available on the machine running the simulator and tests. (2) is more architectural work than I would have liked to do for this, but I’ll make these changes if this is the best approach. What are your thoughts?

> 
> > Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:185
> > +            URL requestFrameURL(URL(), request.frame->url());
> 
> Can't we just call the copy constructor here?

Sorry, I lazily copied this from the synchronous delegate call. That would make it just "URL requestFrameURL(request.frame->url());" , right?

[Alex Christensen]

(In reply to David Quesada from comment #7)
> (In reply to Alex Christensen from comment #6)
> > Comment on attachment 306015 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306015&action=review
> > 
> > Test failure seems unrelated.  This looks good.  It seems like we had a
> > synchronous callback and this adds an asynchronous one.
> 
> As far as I know, nothing uses the synchronous method (specifically, it is
> not being used for the purpose for which it was added), so I would like to
> get rid of it. But removing SPI probably isn't something I can just bundle
> in with another change.
If we're sure nothing uses it, we can remove the calls to it and mark it as dead somehow.
> 
> > You should make an API test that opens a page that requests geolocation data
> > and asynchronously call the decisionHandler with a callOnMainThread in the
> > delegate callback that calls the decisionHandler.  Then we can do things
> > like test the behavior if the delegate callback isn't there, test the
> > behavior if we call the callback twice, make sure there are no
> > use-after-free bugs on the asan bots, make sure the expected behavior
> > happens in JavaScript when a request is accepted and denied, etc.
> 
> I can look into doing this. I wasn't sure API tests would work
> out-of-the-box for this, since we would possibly have to either (1) entitle
> TestWebKitAPI for location services access or (2) expose more about
> WKGeolocationProviderIOS and WebGeolocationCoreLocationProvider so the API
> tests can configure WKGeolocationProviderIOS to go through its motions
> without actually touching CoreLocation.
> 
> If we didn't make either of these changes, the API tests would fail or get
> stuck at the system location prompt when TestWebKitAPI tries to request
> location permission. If we only did (1), there may be flakiness when running
> the tests if geolocation isn’t available on the machine running the
> simulator and tests. (2) is more architectural work than I would have liked
> to do for this, but I’ll make these changes if this is the best approach.
> What are your thoughts?
DumpRenderTree has a MockGeolocationProvider.  We should use something similar, or even move that to WebCoreTestSupport.
> 
> > 
> > > Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:185
> > > +            URL requestFrameURL(URL(), request.frame->url());
> > 
> > Can't we just call the copy constructor here?
> 
> Sorry, I lazily copied this from the synchronous delegate call. That would
> make it just "URL requestFrameURL(request.frame->url());" , right?
auto requestFrameURL = request.frame->url();
I guess that's the operator=, not the copy constructor.

[Alex Christensen]

FYI, there are API tests for geolocation in Tools/TestWebKitAPI/Tests/WebKit2/Geolocation.cpp but none for the cocoa api yet.  Yours might be able to be conceptually similar to those.

[David Quesada]

(In reply to Alex Christensen from comment #9)
> FYI, there are API tests for geolocation in
> Tools/TestWebKitAPI/Tests/WebKit2/Geolocation.cpp but none for the cocoa api
> yet.  Yours might be able to be conceptually similar to those.

Yep, I saw those when I first looked into writing tests for this, but like you said, they don't use the Cocoa API. Right now I'm playing around with adding a "geolocation provider" object on the process pool that (if non-nil) the WKGeolocationProviderIOS will use instead of the WebKit1 location provider class (WebGeolocationCoreLocationProvider). The Cocoa API test can provide a dummy geolocation provider, and will be similar to the Geolocation.cpp tests, just with a different API.

[David Quesada]

Created attachment 306761 [details]
Patch v2

[Alex Christensen]

Comment on attachment 306761 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=306761&action=review

> Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:78
> +static inline RefPtr<WebGeolocationPosition> kit(WebCore::GeolocationPosition *position)

Let's make this return a Ref, and make WebGeolocationPosition::create return a Ref instead of a PassRefPtr.  This will require dereferencing the newly allocated pointer, which we commonly do in WebKit.
return adoptRef(*new WebGeolocationPosition ...

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/Geolocation.mm:28
> +#if WK_API_ENABLED

It looks like this API test will run on Mac, and I'm not sure what will happen if it does.  Maybe we could make this test run on Mac, maybe we should make it just run on iOS.

[David Quesada]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 306761 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306761&action=review
> 
> > Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:78
> > +static inline RefPtr<WebGeolocationPosition> kit(WebCore::GeolocationPosition *position)
> 
> Let's make this return a Ref, and make WebGeolocationPosition::create return
> a Ref instead of a PassRefPtr.  This will require dereferencing the newly
> allocated pointer, which we commonly do in WebKit.
> return adoptRef(*new WebGeolocationPosition ...

Done.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/Geolocation.mm:28
> > +#if WK_API_ENABLED
> 
> It looks like this API test will run on Mac, and I'm not sure what will
> happen if it does.  Maybe we could make this test run on Mac, maybe we
> should make it just run on iOS.

I'll make the tests only run on iOS. The new delegate method and the ability to swap out a location provider are only compiled on iOS, so it doesn't make sense to try to test them on Mac.

[David Quesada]

Created attachment 306764 [details]
Patch v2.1

Addressed Alex's feedback - changed WebGeolocationPosition::create to a Ref<>, compiled out the API tests on Mac, and (attempted to) fix EWS errors.

[Alex Christensen]

Comment on attachment 306764 [details]
Patch v2.1

View in context: https://bugs.webkit.org/attachment.cgi?id=306764&action=review

LGTM except the memory leak.

> Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:92
> +    RetainPtr<id<_WKGeolocationCoreLocationProvider>> _coreLocationProvider;

I'm not sure if the id is necessary here.  If it is, we seem to have a space after id elsewhere in WebKit.

> Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:269
> +    _provider = [[WebGeolocationCoreLocationProvider alloc] initWithListener:self];

We need an adoptNS or this is a memory leak.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/Geolocation.mm:42
> +SOFT_LINK_FRAMEWORK(CoreLocation)
> +SOFT_LINK_CLASS(CoreLocation, CLLocation)

Is there a reason we're soft linking?

[David Quesada]

(In reply to Alex Christensen from comment #15)
> Comment on attachment 306764 [details]
> Patch v2.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306764&action=review
> 
> LGTM except the memory leak.
> 
> > Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:92
> > +    RetainPtr<id<_WKGeolocationCoreLocationProvider>> _coreLocationProvider;
> 
> I'm not sure if the id is necessary here.  If it is, we seem to have a space
> after id elsewhere in WebKit.

The id is necessary since _WKGeolocationCoreLocationProvider is a protocol type, not a the name of a class, which would be usable without id<>.

> 
> > Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:269
> > +    _provider = [[WebGeolocationCoreLocationProvider alloc] initWithListener:self];
> 
> We need an adoptNS or this is a memory leak.

Will fix.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/Geolocation.mm:42
> > +SOFT_LINK_FRAMEWORK(CoreLocation)
> > +SOFT_LINK_CLASS(CoreLocation, CLLocation)
> 
> Is there a reason we're soft linking?

The WebKit code that uses CoreLocation (WebGeolocationCoreLocationProvider) soft links it, so I also soft linked CoreLocation in the API tests. I'll change this to regular linking since there's not much of a point in soft linking in TestWebKitAPI.

[David Quesada]

Created attachment 306828 [details]
Patch v2.2

- Use adoptNS() when creating the WebGeolocationCoreLocationProvider to prevent a memory leak.
- Hard linked CoreLocation.framework in TestWebKitAPI

[WebKit Commit Bot]

Comment on attachment 306828 [details]
Patch v2.2

Clearing flags on attachment: 306828

Committed r215246: <http://trac.webkit.org/changeset/215246>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.