Bug 195996 – Geolocation request not complete when watch request was started in a different web process

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This can be easily reproduced using the web inspector to start the geolocation requests:

1.- Open a browser window and load any https page.
2.- Open the inspector and make a watch request from the console
  navigator.geolocation.watchPosition(function(p) { alert(p); });
3.- Allow the request and close the alert.
4.- Open a new browser window (not a tab, since we need both web view to be visible) and load a different https page
5.- Open the inspector and make a location request from the console
  navigator.geolocation.getCurrentPosition(function(p) { alert(p) })
6.- Allow the request

The alert is not shown in this case. This is because in WebGeolocationManagerProxy::startUpdating() we do nothing when the provider is already updating. We should reply with a DidChangePosition using the last known position, if available. If we are updating, but we still don't have a known position, the request will be completed when WebGeolocationManagerProxy::providerDidChangePosition() is called since it always notifies all web processes.

[Carlos Garcia Campos]

Created attachment 365338 [details]
Patch

[Carlos Garcia Campos]

Created attachment 365339 [details]
Patch

[Alex Christensen]

This can definitely be tested with an API test.

[Carlos Garcia Campos]

Created attachment 365542 [details]
Updated patch with unit test

[Chris Dumez]

Comment on attachment 365542 [details]
Updated patch with unit test

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

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:170
> +    encoder << lastGeolocationPosition;

Why do we need to add yet another WebProcessCreationParameters ? If the page want geolocation, they will ask for it and we'll ask the UIProcess.
I personally do not think it is worth adding a new WebProcessCreationParameter just for this.

> Source/WebKit/UIProcess/WebGeolocationManagerProxy.h:58
> +    Optional<WebCore::GeolocationPosition> lastPosition() const { return m_lastPosition; }

should return a const Optional<WebCore::GeolocationPosition>&

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 365542 [details]
> Updated patch with unit test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365542&action=review
> 
> > Source/WebKit/Shared/WebProcessCreationParameters.cpp:170
> > +    encoder << lastGeolocationPosition;
> 
> Why do we need to add yet another WebProcessCreationParameters ? If the page
> want geolocation, they will ask for it and we'll ask the UIProcess.
> I personally do not think it is worth adding a new
> WebProcessCreationParameter just for this.

If the geolocation request has a maximum age option, and the cached position is recent enough we avoid asking the UI process for the location and the cached one is used instead. I assumed we wanted this because there's a FIXME in WebGeolocationClient::lastPosition().

> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.h:58
> > +    Optional<WebCore::GeolocationPosition> lastPosition() const { return m_lastPosition; }
> 
> should return a const Optional<WebCore::GeolocationPosition>&

ok

[Carlos Garcia Campos]

Created attachment 365704 [details]
Patch

[EWS Watchlist]

Comment on attachment 365704 [details]
Patch

Attachment 365704 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11612034

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365714 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 365542 [details]
> > Updated patch with unit test
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=365542&action=review
> > 
> > > Source/WebKit/Shared/WebProcessCreationParameters.cpp:170
> > > +    encoder << lastGeolocationPosition;
> > 
> > Why do we need to add yet another WebProcessCreationParameters ? If the page
> > want geolocation, they will ask for it and we'll ask the UIProcess.
> > I personally do not think it is worth adding a new
> > WebProcessCreationParameter just for this.
> 
> If the geolocation request has a maximum age option, and the cached position
> is recent enough we avoid asking the UI process for the location and the
> cached one is used instead. I assumed we wanted this because there's a FIXME
> in WebGeolocationClient::lastPosition().

Well, I do not think it is worth adding a new WebProcessCreationParameter just to avoid an IPC that might happen.

[Chris Dumez]

Comment on attachment 365704 [details]
Patch

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

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:170
> +    encoder << lastGeolocationPosition;

I do not think we should do this.

[Alex Christensen]

Comment on attachment 365704 [details]
Patch

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

>> Source/WebKit/Shared/WebProcessCreationParameters.cpp:170
>> +    encoder << lastGeolocationPosition;
> 
> I do not think we should do this.

I agree.

[Ryosuke Niwa]

It's actually pretty important that the UI process only ever sends geolocation to which the user consented the location access. Otherwise, Spectre class of attacks could retrieve this information.

[Carlos Garcia Campos]

Created attachment 365809 [details]
Patch

[Alex Christensen]

Comment on attachment 365809 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/Geolocation.cpp:320
> +    std::string alertText;

Nit: I see no reason not to use WTF::String here.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #15)
> Comment on attachment 365809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365809&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/Geolocation.cpp:320
> > +    std::string alertText;
> 
> Nit: I see no reason not to use WTF::String here.

I'm using std::string only because of Util::toSTD() there isn't a Util::toWTF()

[Alex Christensen]

You're absolutely right.  WKSharedAPICast.h maps WKStringRef to API::String which has a string accessor, but that's not to be used outside of the WebKit project.  You could write a utility that uses WKStringGetUTF8CString and String::fromUTF8, or you could leave this patch as it is.  Either is fine by me, but we should eventually make a toWTF utility.

[Sam Weinig]

(In reply to Carlos Garcia Campos from comment #16)
> (In reply to Alex Christensen from comment #15)
> > Comment on attachment 365809 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=365809&action=review
> > 
> > > Tools/TestWebKitAPI/Tests/WebKit/Geolocation.cpp:320
> > > +    std::string alertText;
> > 
> > Nit: I see no reason not to use WTF::String here.
> 
> I'm using std::string only because of Util::toSTD() there isn't a
> Util::toWTF()

If memory serves me, we chose to use std::string in the unit tests because it tended to work well with the unit testing library.

[Carlos Garcia Campos]

Committed r243533: <https://trac.webkit.org/changeset/243533>

[Radar WebKit Bug Importer]

<rdar://problem/49323894>