Fixes a Geolocation bug with cached positions.

In the case where a watch request returns a cached position, we must clear the cached position to prevent it from being used repeatedly in the case where the watch timer later fires.

Also cleans up some code and adds some comments.

This will be upstreamed to webkit.org in https://bugs.webkit.org/show_bug.cgi?id=30676

Change-Id: I4968731453c3dfd34a8d3466fdaee91d4c4158be

2 files changed, 14 insertions, 9 deletions

diff --git a/WebCore/page/Geolocation.cpp b/WebCore/page/Geolocation.cpp
index 8dfe19c..5fbad47 100644
--- a/WebCore/page/Geolocation.cpp
+++ b/WebCore/page/Geolocation.cpp
@@ -101,6 +101,9 @@ void Geolocation::GeoNotifier::timerFired(Timer<GeoNotifier>*)
 
     if (m_cachedPosition) {
         m_successCallback->handleEvent(m_cachedPosition.get());
+        // Clear the cached position in case this is a watch request, which
+        // will continue to run.
+        m_cachedPosition = 0;
         geolocation->requestReturnedCachedPosition(this);
         return;
     }
@@ -403,16 +406,13 @@ void Geolocation::requestTimedOut(GeoNotifier* notifier)
 void Geolocation::requestReturnedCachedPosition(GeoNotifier* notifier)
 {
     // If this is a one-shot request, stop it.
-    if (m_oneShots.contains(notifier)) {
-        m_oneShots.remove(notifier);
-        if (!hasListeners())
-            m_service->stopUpdating();
-        return;
-    }
+    m_oneShots.remove(notifier);
+    if (!hasListeners())
+        m_service->stopUpdating();
 
     // Otherwise, if the watch still exists, start the service to get updates.
     if (m_watchers.contains(notifier)) {
-        if (m_service->startUpdating(notifier->m_options.get()))
+        if (notifier->hasZeroTimeout() || m_service->startUpdating(notifier->m_options.get()))
             notifier->startTimerIfNeeded();
         else
             notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, "Failed to start Geolocation service"));
@@ -471,8 +471,10 @@ void Geolocation::setIsAllowed(bool allowed)
         makeSuccessCallbacks();
     else {
         GeoNotifierSet::const_iterator end = m_requestsAwaitingCachedPosition.end();
-        for (GeoNotifierSet::const_iterator iter = m_requestsAwaitingCachedPosition.begin(); iter != end; ++iter)
+        for (GeoNotifierSet::const_iterator iter = m_requestsAwaitingCachedPosition.begin(); iter != end; ++iter) {
+            ASSERT(m_cachedPositionManager->cachedPosition());
             (*iter)->setCachedPosition(m_cachedPositionManager->cachedPosition());
+        }
     }
     m_requestsAwaitingCachedPosition.clear();
 }
@@ -622,6 +624,9 @@ void Geolocation::geolocationServiceErrorOccurred(GeolocationService* service)
 {
     ASSERT(service->lastError());
     
+    // Note that we do not stop timers here. For one-shots, the request is
+    // cleared in handleError. For watchers, the spec requires that the timer is
+    // not cleared.
     handleError(service->lastError());
 }
 
diff --git a/WebCore/platform/android/GeolocationServiceAndroid.cpp b/WebCore/platform/android/GeolocationServiceAndroid.cpp
index d44b3f0..43a8f5f 100644
--- a/WebCore/platform/android/GeolocationServiceAndroid.cpp
+++ b/WebCore/platform/android/GeolocationServiceAndroid.cpp
@@ -92,7 +92,7 @@ bool GeolocationServiceAndroid::startUpdating(PositionOptions* options)
 void GeolocationServiceAndroid::stopUpdating()
 {
     // Called when the Geolocation object has no watches or one shots in
-    // progress.
+    // progress. This may be called repeatedly.
     m_javaBridge.clear();
     // Reset last position and error to make sure that we always try to get a
     // new position from the system service when a request is first made.