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

@@ -421,8 +421,16 @@ var DownloadHistoryList = function(publicList, place) {
   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 = {
@@ -454,17 +462,6 @@ this.DownloadHistoryList.prototype = {
   },
   _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.

toolkit/components/downloads/test/unit/test_DownloadHistory.js

@@ -258,6 +258,14 @@ add_task(async function test_DownloadHistory() {
   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);
@@ -266,4 +274,8 @@ add_task(async function test_DownloadHistory() {
   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();
 });