Summary: | Geolocation request not complete when watch request was started in a different web process | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||
Component: | WebKit2 | Assignee: | 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
Carlos Garcia Campos
2019-03-20 05:07:37 PDT
Created attachment 365338 [details]
Patch
Created attachment 365339 [details]
Patch
This can definitely be tested with an API test. Created attachment 365542 [details]
Updated patch with unit test
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>& (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 Created attachment 365704 [details]
Patch
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 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
(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 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 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. 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. Created attachment 365809 [details]
Patch
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. (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() 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. (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. Committed r243533: <https://trac.webkit.org/changeset/243533> |