Bug 1455737 - Clean up DownloadHistory history observer on shutdown to prevent leaks. r=Paolo
authorEd Lee <edilee@mozilla.com>
Sat, 21 Apr 2018 16:59:06 -0700
changeset 414989 b154d7186790f01d339e9dd12df849230f447297
parent 414972 087038ccc3e3304d2ca8342ab3c155eb4222d1eb
child 414990 6747ccc87604bc807ea2c4d26be9ca32eb49c762
push id33885
push usertoros@mozilla.com
push dateSun, 22 Apr 2018 22:12:17 +0000
treeherdermozilla-central@378a8a64401f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersPaolo
bugs1455737
milestone61.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 1455737 - Clean up DownloadHistory history observer on shutdown to prevent leaks. r=Paolo Reverts broken removeView fix and add a test to make sure it doesn't regress again. MozReview-Commit-ID: CWCYUS4GJ3A
toolkit/components/downloads/DownloadHistory.jsm
toolkit/components/downloads/test/unit/test_DownloadHistory.js
--- a/toolkit/components/downloads/DownloadHistory.jsm
+++ b/toolkit/components/downloads/DownloadHistory.jsm
@@ -416,18 +416,26 @@ var DownloadHistoryList = function(publi
   this._slots = [];
   this._slotsForUrl = new Map();
   this._slotForDownload = new WeakMap();
 
   // Start the asynchronous queries to retrieve history and session downloads.
   publicList.addView(this).catch(Cu.reportError);
   let query = {}, options = {};
   PlacesUtils.history.queryStringToQuery(place, query, options);
+
+  // NB: The addObserver call sets our nsINavHistoryResultObserver.result.
   let result = PlacesUtils.history.executeQuery(query.value, options.value);
   result.addObserver(this);
+
+  // Our history result observer is long lived for fast shared views, so free
+  // the reference on shutdown to prevent leaks.
+  Services.obs.addObserver(() => {
+    this.result = null;
+  }, "quit-application-granted");
 };
 
 this.DownloadHistoryList.prototype = {
   __proto__: DownloadList.prototype,
 
   /**
    * This is set when executing the Places query.
    */
@@ -450,27 +458,16 @@ this.DownloadHistoryList.prototype = {
     if (this._result) {
       this._result.root.containerOpen = true;
       PlacesUtils.annotations.addObserver(this);
     }
   },
   _result: null,
 
   /**
-   * Remove the view that belongs to this list via DownloadList's removeView. In
-   * addition, delete the result object to ensure there are no memory leaks.
-   */
-  removeView(aView) {
-    DownloadList.prototype.removeView.call(this, aView);
-
-    // Clean up any active results that might still be observing. See bug 1455737
-    this.result = null;
-  },
-
-  /**
    * Index of the first slot that contains a session download. This is equal to
    * the length of the list when there are no session downloads.
    */
   _firstSessionSlotIndex: 0,
 
   _insertSlot({ slot, index, slotsForUrl }) {
     // Add the slot to the ordered array.
     this._slots.splice(index, 0, slot);
--- a/toolkit/components/downloads/test/unit/test_DownloadHistory.js
+++ b/toolkit/components/downloads/test/unit/test_DownloadHistory.js
@@ -253,17 +253,29 @@ add_task(async function test_DownloadHis
   let allHistoryList2 = await DownloadHistory.getList({ type: Downloads.ALL,
     maxHistoryResults: 3 });
   // Prepare the set of downloads to contain fewer history downloads by removing
   // the oldest ones.
   let allView2 = new TestView(allView.expected.slice(3));
   await allHistoryList2.addView(allView2);
   await allView2.waitForExpected();
 
+  // Create a dummy list and view like the previous limited one to just add and
+  // remove its view to make sure it doesn't break other lists' updates.
+  let dummyList = await DownloadHistory.getList({ type: Downloads.ALL,
+    maxHistoryResults: 3 });
+  let dummyView = new TestView([]);
+  await dummyList.addView(dummyView);
+  await dummyList.removeView(dummyView);
+
   // Clear history and check that session downloads with partial data remain.
   // Private downloads are also not cleared when clearing history.
   view.expected = view.expected.filter(d => d.hasPartialData);
   allView.expected = allView.expected.filter(d => d.hasPartialData ||
                                                   d.isPrivate);
   await PlacesUtils.history.clear();
   await view.waitForExpected();
   await allView.waitForExpected();
+
+  // Check that the dummy view above did not prevent the limited from updating.
+  allView2.expected = allView.expected;
+  await allView2.waitForExpected();
 });