Closed Bug 1330954 Opened 3 years ago Closed 3 years ago

Fix leakiness of shield system add-on

Categories

(Firefox :: Normandy Client, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

+++ 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 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.
Attachment #8826573 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8826573 - Flags: review?(mcooper) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a6b1db06ced9
fix leakiness of SHIELD system add-on and re-enable test, r=kmag,mythmon
https://hg.mozilla.org/mozilla-central/rev/a6b1db06ced9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.