Closed
Bug 1330954
Opened 4 years ago
Closed 4 years ago
Fix leakiness of shield system add-on
Categories
(Firefox :: Normandy Client, defect)
Firefox
Normandy Client
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 hidden (mozreview-request) |
Comment 2•4 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Comment 4•4 years ago
|
||
mozreview-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
Comment 6•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a6b1db06ced9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment 7•4 years ago
|
||
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
Updated•3 years ago
|
Product: Shield → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•