1330954 - Fix leakiness of shield system add-on

Status: RESOLVED FIXED

(Firefox :: Normandy Client, defect)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1326225 +++

After bug 1326225, the add-on is leaking browser windows when using heartbeat, and we've disabled the test that shows this. We should fix the leaks and re-enable the test.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1330954 - fix leakiness of SHIELD system add-on and re-enable test,

There are 3 issues this tackles:
- we were keeping a reference to the notification we added (this.notice), which kept windows alive.
- we had the CleanupManager keep a reference to our close method, which kept a reference to us, which kept a reference to the window.
- the fact that we use timeouts to call this.close() in 2 places meant this.close would get multiple times, which meant we errored out on later occasions, because various things had been nulled out. This tidies up the timeouts when cleanup is called to avoid re-entrancy and/or errors and/or leaks.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8826573 [details]
Bug 1330954 - fix leakiness of SHIELD system add-on and re-enable test,

https://reviewboard.mozilla.org/r/104524/#review105170

::: browser/extensions/shield-recipe-client/lib/CleanupManager.jsm:9
(Diff revision 1)
>  
>  "use strict";
>  
>  this.EXPORTED_SYMBOLS = ["CleanupManager"];
>  
> -const cleanupHandlers = [];
> +const cleanupHandlers = new Set([]);

Nit: The argument isn't necessary. Just `new Set()` should do the same.

::: browser/extensions/shield-recipe-client/lib/CleanupManager.jsm:22
(Diff revision 1)
> +    cleanupHandlers.delete(handler);
>    },
>  
>    cleanup() {
>      for (const handler of cleanupHandlers) {
>        handler();

While we're here, can we add a try-catch around the handler calls?

::: browser/extensions/shield-recipe-client/lib/Heartbeat.jsm:211
(Diff revision 1)
>        this.maybeNotifyHeartbeat("SurveyExpired");
>        this.close();
>      }, surveyDuration);
>  
>      this.sandboxManager.addHold("heartbeat");
> -    CleanupManager.addCleanupHandler(() => this.close());
> +    CleanupManager.addCleanupHandler(this.close);

I think we're going to need something like `this.close = this.close.bind(this)` in the constructor for this to work as expected.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8826573 [details]
Bug 1330954 - fix leakiness of SHIELD system add-on and re-enable test,

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

Comment 4

[Michael Cooper [:mythmon]]

Comment on attachment 8826573 [details]
Bug 1330954 - fix leakiness of SHIELD system add-on and re-enable test,

https://reviewboard.mozilla.org/r/104524/#review105296

Thanks for tracking this down.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6b1db06ced9
fix leakiness of SHIELD system add-on and re-enable test, r=kmag,mythmon

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/0082806a04a7565aea9c66c7f838f06848eb6733
Bug 1330954 - fix leakiness of SHIELD system add-on and re-enable test