From 3d0449b2ffef448e28411c1b2af366bbcb6d9f63 Mon Sep 17 00:00:00 2001
From: Gijs Kruitbosch <gijskruitbosch@gmail.com>
Date: Fri, 13 Jan 2017 13:42:47 +0000
Subject: [PATCH] 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

--HG--
extra : rebase_source : 8646b7fe965b7110a69a728b54e5c149c750f9c3
---
 .../lib/CleanupManager.jsm                    | 14 +++++++--
 .../shield-recipe-client/lib/Heartbeat.jsm    | 29 ++++++++++++-------
 .../shield-recipe-client/test/browser.ini     |  1 -
 3 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/browser/extensions/shield-recipe-client/lib/CleanupManager.jsm b/browser/extensions/shield-recipe-client/lib/CleanupManager.jsm
index b2ebae69c433..2e5f2867f374 100644
--- a/browser/extensions/shield-recipe-client/lib/CleanupManager.jsm
+++ b/browser/extensions/shield-recipe-client/lib/CleanupManager.jsm
@@ -6,16 +6,24 @@
 
 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);
+      }
     }
   },
 };
diff --git a/browser/extensions/shield-recipe-client/lib/Heartbeat.jsm b/browser/extensions/shield-recipe-client/lib/Heartbeat.jsm
index 8742c347c6e3..64d62243aeaf 100644
--- a/browser/extensions/shield-recipe-client/lib/Heartbeat.jsm
+++ b/browser/extensions/shield-recipe-client/lib/Heartbeat.jsm
@@ -100,6 +100,7 @@ this.Heartbeat = class {
 
     // so event handlers are consistent
     this.handleWindowClosed = this.handleWindowClosed.bind(this);
+    this.close = this.close.bind(this);
 
     if (this.options.engagementButtonLabel) {
       this.buttons = [{
@@ -208,7 +209,7 @@ this.Heartbeat = class {
     }, surveyDuration);
 
     this.sandboxManager.addHold("heartbeat");
-    CleanupManager.addCleanupHandler(() => this.close());
+    CleanupManager.addCleanupHandler(this.close);
   }
 
   maybeNotifyHeartbeat(name, data = {}) {
@@ -281,10 +282,7 @@ this.Heartbeat = class {
       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;
@@ -315,12 +313,16 @@ this.Heartbeat = class {
       this.chromeWindow.gBrowser.selectedTab = this.chromeWindow.gBrowser.addTab(this.options.postAnswerUrl.toString());
     }
 
-    if (this.surveyEndTimer) {
-      clearTimeout(this.surveyEndTimer);
-      this.surveyEndTimer = null;
-    }
+    this.endTimerIfPresent("surveyEndTimer");
 
-    setTimeout(() => this.close(), NOTIFICATION_TIME);
+    this.engagementCloseTimer = setTimeout(() => this.close(), NOTIFICATION_TIME);
+  }
+
+  endTimerIfPresent(timerName) {
+    if (this[timerName]) {
+      clearTimeout(this[timerName]);
+      this[timerName] = null;
+    }
   }
 
   handleWindowClosed() {
@@ -333,6 +335,10 @@ this.Heartbeat = class {
   }
 
   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);
@@ -340,7 +346,10 @@ this.Heartbeat = class {
     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);
   }
 };
diff --git a/browser/extensions/shield-recipe-client/test/browser.ini b/browser/extensions/shield-recipe-client/test/browser.ini
index 56ff2d9c0645..75d392fb3c07 100644
--- a/browser/extensions/shield-recipe-client/test/browser.ini
+++ b/browser/extensions/shield-recipe-client/test/browser.ini
@@ -3,7 +3,6 @@
 [browser_EventEmitter.js]
 [browser_Storage.js]
 [browser_Heartbeat.js]
-skip-if = true # bug 1325409
 [browser_NormandyApi.js]
   support-files =
     test_server.sjs