Bug 1433230 introduced a memory leak when running the test: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html I managed to track it down to DownloadHistory.jsm adding an observer on the result object: https://dxr.mozilla.org/mozilla-central/rev/3cc613bf13443acc2fea4804872fb3ca56757181/toolkit/components/downloads/DownloadHistory.jsm#425, but not properly removing it. The fix would be to make ensure that we destroy any result object on DownloadHistory when we remove the view: https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/browser/components/downloads/DownloadsCommon.jsm#795

Comment on attachment 8969786 [details] Bug 1455737 - Remove the result object from DownloadHistory when removing the view https://reviewboard.mozilla.org/r/238618/#review244330 We should fix this by overriding DownloadHistoryList.removeView ::: browser/components/downloads/DownloadsCommon.jsm:798 (Diff revision 1) > * DownloadsView object to be removed. > */ > removeView(aView) { > - this._promiseList.then(list => list.removeView(aView)) > - .catch(Cu.reportError); > + this._promiseList.then(list => { > + // If we have an old result lying around, remove it. See bug 1455737 > + if (list.result) { When moved to DownloadHistoryList, no need to check. Just always assign null as its results setter checks. ::: browser/components/downloads/DownloadsCommon.jsm:801 (Diff revision 1) > - this._promiseList.then(list => list.removeView(aView)) > - .catch(Cu.reportError); > + this._promiseList.then(list => { > + // If we have an old result lying around, remove it. See bug 1455737 > + if (list.result) { > + list.result = null; > + } > + list.removeView(aView) Because we're calling removeView, this seems like something the list itself should know to do. Not all lists track a .result I believe -- only DownloadHistoryList. The generic DownloadList.removeView: https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/toolkit/components/downloads/DownloadList.jsm#161-176 So the DownloadHistoryList that has DownloadList proto should have a custom removeView method: https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/toolkit/components/downloads/DownloadHistory.jsm#428-429 Similar to how DownloadsIndicatorDataCtor calls its prototype's removeView before doings its own stuff: https://searchfox.org/mozilla-central/source/browser/components/downloads/DownloadsCommon.jsm#1097-1098

Comment on attachment 8969786 [details] Bug 1455737 - Remove the result object from DownloadHistory when removing the view https://reviewboard.mozilla.org/r/238618/#review244342 ::: toolkit/components/downloads/DownloadHistory.jsm:466 (Diff revision 3) > + removeView(aView) { > + DownloadList.prototype.removeView.call(this, aView); > + > + // Clean up any active results that might still be observing. See bug 1455737 > + this.result = null; > + }, nit: newline after !

Pushed by usarracini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40c5d3600d35 Remove the result object from DownloadHistory when removing the view r=Mardak

Thanks for finding this bug, but the fix is incorrect and must be backed out because it breaks DownloadHistoryList when more than one view is registered, and landed without a regression test. It looks like I can't push the backout to autoland myself, it would be nice if a sheriff could do it before merging, otherwise I'll land a revert after the code reaches mozilla-central. This would give us some more time to land a proper fix with a regression test, and of course if a follow-up is ready sooner we can just land it additionally. Ed, thanks for doing the first pass review here, appreciated! Even if this review is formally enough, before landing it's still better to ask someone familiar with the sub-module for a rubberstamp when the patch looks good to land.

We need a separate test that adds two views to the same list, and we can either just reach into the objects and check that the history result was closed only when unregistering the second view, or alternatively add a history download after unregistering one of the views, and checking that the other still gets a result. The test should be added in this file: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_DownloadHistory.js The current assumption throughout the existing tests is that views on DownloadHistoryList don't have to be unregistered. If this assumption changes, we also have to update the existing test to match each addView call with a removeView call.

Pushed by aciure@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b03b4bee868e Remove the result object from DownloadHistory when removing the view r=Mardak on a CLOSED TREE

(In reply to :Paolo Amadini from comment #10) > https://hg.mozilla.org/integration/mozilla-inbound/rev/cf89da716c0b This backout caused memory leaks: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=14cd9f1d6365f5a5e4c18ff8527012e45a39c9cd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=174929326&filter-searchStr=Linux+x64+asan+Mochitests+test-linux64-asan%2Fopt-mochitest-chrome-3+M%28c3%29 https://treeherder.mozilla.org/logviewer.html#?job_id=174929326&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/cf89da716c0b was backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b03b4bee868e467fa527a54b7867daea1902a97d

Most probably this is because bug 1433230 also landed in the meantime, preventing the backout. We should probably revert bug 1433230 first and this one afterwards, however the other bug is linked to a GitHub issue so I don't know the workflow for performing a backout in this situation. Hopefully one of Andrei, Ed, or Ursula can help?

Bug 1433230 landed as part of bug 1455216 export. I have a fix to back out the original change here and fix the leak that prevented the attempted backout while also adding a test to make sure we don't regress multiple views again. I also made sure that bug 1448081's tests pass too.

Comment on attachment 8969951 [details] Bug 1455737 - Clean up DownloadHistory history observer on shutdown to prevent leaks. https://reviewboard.mozilla.org/r/238732/#review244462 Looks great, thanks for finding this solution and updating the test! ::: toolkit/components/downloads/test/unit/test_DownloadHistory.js:267 (Diff revision 1) > + // 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); > + dummyList.removeView(dummyView); Use "await" for removeView too.

Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b154d7186790 Clean up DownloadHistory history observer on shutdown to prevent leaks. r=Paolo