Bug 911595 - Avoid shutting down geolocation request twice. r=jdm
authorGuilherme Gonçalves <ggp@mozilla.com>
Thu, 12 Sep 2013 11:08:50 -0400
changeset 159758 cedb0b0bdd79ab641b80f31600f08ccaa70ac0bb
parent 159757 3eec9be9fcd01461d43abaec8e3b5e699ba3ae72
child 159759 3d5ec876edb3421e2ccc671c6319ba0311b4937f
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
bugs911595
milestone26.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 911595 - Avoid shutting down geolocation request twice. r=jdm
dom/src/geolocation/nsGeolocation.cpp
dom/tests/mochitest/geolocation/Makefile.in
dom/tests/mochitest/geolocation/test_handlerSpinsEventLoop.html
testing/mochitest/android.json
testing/mochitest/androidx86.json
testing/mochitest/b2g.json
--- a/dom/src/geolocation/nsGeolocation.cpp
+++ b/dom/src/geolocation/nsGeolocation.cpp
@@ -364,21 +364,24 @@ NS_IMPL_CYCLE_COLLECTING_RELEASE(nsGeolo
 
 NS_IMPL_CYCLE_COLLECTION_3(nsGeolocationRequest, mCallback, mErrorCallback, mLocator)
 
 NS_IMETHODIMP
 nsGeolocationRequest::Notify(nsITimer* aTimer)
 {
   MOZ_ASSERT(!mShutdown, "timeout after shutdown");
 
-  NotifyError(nsIDOMGeoPositionError::TIMEOUT);
   if (!mIsWatchPositionRequest) {
     Shutdown();
     mLocator->RemoveRequest(this);
-  } else if (!mShutdown) {
+  }
+
+  NotifyError(nsIDOMGeoPositionError::TIMEOUT);
+
+  if (!mShutdown) {
     SetTimeoutTimer();
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsGeolocationRequest::GetPrincipal(nsIPrincipal * *aRequestingPrincipal)
@@ -529,16 +532,21 @@ nsGeolocationRequest::SendLocation(nsIDO
   }
 
   if (!wrapped) {
     NotifyError(nsIDOMGeoPositionError::POSITION_UNAVAILABLE);
     return;
   }
 
   mLocator->SetCachedPosition(wrapped);
+  if (!mIsWatchPositionRequest) {
+    // Cancel timer and position updates in case the position
+    // callback spins the event loop
+    Shutdown();
+  }
 
   // Ensure that the proper context is on the stack (bug 452762)
   nsCxPusher pusher;
   pusher.PushNull();
   nsAutoMicroTask mt;
   if (mCallback.HasWebIDLCallback()) {
     ErrorResult err;
     PositionCallback* callback = mCallback.GetWebIDLCallback();
@@ -547,19 +555,20 @@ nsGeolocationRequest::SendLocation(nsIDO
     callback->Call(*wrapped, err);
   } else {
     nsIDOMGeoPositionCallback* callback = mCallback.GetXPCOMCallback();
 
     MOZ_ASSERT(callback);
     callback->HandleEvent(aPosition);
   }
 
-  if (!mIsWatchPositionRequest) {
-    Shutdown();
-  } else if (!mShutdown) { // The handler may have called clearWatch
+  if (!mShutdown) {
+    // For watch requests, the handler may have called clearWatch
+    MOZ_ASSERT(mIsWatchPositionRequest,
+               "non-shutdown getCurrentPosition request after callback!");
     SetTimeoutTimer();
   }
 }
 
 nsIPrincipal*
 nsGeolocationRequest::GetPrincipal()
 {
   if (!mLocator) {
--- a/dom/tests/mochitest/geolocation/Makefile.in
+++ b/dom/tests/mochitest/geolocation/Makefile.in
@@ -6,16 +6,17 @@ MOCHITEST_FILES	= \
 		test_allowCurrent.html \
 		test_allowWatch.html \
 		test_cachedPosition.html \
 		test_cancelCurrent.html \
 		test_cancelWatch.html \
 		test_clearWatch.html \
 		test_clearWatch_invalid.html \
 		test_geolocation_is_undefined_when_pref_is_off.html \
+		test_handlerSpinsEventLoop.html \
 		test_manyCurrentConcurrent.html \
 		test_manyCurrentSerial.html \
 		test_manyWatchConcurrent.html \
 		test_manyWatchSerial.html \
 		test_manyWindows.html \
 		test_optional_api_params.html \
 		test_shutdown.html \
 		test_windowClose.html \
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/geolocation/test_handlerSpinsEventLoop.html
@@ -0,0 +1,66 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=911595
+-->
+<head>
+  <title>Test for spinning the event loop inside position handlers</title>
+  <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=911595 ">Mozilla Bug 911595</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+/*
+ * In bug 911595 , spinning the event loop from inside position
+ * handlers could cause both success and error callbacks to be
+ * fired for the same request if that request has a small timeout.
+ */
+
+SimpleTest.waitForExplicitFinish();
+
+resume_geolocationProvider(function() {
+  force_prompt(true, test1);
+});
+
+function spinEventLoopAndSetTimeout() {
+  if (successCallbackCalled || errorCallbackCalled) {
+    // this should only be called once from either callback
+    return;
+  }
+
+  window.showModalDialog("javascript:window.close()");
+
+  setTimeout(function() {
+    ok(successCallbackCalled != errorCallbackCalled, "Ensure only one callback is called");
+    SimpleTest.finish();
+  }, 5);
+}
+
+var successCallbackCalled = false;
+function successCallback(position) {
+  spinEventLoopAndSetTimeout();
+  successCallbackCalled = true;
+}
+
+var errorCallbackCalled = false;
+function errorCallback(error) {
+  spinEventLoopAndSetTimeout();
+  errorCallbackCalled = true;
+}
+
+function test1() {
+  navigator.geolocation.getCurrentPosition(successCallback, errorCallback, {timeout: 1});
+}
+</script>
+</pre>
+</body>
+</html>
--- a/testing/mochitest/android.json
+++ b/testing/mochitest/android.json
@@ -179,16 +179,17 @@
  "dom/tests/mochitest/general/test_497898.html": "",
  "dom/tests/mochitest/general/test_focusrings.xul": "TIMED_OUT",
  "dom/tests/mochitest/general/test_vibrator.html": "CRASH_SUTAGENT",
  "dom/tests/mochitest/general/test_showModalDialog.html": "Don't run modal tests on Android",
  "dom/tests/mochitest/geolocation/test_allowCurrent.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_allowWatch.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_cachedPosition.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_clearWatch.html": "TIMED_OUT",
+ "dom/tests/mochitest/geolocation/test_handlerSpinsEventLoop.html": "Don't run modal tests on Android",
  "dom/tests/mochitest/geolocation/test_manyCurrentConcurrent.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_manyCurrentSerial.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_manyWatchConcurrent.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_manyWatchSerial.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_mozsettings.html": "mozSettings is undefined",
  "dom/tests/mochitest/geolocation/test_mozsettingsWatch.html": "mozSettings is undefined",
  "dom/tests/mochitest/geolocation/test_shutdown.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_windowClose.html": "TIMED_OUT",
--- a/testing/mochitest/androidx86.json
+++ b/testing/mochitest/androidx86.json
@@ -243,16 +243,17 @@
  "dom/tests/mochitest/general/test_497898.html": "",
  "dom/tests/mochitest/general/test_focusrings.xul": "TIMED_OUT",
  "dom/tests/mochitest/general/test_vibrator.html": "CRASH_SUTAGENT",
  "dom/tests/mochitest/general/test_showModalDialog.html": "Don't run modal tests on Android",
  "dom/tests/mochitest/geolocation/test_allowCurrent.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_allowWatch.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_cachedPosition.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_clearWatch.html": "TIMED_OUT",
+ "dom/tests/mochitest/geolocation/test_handlerSpinsEventLoop.html": "Don't run modal tests on Android",
  "dom/tests/mochitest/geolocation/test_manyCurrentConcurrent.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_manyCurrentSerial.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_manyWatchConcurrent.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_manyWatchSerial.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_mozsettings.html": "mozSettings is undefined",
  "dom/tests/mochitest/geolocation/test_mozsettingsWatch.html": "mozSettings is undefined",
  "dom/tests/mochitest/geolocation/test_shutdown.html": "TIMED_OUT",
  "dom/tests/mochitest/geolocation/test_windowClose.html": "TIMED_OUT",
--- a/testing/mochitest/b2g.json
+++ b/testing/mochitest/b2g.json
@@ -342,16 +342,17 @@
 
     "dom/tests/mochitest/geolocation/test_allowCurrent.html":"Bug 910235 - Error: no message manager set when calling method: [nsIObserver::observe]",
     "dom/tests/mochitest/geolocation/test_allowWatch.html":"",
     "dom/tests/mochitest/geolocation/test_cachedPosition.html":"",
     "dom/tests/mochitest/geolocation/test_cancelCurrent.html":"",
     "dom/tests/mochitest/geolocation/test_cancelWatch.html":"",
     "dom/tests/mochitest/geolocation/test_clearWatch.html":"",
     "dom/tests/mochitest/geolocation/test_clearWatch_invalid.html":"",
+    "dom/tests/mochitest/geolocation/test_handlerSpinsEventLoop.html": "showmodaldialog",
     "dom/tests/mochitest/geolocation/test_manyCurrentConcurrent.html":"",
     "dom/tests/mochitest/geolocation/test_manyCurrentSerial.html":"",
     "dom/tests/mochitest/geolocation/test_manyWatchConcurrent.html":"",
     "dom/tests/mochitest/geolocation/test_manyWatchSerial.html":"",
     "dom/tests/mochitest/geolocation/test_manyWindows.html":"",
     "dom/tests/mochitest/geolocation/test_mozsettings.html":"",
     "dom/tests/mochitest/geolocation/test_mozsettingsWatch.html":"",
     "dom/tests/mochitest/geolocation/test_optional_api_params.html":"",