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
--- 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();
});