Bug 1330954 - fix leakiness of SHIELD system add-on and re-enable test, r=kmag,mythmon
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Fri, 13 Jan 2017 13:42:47 +0000
changeset 374482 a6b1db06ced9a83d00630a1e10b7d782639ca8b1
parent 374481 4374afb7411ed698716013a6b2c5f8a4235aeb3b
child 374483 4ed09f5f11991b28f838d657e999992d9ad22d46
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, mythmon
bugs1330954
milestone53.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 1330954 - fix leakiness of SHIELD system add-on and re-enable test, r=kmag,mythmon There are 3 issues this tackles: - we were keeping a reference to the notification we added (this.notice), which kept windows alive. - we had the CleanupManager keep a reference to our close method, which kept a reference to us, which kept a reference to the window. - the fact that we use timeouts to call this.close() in 2 places meant this.close would get called multiple times, which meant we errored out on later occasions, because various things had been nulled out. This tidies up the timeouts when cleanup is called to avoid re-entrancy/errors/leaks. MozReview-Commit-ID: EYvK7bQEh3X
browser/extensions/shield-recipe-client/lib/CleanupManager.jsm
browser/extensions/shield-recipe-client/lib/Heartbeat.jsm
browser/extensions/shield-recipe-client/test/browser.ini
--- a/browser/extensions/shield-recipe-client/lib/CleanupManager.jsm
+++ b/browser/extensions/shield-recipe-client/lib/CleanupManager.jsm
@@ -1,21 +1,29 @@
 /* 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";
 
 this.EXPORTED_SYMBOLS = ["CleanupManager"];
 
-const cleanupHandlers = [];
+const cleanupHandlers = new Set();
 
 this.CleanupManager = {
   addCleanupHandler(handler) {
-    cleanupHandlers.push(handler);
+    cleanupHandlers.add(handler);
+  },
+
+  removeCleanupHandler(handler) {
+    cleanupHandlers.delete(handler);
   },
 
   cleanup() {
     for (const handler of cleanupHandlers) {
-      handler();
+      try {
+        handler();
+      } catch (ex) {
+        Cu.reportError(ex);
+      }
     }
   },
 };
--- a/browser/extensions/shield-recipe-client/lib/Heartbeat.jsm
+++ b/browser/extensions/shield-recipe-client/lib/Heartbeat.jsm
@@ -95,16 +95,17 @@ this.Heartbeat = class {
     this.eventEmitter = eventEmitter;
     this.sandboxManager = sandboxManager;
     this.options = options;
     this.surveyResults = {};
     this.buttons = null;
 
     // so event handlers are consistent
     this.handleWindowClosed = this.handleWindowClosed.bind(this);
+    this.close = this.close.bind(this);
 
     if (this.options.engagementButtonLabel) {
       this.buttons = [{
         label: this.options.engagementButtonLabel,
         callback: () => {
           // Let the consumer know user engaged.
           this.maybeNotifyHeartbeat("Engaged");
 
@@ -203,17 +204,17 @@ this.Heartbeat = class {
 
     const surveyDuration = Preferences.get(PREF_SURVEY_DURATION, 300) * 1000;
     this.surveyEndTimer = setTimeout(() => {
       this.maybeNotifyHeartbeat("SurveyExpired");
       this.close();
     }, surveyDuration);
 
     this.sandboxManager.addHold("heartbeat");
-    CleanupManager.addCleanupHandler(() => this.close());
+    CleanupManager.addCleanupHandler(this.close);
   }
 
   maybeNotifyHeartbeat(name, data = {}) {
     if (this.pingSent) {
       log.warn("Heartbeat event recieved after Telemetry ping sent. name:", name, "data:", data);
       return;
     }
 
@@ -276,20 +277,17 @@ this.Heartbeat = class {
         addClientId: true,
         addEnvironment: true,
       });
 
       // only for testing
       this.eventEmitter.emit("TelemetrySent", Cu.cloneInto(payload, this.sandboxManager.sandbox));
 
       // Survey is complete, clear out the expiry timer & survey configuration
-      if (this.surveyEndTimer) {
-        clearTimeout(this.surveyEndTimer);
-        this.surveyEndTimer = null;
-      }
+      this.endTimerIfPresent("surveyEndTimer");
 
       this.pingSent = true;
       this.surveyResults = null;
     }
 
     if (cleanup) {
       this.cleanup();
     }
@@ -310,37 +308,48 @@ this.Heartbeat = class {
     if (this.options.postAnswerUrl) {
       for (const key in engagementParams) {
         this.options.postAnswerUrl.searchParams.append(key, engagementParams[key]);
       }
       // Open the engagement URL in a new tab.
       this.chromeWindow.gBrowser.selectedTab = this.chromeWindow.gBrowser.addTab(this.options.postAnswerUrl.toString());
     }
 
-    if (this.surveyEndTimer) {
-      clearTimeout(this.surveyEndTimer);
-      this.surveyEndTimer = null;
+    this.endTimerIfPresent("surveyEndTimer");
+
+    this.engagementCloseTimer = setTimeout(() => this.close(), NOTIFICATION_TIME);
+  }
+
+  endTimerIfPresent(timerName) {
+    if (this[timerName]) {
+      clearTimeout(this[timerName]);
+      this[timerName] = null;
     }
-
-    setTimeout(() => this.close(), NOTIFICATION_TIME);
   }
 
   handleWindowClosed() {
     this.maybeNotifyHeartbeat("WindowClosed");
   }
 
   close() {
     this.notificationBox.removeNotification(this.notice);
     this.cleanup();
   }
 
   cleanup() {
+    // Kill the timers which might call things after we've cleaned up:
+    this.endTimerIfPresent("surveyEndTimer");
+    this.endTimerIfPresent("engagementCloseTimer");
+
     this.sandboxManager.removeHold("heartbeat");
     // remove listeners
     this.chromeWindow.removeEventListener("SSWindowClosing", this.handleWindowClosed);
     // remove references for garbage collection
     this.chromeWindow = null;
     this.notificationBox = null;
     this.notification = null;
+    this.notice = null;
     this.eventEmitter = null;
     this.sandboxManager = null;
+    // Ensure we don't re-enter and release the CleanupManager's reference to us:
+    CleanupManager.removeCleanupHandler(this.close);
   }
 };
--- a/browser/extensions/shield-recipe-client/test/browser.ini
+++ b/browser/extensions/shield-recipe-client/test/browser.ini
@@ -1,9 +1,8 @@
 [browser_driver_uuids.js]
 [browser_env_expressions.js]
 [browser_EventEmitter.js]
 [browser_Storage.js]
 [browser_Heartbeat.js]
-skip-if = true # bug 1325409
 [browser_NormandyApi.js]
   support-files =
     test_server.sjs