Bug 38323 – Prevents setLastPosition() before startUpdating() being called.

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

[Marcus Bulach]

Prevents setLastPosition() before startUpdating() being called.

[Marcus Bulach]

Created attachment 54693 [details]
Patch

[Jeremy Orlow]

Comment on attachment 54693 [details]
Patch

> diff --git a/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp b/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> index 07f09da30cd97975622fe13fbd867496a51a9e88..9d755a15d4f7892b81a4e78638e9dbae25c181b4 100644
> --- a/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> +++ b/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> @@ -85,6 +85,7 @@ private:
>      // GeolocationServiceChromium owns us, we only have a pointer back to it.
>      GeolocationServiceChromium* m_GeolocationServiceChromium;
>      int m_bridgeId;
> +    bool m_startUpdatingRequested;
>  };
>  
>  GeolocationServiceBridge* createGeolocationServiceBridgeImpl(GeolocationServiceChromium* geolocationServiceChromium)
> @@ -93,7 +94,8 @@ GeolocationServiceBridge* createGeolocationServiceBridgeImpl(GeolocationServiceC
>  }
>  
>  WebGeolocationServiceBridgeImpl::WebGeolocationServiceBridgeImpl(GeolocationServiceChromium* geolocationServiceChromium)
> -    : m_GeolocationServiceChromium(geolocationServiceChromium)
> +    : m_GeolocationServiceChromium(geolocationServiceChromium),

comma on next line

[Marcus Bulach]

Created attachment 54801 [details]
Patch

[Marcus Bulach]

(In reply to comment #2)
> (From update of attachment 54693 [details])
> > diff --git a/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp b/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> > index 07f09da30cd97975622fe13fbd867496a51a9e88..9d755a15d4f7892b81a4e78638e9dbae25c181b4 100644
> > --- a/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> > +++ b/WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> > @@ -85,6 +85,7 @@ private:
> >      // GeolocationServiceChromium owns us, we only have a pointer back to it.
> >      GeolocationServiceChromium* m_GeolocationServiceChromium;
> >      int m_bridgeId;
> > +    bool m_startUpdatingRequested;
> >  };
> >  
> >  GeolocationServiceBridge* createGeolocationServiceBridgeImpl(GeolocationServiceChromium* geolocationServiceChromium)
> > @@ -93,7 +94,8 @@ GeolocationServiceBridge* createGeolocationServiceBridgeImpl(GeolocationServiceC
> >  }
> >  
> >  WebGeolocationServiceBridgeImpl::WebGeolocationServiceBridgeImpl(GeolocationServiceChromium* geolocationServiceChromium)
> > -    : m_GeolocationServiceChromium(geolocationServiceChromium)
> > +    : m_GeolocationServiceChromium(geolocationServiceChromium),
> 
> comma on next line

done.

[Jeremy Orlow]

Steve, please do the review.

[Steve Block]

Comment on attachment 54801 [details]
Patch

> +        This prevents a page requesting permission when it has just accessed navigator.geolocation (without calling navigator.geolocation.getGeolocationPosition).
Did you mean navigator.geolocation.getCurrentPosition/watchPosition?

> +    m_startUpdatingRequested = true;
Don't you also need to set this to false in stopUpdating() to prevent superfluous permission requests after all location requests have stopped?

If a call to navigator.getCurrentPosition/watchPosition is successful, then permission must have been granted by the time stopUpdating() is called, so there's no problem. However, if the service returns an error, or JavaScript cancels the location request before the service returns anything, then there will be no location requests in progress and no permission request will have been made. With the current patch, m_startUpdatingRequested will still be true so subsequent calls to setLastPosition() due to activity in other frames will wrongly trigger a permission request.

Or does the browser process already have logic to prevent this?

Maybe it's better to modify the browser process to only broadcast location information to bridges that have called startUpdating() but not stopUpdating(), as you suggest in the Chromium bug?

[Marcus Bulach]

(In reply to comment #6)
> (From update of attachment 54801 [details])
> > +        This prevents a page requesting permission when it has just accessed navigator.geolocation (without calling navigator.geolocation.getGeolocationPosition).
> Did you mean navigator.geolocation.getCurrentPosition/watchPosition?

duh! :) fixed.

> 
> > +    m_startUpdatingRequested = true;
> Don't you also need to set this to false in stopUpdating() to prevent
> superfluous permission requests after all location requests have stopped?
> 
> If a call to navigator.getCurrentPosition/watchPosition is successful, then
> permission must have been granted by the time stopUpdating() is called, so
> there's no problem. However, if the service returns an error, or JavaScript
> cancels the location request before the service returns anything, then there
> will be no location requests in progress and no permission request will have
> been made. With the current patch, m_startUpdatingRequested will still be true
> so subsequent calls to setLastPosition() due to activity in other frames will
> wrongly trigger a permission request.
> 
> Or does the browser process already have logic to prevent this?
> 
> Maybe it's better to modify the browser process to only broadcast location
> information to bridges that have called startUpdating() but not stopUpdating(),
> as you suggest in the Chromium bug?

that's a *very* good point, thanks for raising it! I didn't realize startUpdating / stopUpdating weren't balanced, so the current implementation has a bug as it dettaches the bridge unconditionally on "stopUpdating"...
I think I can solve both bugs here rather than in the browser side, let me try a different change and will update soon.

thanks again for raising this!

[Marcus Bulach]

Created attachment 55018 [details]
Patch

[Marcus Bulach]

Hi Steve,

thanks again for your review! comments inline:

(In reply to comment #6)
> (From update of attachment 54801 [details])
> > +        This prevents a page requesting permission when it has just accessed navigator.geolocation (without calling navigator.geolocation.getGeolocationPosition).
> Did you mean navigator.geolocation.getCurrentPosition/watchPosition?
> 
> > +    m_startUpdatingRequested = true;
> Don't you also need to set this to false in stopUpdating() to prevent
> superfluous permission requests after all location requests have stopped?

thanks for pointing this out: there was actually another issue with stopUpdating detaching the bridge.
fixed now by using a counter around start / stop updating.

> If a call to navigator.getCurrentPosition/watchPosition is successful, then
> permission must have been granted by the time stopUpdating() is called, so
> there's no problem. However, if the service returns an error, or JavaScript
> cancels the location request before the service returns anything, then there
> will be no location requests in progress and no permission request will have
> been made. With the current patch, m_startUpdatingRequested will still be true
> so subsequent calls to setLastPosition() due to activity in other frames will
> wrongly trigger a permission request.

yep, I guess the new patch addresses the javascript cancelling the request.
as for the service throwing error condition: if i understood the flow correctly it will eventually call stopUpdating, right? so the counter will be balanced?

> 
> Or does the browser process already have logic to prevent this?

nope, the browser doesn't know about this.
speaking of which, I added a browser test on the other side:
http://codereview.chromium.org/1950001/show

it fails without this patch, and obviously passes with.. :)

> 
> Maybe it's better to modify the browser process to only broadcast location
> information to bridges that have called startUpdating() but not stopUpdating(),
> as you suggest in the Chromium bug?

I think adding this logic up in the browser process would further complicate things. the issue was that the bridge was being attached too early, so I hope this patch now fixes it (by attaching it later), and also the other issue you raised about start / stop. 
from the browser point of view, a bridge now is only attached when it has a pending request.

would you please take another look?

thanks!

[Steve Block]

> that's a *very* good point, thanks for raising it! I didn't realize startUpdating / stopUpdating weren't balanced,
> so the current implementation has a bug as it dettaches the bridge unconditionally on "stopUpdating"...
I don't think that's a bug. Calls to startUpdating() and stopUpdating() from the Geolocation object are intentionally unbalanced. startUpdating() is called each time a new request (getCurrentPosition or watchPosition) is started. lastPosition() is only called when the last request is complete. See Geolocation::hasListeners().

FWIW, this is something I'd like to change. Currently, the fact that stopUpdating() is called only once means that if a request is made with enableHighAccuracy=true, the GeolocationService will always use enableHighAccuracy=true for all future requests within the lifetime of the Geolocation object.

> as for the service throwing error condition: if i understood the flow correctly it will eventually call stopUpdating, right?
Yes, that's right. If the call to startUpdating() fails, the notifier will call back to Geolocation::fatalErrorOccurred() which will call stopUpdating() if there are no longer any requests in progress.

[Marcus Bulach]

Created attachment 55564 [details]
Patch

[Marcus Bulach]

thanks Steve! comments inline, another look please?

(In reply to comment #10)
> > that's a *very* good point, thanks for raising it! I didn't realize startUpdating / stopUpdating weren't balanced,
> > so the current implementation has a bug as it dettaches the bridge unconditionally on "stopUpdating"...
> I don't think that's a bug. Calls to startUpdating() and stopUpdating() from the Geolocation object are intentionally unbalanced. startUpdating() is called each time a new request (getCurrentPosition or watchPosition) is started. lastPosition() is only called when the last request is complete. See Geolocation::hasListeners().

I think I finally understood the flow, thanks for the clarification!
It turns out it's simpler than the original patches I proposed.
The original problem was that the bridge was being attached on the ctor, rather than on startUpdating, because requestPermission could be called without a corresponding startUpdating.

I added a new method "attachBridgeIfNeeded()", which seems to make things simpler and removed the flag / refcounting / guards around start/stop updating.

please, let me know if this makes sense.

> FWIW, this is something I'd like to change. Currently, the fact that stopUpdating() is called only once means that if a request is made with enableHighAccuracy=true, the GeolocationService will always use enableHighAccuracy=true for all future requests within the lifetime of the Geolocation object.
> 
> > as for the service throwing error condition: if i understood the flow correctly it will eventually call stopUpdating, right?
> Yes, that's right. If the call to startUpdating() fails, the notifier will call back to Geolocation::fatalErrorOccurred() which will call stopUpdating() if there are no longer any requests in progress.

ok, thanks! with the latest patch, stopUpdating remains unchanged.

[WebKit Commit Bot]

Comment on attachment 55564 [details]
Patch

Clearing flags on attachment: 55564

Committed r59215: <http://trac.webkit.org/changeset/59215>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.