Clean up DownloadHistory history observer on shutdown to prevent leaks

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ursula, Assigned: Mardak)

Tracking

57 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

a year ago
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
Assignee

Updated

a year ago
Component: Activity Streams: Newtab → Downloads Panel
Reporter

Updated

a year ago
Attachment #8969786 - Flags: review?(edilee)
Comment hidden (mozreview-request)
Assignee

Comment 3

a year ago
mozreview-review
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
Attachment #8969786 - Flags: review?(edilee) → review-
Comment hidden (mozreview-request)
Assignee

Comment 5

a year ago
mozreview-review
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 !
Attachment #8969786 - Flags: review?(edilee) → review+
Comment hidden (mozreview-request)

Comment 7

a year ago
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

Comment 8

a year ago
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.
Flags: needinfo?(usarracini)
Flags: needinfo?(ryanvm)

Updated

a year ago
Component: Downloads Panel → Downloads API
Product: Firefox → Toolkit

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40c5d3600d35
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 10

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf89da716c0b
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---

Comment 11

a year ago
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.

Comment 12

a year ago
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

Comment 14

a year ago
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?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(edilee)
Flags: needinfo?(aciure)
Flags: needinfo?(aciure)

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b03b4bee868e
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Assignee

Comment 16

a year ago
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.
Assignee: usarracini → edilee
Blocks: 1448081
Status: RESOLVED → REOPENED
Flags: needinfo?(usarracini)
Flags: needinfo?(edilee)
Resolution: FIXED → ---
Summary: Remove the result object from DownloadHistory when removing the view → Clean up DownloadHistory history observer on shutdown to prevent leaks
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
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.
Attachment #8969951 - Flags: review?(paolo.mozmail) → review+

Comment 19

a year ago
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
Comment hidden (mozreview-request)

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b154d7186790
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.