Bug 1145911 - fix leak and other test failures for reading list, r=gijs a=readinglist
authorFlorian Quèze <florian@queze.net>
Sat, 21 Mar 2015 08:56:40 -0700
changeset 259734 54db0a4c777f
parent 259733 4f9d6ba4a684
child 259735 cb38b4973ea9
push id721
push userjlund@mozilla.com
push date2015-04-21 23:03 +0000
treeherdermozilla-release@d27c9211ebb3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgijs, readinglist
bugs1145911
milestone38.0a2
Bug 1145911 - fix leak and other test failures for reading list, r=gijs a=readinglist
browser/base/content/browser-readinglist.js
browser/base/content/test/general/browser_readerMode.js
--- a/browser/base/content/browser-readinglist.js
+++ b/browser/base/content/browser-readinglist.js
@@ -53,16 +53,21 @@ let ReadingListUI = {
    */
   uninit() {
     Preferences.ignore("browser.readinglist.enabled", this.updateUI, this);
 
     const mm = window.messageManager;
     for (let msg of this.MESSAGES) {
       mm.removeMessageListener(msg, this);
     }
+
+    if (this.listenerRegistered) {
+      ReadingList.removeListener(this);
+      this.listenerRegistered = false;
+    }
   },
 
   /**
    * Whether the ReadingList feature is enabled or not.
    * @type {boolean}
    */
   get enabled() {
     return Preferences.get("browser.readinglist.enabled", false);
@@ -86,17 +91,17 @@ let ReadingListUI = {
       // This is a no-op if we're already registered.
       ReadingList.addListener(this);
       this.listenerRegistered = true;
     } else {
       if (this.listenerRegistered) {
         // This is safe to call if we're not currently registered, but we don't
         // want to forcibly load the normally lazy-loaded module on startup.
         ReadingList.removeListener(this);
-        this.listenerRegistered = true;
+        this.listenerRegistered = false;
       }
 
       this.hideSidebar();
     }
 
     document.getElementById(READINGLIST_COMMAND_ID).setAttribute("hidden", !enabled);
   },
 
@@ -246,16 +251,22 @@ let ReadingListUI = {
     if (!uri) {
       this.toolbarButton.setAttribute("hidden", true);
       if (this.isSidebarOpen)
         document.getElementById("sidebar").contentWindow.postMessage(msg, "*");
       return;
     }
 
     let isInList = yield ReadingList.hasItemForURL(uri);
+
+    if (window.closed) {
+      // Skip updating the UI if the window was closed since our hasItemForURL call.
+      return;
+    }
+
     if (this.isSidebarOpen) {
       if (isInList)
         msg.url = typeof uri == "string" ? uri : uri.spec;
       document.getElementById("sidebar").contentWindow.postMessage(msg, "*");
     }
     this.setToolbarButtonState(isInList);
   }),
 
--- a/browser/base/content/test/general/browser_readerMode.js
+++ b/browser/base/content/test/general/browser_readerMode.js
@@ -60,16 +60,21 @@ add_task(function* () {
   // After we click ReadingList button, status should be "open".
   listButton.click();
   yield promiseWaitForCondition(() => listButton.classList.contains("on"));
   ok(listButton.classList.contains("on"),
     "List button should now indicate SideBar-ReadingList open.");
   ok(ReadingListUI.isSidebarOpen,
     "The ReadingListUI should now indicate SideBar-ReadingList open.");
 
+  // Now close the sidebar.
+  listButton.click();
+  yield promiseWaitForCondition(() => !listButton.classList.contains("on"));
+  ok(!ReadingListUI.isSidebarOpen, "The sidebar should be closed.");
+
   readerButton.click();
   yield promiseTabLoadEvent(tab);
   is(gBrowser.selectedBrowser.currentURI.spec, url, "Original page loaded after clicking active reader mode button");
 
   // Load a new tab that is NOT reader-able.
   let newTab = gBrowser.selectedTab = gBrowser.addTab();
   yield promiseTabLoadEvent(newTab, "about:robots");
   yield promiseWaitForCondition(() => readerButton.hidden);