Bug 596481 - Geolocation sometimes hangs without returning result or running any callbacks. r=alon, a=blocking-2.0

--HG--
extra : rebase_source : 8ed6bf1b9223b15a6961ecdc71203a5c48ec64d5

dom/src/geolocation/nsGeolocation.cpp

@@ -242,6 +242,7 @@ nsGeolocationRequest::nsGeolocationRequest(nsGeolocation* aLocator,
                                            nsIDOMGeoPositionOptions* aOptions)
   : mAllowed(PR_FALSE),
     mCleared(PR_FALSE),
+    mIsFirstUpdate(PR_TRUE),
     mCallback(aCallback),
     mErrorCallback(aErrorCallback),
     mOptions(aOptions),
@@ -418,6 +419,10 @@ nsGeolocationRequest::SetTimeoutTimer()
 void
 nsGeolocationRequest::MarkCleared()
 {
+  if (mTimeoutTimer) {
+    mTimeoutTimer->Cancel();
+    mTimeoutTimer = nsnull;
+  }
   mCleared = PR_TRUE;
 }
 
@@ -452,6 +457,23 @@ nsGeolocationRequest::SendLocation(nsIDOMGeoPosition* aPosition)
   SetTimeoutTimer();
 }
 
+void
+nsGeolocationRequest::Update(nsIDOMGeoPosition* aPosition, PRBool isBetter)
+{
+  // Only dispatch callbacks if this is the first position for this request, or
+  // if the accuracy is as good or improving.
+  //
+  // This ensures that all listeners get at least one position callback, particularly
+  // in the case when newly detected positions are all less accurate than the cached one.
+  //
+  // Fixes bug 596481
+  if (mIsFirstUpdate || isBetter) {
+    mIsFirstUpdate = PR_FALSE;
+    nsCOMPtr<nsIRunnable> ev  = new RequestSendLocationEvent(aPosition, this);
+    NS_DispatchToMainThread(ev);
+  }
+}
+
 void
 nsGeolocationRequest::Shutdown()
 {
@@ -636,13 +658,13 @@ nsGeolocationService::Update(nsIDOMGeoPosition *aSomewhere)
   // here we have to determine this aSomewhere is a "better"
   // position than any previously recv'ed.
 
-  if (!IsBetterPosition(aSomewhere))
-    return NS_OK;
-
-  SetCachedPosition(aSomewhere);
+  PRBool isBetter = IsBetterPosition(aSomewhere);
+  if (isBetter) {
+    SetCachedPosition(aSomewhere);
+  }
 
   for (PRUint32 i = 0; i< mGeolocators.Length(); i++)
-    mGeolocators[i]->Update(aSomewhere);
+    mGeolocators[i]->Update(aSomewhere, isBetter);
   return NS_OK;
 }
 
@@ -659,7 +681,7 @@ nsGeolocationService::IsBetterPosition(nsIDOMGeoPosition *aSomewhere)
   nsCOMPtr<nsIDOMGeoPosition> lastPosition = geoService->GetCachedPosition();
   if (!lastPosition)
     return PR_TRUE;
-  
+
   nsresult rv;
   DOMTimeStamp oldTime;
   rv = lastPosition->GetTimestamp(&oldTime);
@@ -974,22 +996,19 @@ nsGeolocation::RemoveRequest(nsGeolocationRequest* aRequest)
 }
 
 void
-nsGeolocation::Update(nsIDOMGeoPosition *aSomewhere)
+nsGeolocation::Update(nsIDOMGeoPosition *aSomewhere, PRBool isBetter)
 {
   if (!WindowOwnerStillExists())
     return Shutdown();
 
   for (PRUint32 i = 0; i< mPendingCallbacks.Length(); i++) {
-    nsCOMPtr<nsIRunnable> ev  = new RequestSendLocationEvent(aSomewhere,
-                                                             mPendingCallbacks[i]);
-    NS_DispatchToMainThread(ev);
+    mPendingCallbacks[i]->Update(aSomewhere, isBetter);
   }
   mPendingCallbacks.Clear();
 
   // notify everyone that is watching
   for (PRUint32 i = 0; i< mWatchingCallbacks.Length(); i++) {
-    nsCOMPtr<nsIRunnable> ev  = new RequestSendLocationEvent(aSomewhere, mWatchingCallbacks[i]);
-    NS_DispatchToMainThread(ev);
+    mWatchingCallbacks[i]->Update(aSomewhere, isBetter);
   }
 }
 

dom/src/geolocation/nsGeolocation.h

@@ -95,6 +95,10 @@ class nsGeolocationRequest
   nsresult Init();
   void Shutdown();
 
+  // Called by the geolocation device to notify that a location has changed.
+  // isBetter: the accuracy is as good or better than the previous position. 
+  void Update(nsIDOMGeoPosition* aPosition, PRBool isBetter);
+
   void SendLocation(nsIDOMGeoPosition* location);
   void MarkCleared();
   PRBool IsActive() {return !mCleared;}
@@ -113,6 +117,7 @@ class nsGeolocationRequest
   void NotifyError(PRInt16 errorCode);
   PRPackedBool mAllowed;
   PRPackedBool mCleared;
+  PRPackedBool mIsFirstUpdate;
 
   nsCOMPtr<nsITimer> mTimeoutTimer;
   nsCOMPtr<nsIDOMGeoPositionCallback> mCallback;
@@ -201,7 +206,8 @@ public:
   nsresult Init(nsIDOMWindow* contentDom=nsnull);
 
   // Called by the geolocation device to notify that a location has changed.
-  void Update(nsIDOMGeoPosition* aPosition);
+  // isBetter: the accuracy is as good or better than the previous position. 
+  void Update(nsIDOMGeoPosition* aPosition, PRBool isBetter);
 
   // Returns true if any of the callbacks are repeating
   PRBool HasActiveCallbacks();

dom/tests/mochitest/geolocation/Makefile.in

@@ -58,7 +58,8 @@ _TEST_FILES	= \
 		test_manyWindows.html \
 		test_optional_api_params.html \
 		test_windowClose.html \
-	        test_timerRestartWatch.html \
+		test_timerRestartWatch.html \
+		test_worseAccuracyDoesNotBlockCallback.html \
 		geolocation.html \
 		geolocation_common.js  \
 		network_geolocation.sjs \

dom/tests/mochitest/geolocation/geolocation_common.js

@@ -50,6 +50,13 @@ function stop_geolocationProvider()
   sleep(1000);
 }
 
+function worse_geolocationProvider()
+{
+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
+  prefs.setCharPref("geo.wifi.uri", "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs?action=worse-accuracy");
+}
+
 function resume_geolocationProvider()
 {
   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

dom/tests/mochitest/geolocation/network_geolocation.sjs

@@ -17,7 +17,7 @@ function parseQueryString(str)
   return params;
 }
 
-function getPosition()
+function getPosition(action)
 {  
     // this isn't the w3c data structure, it is the network location provider structure.
 
@@ -39,7 +39,7 @@ function getPosition()
     longitude: -122.08769,
 
     altitude: 42,
-    accuracy: 42,
+    accuracy: (action == "worse-accuracy") ? 100 : 42,
     altitude_accuracy: 42,
   };
   
@@ -58,7 +58,7 @@ function handleRequest(request, response)
       return;
   }
 
-  var position = getPosition();
+  var position = getPosition(params.action);
 
   if (params.action == "respond-garbage") {
      // better way?

dom/tests/mochitest/geolocation/test_worseAccuracyDoesNotBlockCallback.html

@@ -0,0 +1,45 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=494924
+-->
+<head>
+  <title>Test for getCurrentPosition </title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="geolocation_common.js"></script>
+
+<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=494924">Mozilla Bug 494924</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+SimpleTest.waitForExplicitFinish();
+resume_geolocationProvider();
+force_prompt(true);
+
+function successCallback2(position) {
+  check_geolocation(position);
+  reset_prompt();
+  SimpleTest.finish();
+}
+
+function successCallback1(position) {
+  check_geolocation(position);
+  worse_geolocationProvider();
+  navigator.geolocation.getCurrentPosition(successCallback2, null, { maximumAge: 0 });
+}
+
+navigator.geolocation.getCurrentPosition(successCallback1, null, { maximumAge: 0 });
+
+</script>
+</pre>
+</body>
+</html>
+