Bug 1238427 - Avoid a strong reference from the timeout timer to nsGeolocationRequest. r=jdm

The timeout timer of a geolocation request holds a strong reference to
the request. This can cause the window to leak if the request is not
completed before the tab containing the window is closed.

To fix this, I made the timer instead hold a strong reference to a
wrapper class that has only a weak reference to the request. The
request destructor must now cancel the timeout timer.

I also outlined a call to StopTimeoutTimer() in
nsGeolocationRequest::Shutdown().

dom/geolocation/nsGeolocation.cpp

@@ -31,6 +31,7 @@
 #include "mozilla/Preferences.h"
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/dom/Event.h"
+#include "mozilla/WeakPtr.h"
 #include "mozilla/dom/PermissionMessageUtils.h"
 #include "mozilla/dom/SettingChangeNotificationBinding.h"
 #include "mozilla/dom/WakeLock.h"
@@ -75,13 +76,12 @@ using namespace mozilla::hal;
 
 class nsGeolocationRequest final
  : public nsIContentPermissionRequest
- , public nsITimerCallback
  , public nsIGeolocationUpdate
+ , public SupportsWeakPtr<nsGeolocationRequest>
 {
  public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_NSICONTENTPERMISSIONREQUEST
-  NS_DECL_NSITIMERCALLBACK
   NS_DECL_NSIGEOLOCATIONUPDATE
 
   NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsGeolocationRequest, nsIContentPermissionRequest)
@@ -93,6 +93,9 @@ class nsGeolocationRequest final
                        uint8_t aProtocolType,
                        bool aWatchPositionRequest = false,
                        int32_t aWatchId = 0);
+
+  MOZ_DECLARE_WEAKREFERENCE_TYPENAME(nsGeolocationRequest)
+
   void Shutdown();
 
   void SendLocation(nsIDOMGeoPosition* aLocation);
@@ -107,6 +110,23 @@ class nsGeolocationRequest final
  private:
   virtual ~nsGeolocationRequest();
 
+  class TimerCallbackHolder final : public nsITimerCallback
+  {
+  public:
+    NS_DECL_ISUPPORTS
+    NS_DECL_NSITIMERCALLBACK
+
+    TimerCallbackHolder(nsGeolocationRequest* aRequest)
+      : mRequest(aRequest)
+    {}
+
+  private:
+    ~TimerCallbackHolder() {}
+    WeakPtr<nsGeolocationRequest> mRequest;
+  };
+
+  void Notify();
+
   already_AddRefed<nsIDOMGeoPosition> AdjustedLocation(nsIDOMGeoPosition*);
 
   bool mIsWatchPositionRequest;
@@ -379,12 +399,12 @@ nsGeolocationRequest::nsGeolocationRequest(Geolocation* aLocator,
 
 nsGeolocationRequest::~nsGeolocationRequest()
 {
+  StopTimeoutTimer();
 }
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGeolocationRequest)
   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContentPermissionRequest)
   NS_INTERFACE_MAP_ENTRY(nsIContentPermissionRequest)
-  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
   NS_INTERFACE_MAP_ENTRY(nsIGeolocationUpdate)
 NS_INTERFACE_MAP_END
 
@@ -393,12 +413,11 @@ NS_IMPL_CYCLE_COLLECTING_RELEASE(nsGeolocationRequest)
 
 NS_IMPL_CYCLE_COLLECTION(nsGeolocationRequest, mCallback, mErrorCallback, mLocator)
 
-NS_IMETHODIMP
-nsGeolocationRequest::Notify(nsITimer* aTimer)
+void
+nsGeolocationRequest::Notify()
 {
   StopTimeoutTimer();
   NotifyErrorAndShutdown(nsIDOMGeoPositionError::TIMEOUT);
-  return NS_OK;
 }
 
 void
@@ -561,7 +580,8 @@ nsGeolocationRequest::SetTimeoutTimer()
     }
 
     mTimeoutTimer = do_CreateInstance("@mozilla.org/timer;1");
-    mTimeoutTimer->InitWithCallback(this, timeout, nsITimer::TYPE_ONE_SHOT);
+    RefPtr<TimerCallbackHolder> holder = new TimerCallbackHolder(this);
+    mTimeoutTimer->InitWithCallback(holder, timeout, nsITimer::TYPE_ONE_SHOT);
   }
 }
 
@@ -741,10 +761,7 @@ nsGeolocationRequest::Shutdown()
   MOZ_ASSERT(!mShutdown, "request shutdown twice");
   mShutdown = true;
 
-  if (mTimeoutTimer) {
-    mTimeoutTimer->Cancel();
-    mTimeoutTimer = nullptr;
-  }
+  StopTimeoutTimer();
 
   // If there are no other high accuracy requests, the geolocation service will
   // notify the provider to switch to the default accuracy.
@@ -756,6 +773,23 @@ nsGeolocationRequest::Shutdown()
   }
 }
 
+
+////////////////////////////////////////////////////
+// nsGeolocationRequest::TimerCallbackHolder
+////////////////////////////////////////////////////
+
+NS_IMPL_ISUPPORTS(nsGeolocationRequest::TimerCallbackHolder, nsISupports, nsITimerCallback)
+
+NS_IMETHODIMP
+nsGeolocationRequest::TimerCallbackHolder::Notify(nsITimer*)
+{
+  if (mRequest) {
+    mRequest->Notify();
+  }
+  return NS_OK;
+}
+
+
 ////////////////////////////////////////////////////
 // nsGeolocationService
 ////////////////////////////////////////////////////

dom/tests/browser/browser.ini

@@ -7,6 +7,7 @@ support-files =
   test-console-api.html
   test_bug1004814.html
   worker_bug1004814.js
+  geo_leak_test.html
 
 [browser_test_toolbars_visibility.js]
 support-files =
@@ -20,6 +21,7 @@ skip-if = buildapp == 'mulet'
 [browser_autofocus_background.js]
 skip-if= buildapp == 'mulet'
 [browser_autofocus_preference.js]
+[browser_bug1238427.js]
 [browser_bug396843.js]
 [browser_focus_steal_from_chrome.js]
 [browser_focus_steal_from_chrome_during_mousedown.js]

dom/tests/browser/browser_bug1238427.js

@@ -0,0 +1,31 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+"use strict";
+
+const TEST_URI = "http://example.com/" +
+                 "browser/dom/tests/browser/geo_leak_test.html";
+
+const BASE_GEO_URL = "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs";
+
+add_task(function* () {
+  Services.prefs.setBoolPref("geo.prompt.testing", true);
+  Services.prefs.setBoolPref("geo.prompt.testing.allow", true);
+
+  // Make the geolocation provider responder very slowly to ensure that
+  // it does not reply before we close the tab.
+  Services.prefs.setCharPref("geo.wifi.uri", BASE_GEO_URL + "?delay=100000");
+
+  // Open the test URI and close it. The test harness will make sure that the
+  // page is cleaned up after some GCs. If geolocation is not shut down properly,
+  // it will show up as a non-shutdown leak.
+  yield BrowserTestUtils.withNewTab({
+    gBrowser,
+    url: TEST_URI
+  }, function* (browser) { /* ... */ });
+
+  ok(true, "Need to do something in this test");
+});
+

dom/tests/browser/geo_leak_test.html

@@ -0,0 +1,17 @@
+<html>
+<head>
+<meta http-equiv="Content-Type" content="text/html;charset=utf-8" />
+<title>Geolocation incomplete position leak test</title>
+<script type="text/javascript">
+
+function successCallback(position) {}
+function errorCallback() {}
+
+function init() {
+  navigator.geolocation.getCurrentPosition(successCallback, errorCallback);
+}
+</script>
+</head>
+<body onload="init()">
+</body>
+</html>