Bug 103540 - Use GeolocationController's last geoposition as cached position.
Summary: Use GeolocationController's last geoposition as cached position.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Knottenbelt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-28 10:42 PST by John Knottenbelt
Modified: 2012-12-03 04:05 PST (History)
3 users (show)

See Also:


Attachments
Patch (9.21 KB, patch)
2012-11-28 10:47 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (8.86 KB, patch)
2012-11-29 08:53 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2012-11-28 10:42:58 PST
Use GeolocationController's last geoposition as cached position.
Comment 1 John Knottenbelt 2012-11-28 10:47:58 PST
Created attachment 176520 [details]
Patch
Comment 2 John Knottenbelt 2012-11-28 10:50:13 PST
The page's GeolocationController mediates access to the GeolocationClient for multiple frames' Geolocation instances.  This patch changes the position cache to be on the GeolocationController rather than on the Geolocation instance.

This fixes a bug where if one frame has added a watch and has received a position, then a request for a cached position from a second frame does not succeed because the Geolocation instance in the second frame's position cache hasn't received the position update that went to the first frame.
Comment 3 Benjamin Poulain 2012-11-28 12:39:35 PST
(In reply to comment #2)
> The page's GeolocationController mediates access to the GeolocationClient for multiple frames' Geolocation instances.  This patch changes the position cache to be on the GeolocationController rather than on the Geolocation instance.

Yes, you are right.

The cached position could be even higher in the stack actually. Moving it towards GeolocationController sounds like a step in the right direction.

> This fixes a bug where if one frame has added a watch and has received a position, then a request for a cached position from a second frame does not succeed because the Geolocation instance in the second frame's position cache hasn't received the position update that went to the first frame.

Note that this does not necessarily have to be true. If I remember correctly, the spec does not mandate the scope of the cache. What you describe seems like a desirable behavior though.

I'll review your patch later, I need to finish something.
Comment 4 Benjamin Poulain 2012-11-28 13:43:53 PST
Comment on attachment 176520 [details]
Patch

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

The patch looks good, the test seems well designed for what we want to cover.

One extra concern is privacy with the "leaking" of information through the page.

For example, by having a frame with Google Maps, you can find if the user has authorized Google Maps to access location.
I think this is not blocking because finding the same information through timing attacks is just as easy. Moreover, that is a pretty poor leak if the authorization client is implemented properly.

> Source/WebCore/ChangeLog:9
> +        GeolocationClient for multiple frames' Geolocation instances.  This

Two space after the period.

> LayoutTests/fast/dom/Geolocation/resources/cached-position-while-watching-inner.html:6
> +            'success':  true,

Double space after the colon.

> LayoutTests/fast/dom/Geolocation/resources/cached-position-while-watching-inner.html:8
> +            'message': 'Received cached position lat: ' + position.coords.latitude + ', long: ' +
> +                       position.coords.longitude

Coding style: position.coords.longitude should be on the same line, or the "+" should be on the second line.

> LayoutTests/fast/dom/Geolocation/script-tests/cached-position-while-watching.js:2
> +description('Tests that a cached position can be obtained in one frame after another frame has created a geolocation watch.');
> +

Maybe inline "cached-position-while-watching.js" in "cached-position-while-watching.html"? It is just my personal preference, your call.

> LayoutTests/fast/dom/Geolocation/script-tests/cached-position-while-watching.js:17
> +navigator.geolocation.watchPosition(
> +    function() {

You could use getCurrentPosition(). That would avoid isFirstPosition and make the timing more interesting for the test.
Comment 5 John Knottenbelt 2012-11-29 08:37:01 PST
Comment on attachment 176520 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        GeolocationClient for multiple frames' Geolocation instances.  This
> 
> Two space after the period.

Done.

>> LayoutTests/fast/dom/Geolocation/resources/cached-position-while-watching-inner.html:6
>> +            'success':  true,
> 
> Double space after the colon.

Done.

>> LayoutTests/fast/dom/Geolocation/resources/cached-position-while-watching-inner.html:8
>> +                       position.coords.longitude
> 
> Coding style: position.coords.longitude should be on the same line, or the "+" should be on the second line.

Thanks, I put it all on one line.

>> LayoutTests/fast/dom/Geolocation/script-tests/cached-position-while-watching.js:2
>> +
> 
> Maybe inline "cached-position-while-watching.js" in "cached-position-while-watching.html"? It is just my personal preference, your call.

I had done that originally, but I changed it to use a separate js for consistency with the other tests in fast/dom/Geolocation which also use the test framework (../resources/js-test-pre.js, ../resources/js-test-post.js). I prefer to keep it as is, if it's ok with you.

>> LayoutTests/fast/dom/Geolocation/script-tests/cached-position-while-watching.js:17
>> +    function() {
> 
> You could use getCurrentPosition(). That would avoid isFirstPosition and make the timing more interesting for the test.

You're right, the watch is unnecessary. I'll change it to use getCurrentPosition.
Comment 6 John Knottenbelt 2012-11-29 08:37:37 PST
Thanks for reviewing, Benjamin! I'll upload an improved version shortly.
Comment 7 John Knottenbelt 2012-11-29 08:53:17 PST
Created attachment 176744 [details]
Patch
Comment 8 Benjamin Poulain 2012-11-29 14:02:17 PST
> > Maybe inline "cached-position-while-watching.js" in "cached-position-while-watching.html"? It is just my personal preference, your call.
> 
> I had done that originally, but I changed it to use a separate js for consistency with the other tests in fast/dom/Geolocation which also use the test framework (../resources/js-test-pre.js, ../resources/js-test-post.js). I prefer to keep it as is, if it's ok with you.

There used to be a policy regarding that, which is why most tests in Geolocation use that style.
As far as I know, nobody enforce that anymore. Some recent Geolocation tests have the JavaScript in the html file.

In the end, I think both style of tests are useful since both are used on the Web.
Comment 9 WebKit Review Bot 2012-11-29 14:19:23 PST
Comment on attachment 176744 [details]
Patch

Clearing flags on attachment: 176744

Committed r136164: <http://trac.webkit.org/changeset/136164>
Comment 10 WebKit Review Bot 2012-11-29 14:19:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 János Badics 2012-12-03 04:05:30 PST
After r136164, fast/dom/Geolocation/cached-position-iframe.html fails with timeout on Qt-WK2. Detailed information can be found at bug 103876:

I will skip this test until the proper fix.