1455737 - Clean up DownloadHistory history observer on shutdown to prevent leaks

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement, P1)

Description

[Ursula Sarracini (:ursula)]

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 1

[Ursula Sarracini (:ursula)]

Attachment: Bug 1455737 - Remove the result object from DownloadHistory when removing the view

Review commit: https://reviewboard.mozilla.org/r/238618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238618/

Comment 2

[Ursula Sarracini (:ursula)]

Comment on attachment 8969786 [details]
Bug 1455737 - Remove the result object from DownloadHistory when removing the view

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238618/diff/1-2/

Comment 3

[Ed Lee :Mardak]

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 4

[Ursula Sarracini (:ursula)]

Comment on attachment 8969786 [details]
Bug 1455737 - Remove the result object from DownloadHistory when removing the view

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238618/diff/2-3/

Comment 5

[Ed Lee :Mardak]

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 !

Comment 6

[Ursula Sarracini (:ursula)]

Comment on attachment 8969786 [details]
Bug 1455737 - Remove the result object from DownloadHistory when removing the view

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238618/diff/3-4/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40c5d3600d35
Remove the result object from DownloadHistory when removing the view r=Mardak

Comment 8

[:Paolo Amadini]

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.

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/40c5d3600d35

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf89da716c0b

Comment 11

[:Paolo Amadini]

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

[Pulsebot]

Pushed by [e-mail]:
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 13

[Andrei Ciure[:aciure]]

(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

Comment 14

[:Paolo Amadini]

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?

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b03b4bee868e

Comment 16

[Ed Lee :Mardak]

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 17

[Ed Lee :Mardak]

Attachment: Bug 1455737 - Clean up DownloadHistory history observer on shutdown to prevent leaks.

Reverts broken removeView fix and add a test to make sure it doesn't regress again.

Review commit: https://reviewboard.mozilla.org/r/238732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238732/

Comment 18

[:Paolo Amadini]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b154d7186790
Clean up DownloadHistory history observer on shutdown to prevent leaks. r=Paolo

Comment 20

[Ed Lee :Mardak]

Comment on attachment 8969951 [details]
Bug 1455737 - Clean up DownloadHistory history observer on shutdown to prevent leaks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238732/diff/1-2/

Comment 21

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/b154d7186790