Bug 103540 – Use GeolocationController's last geoposition as cached position.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Knottenbelt]

Use GeolocationController's last geoposition as cached position.

[John Knottenbelt]

Created attachment 176520 [details]
Patch

[John Knottenbelt]

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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

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.

[John Knottenbelt]

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.

[John Knottenbelt]

Thanks for reviewing, Benjamin! I'll upload an improved version shortly.

[John Knottenbelt]

Created attachment 176744 [details]
Patch

[Benjamin Poulain]

> > 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.

[WebKit Review Bot]

Comment on attachment 176744 [details]
Patch

Clearing flags on attachment: 176744

Committed r136164: <http://trac.webkit.org/changeset/136164>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[János Badics]

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.