Bug 195996

Summary: Geolocation request not complete when watch request was started in a different web process
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, ews-watchlist, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Updated patch with unit test
none
Patch
achristensen: review-, ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Patch achristensen: review+

Description Carlos Garcia Campos 2019-03-20 05:07:37 PDT
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.
Comment 1 Carlos Garcia Campos 2019-03-20 05:14:24 PDT
Created attachment 365338 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-03-20 05:45:34 PDT
Created attachment 365339 [details]
Patch
Comment 3 Alex Christensen 2019-03-20 13:19:45 PDT
This can definitely be tested with an API test.
Comment 4 Carlos Garcia Campos 2019-03-21 06:29:55 PDT
Created attachment 365542 [details]
Updated patch with unit test
Comment 5 Chris Dumez 2019-03-21 08:53:55 PDT
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>&
Comment 6 Carlos Garcia Campos 2019-03-22 03:29:56 PDT
(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
Comment 7 Carlos Garcia Campos 2019-03-22 03:50:17 PDT
Created attachment 365704 [details]
Patch
Comment 8 EWS Watchlist 2019-03-22 05:06:41 PDT
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
Comment 9 EWS Watchlist 2019-03-22 05:06:43 PDT
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
Comment 10 Chris Dumez 2019-03-22 08:36:38 PDT
(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.
Comment 11 Chris Dumez 2019-03-22 08:36:50 PDT
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.
Comment 12 Alex Christensen 2019-03-22 09:53:45 PDT
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.
Comment 13 Ryosuke Niwa 2019-03-22 18:51:26 PDT
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.
Comment 14 Carlos Garcia Campos 2019-03-23 02:56:04 PDT
Created attachment 365809 [details]
Patch
Comment 15 Alex Christensen 2019-03-25 14:17:10 PDT
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.
Comment 16 Carlos Garcia Campos 2019-03-26 03:01:01 PDT
(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()
Comment 17 Alex Christensen 2019-03-26 14:26:18 PDT
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.
Comment 18 Sam Weinig 2019-03-26 17:48:18 PDT
(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.
Comment 19 Carlos Garcia Campos 2019-03-27 02:36:35 PDT
Committed r243533: <https://trac.webkit.org/changeset/243533>
Comment 20 Radar WebKit Bug Importer 2019-03-27 03:01:18 PDT
<rdar://problem/49323894>