Closed
Bug 1226185
Opened 9 years ago
Closed 9 years ago
Fix leaks in about:debugging
Categories
(DevTools :: about:debugging, defect)
DevTools
about:debugging
Tracking
(firefox45 fixed)
RESOLVED
FIXED
Firefox 45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(2 files, 1 obsolete file)
1.04 KB,
patch
|
janx
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
janx
:
review+
|
Details | Diff | Splinter Review |
about:debugging still has no tests. I tried writing the first, but I got report of at least two leaks. We should fix them in order to have tests!
Assignee | ||
Comment 1•9 years ago
|
||
Services.pref.addObserver was leaking the scope function passed to it.
itself keeping about:debugging alive.
The fix end up complexifying the checkboxes quite a bit
and I think moving them to full react would make it easier at the end.
Attachment #8689579 -
Flags: review?(janx)
Assignee | ||
Comment 2•9 years ago
|
||
componentWillUnmount wasn't called on tab close,
so that AddonManager.removeListener wasn't either.
And the AddonManager end up keeping about:debugging alive via this react object.
Again, being full react would help here, we would only have to unmount the root element.
Attachment #8689582 -
Flags: review?(janx)
Comment 3•9 years ago
|
||
Comment on attachment 8689579 [details] [diff] [review]
Fix preferences listener leak in about:debugging.
Review of attachment 8689579 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Services.pref.addObserver was leaking the scope function passed to it.
> itself keeping about:debugging alive.
Thanks for addressing this!
> The fix end up complexifying the checkboxes quite a bit
> and I think moving them to full react would make it easier at the end.
Well, since everybody wants to switch the checkboxes to React, I guess that will happen eventually. My current PrefComponent prototype is 50+ lines of React, but I'd like to make it generic for boolean/string/int prefs to justify such verbosity.
Regardless, I think there is an easier way to fix the issue at hand, and I'd like to keep the `updateCheckbox()` method contained:
You could create a `this._prefListeners` object on `AboutDebugging`, then in the mapped function add `this._prefListeners[pref] = updateCheckbox`, and in `destroy()` iterate over `_prefListeners`?
Attachment #8689579 -
Flags: review?(janx) → feedback+
Comment 4•9 years ago
|
||
Comment on attachment 8689582 [details] [diff] [review]
Ensure to unmount about:debugging components on tab close
Review of attachment 8689582 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> componentWillUnmount wasn't called on tab close,
> so that AddonManager.removeListener wasn't either.
> And the AddonManager end up keeping about:debugging alive via this react
> object.
Thanks a lot for catching that.
> Again, being full react would help here, we would only have to unmount the
> root element.
That's something I've considered as well. If you feel like this would be a huge win for about:debugging, I'd be OK with going full React.
I just think that for simple things like a few headers, links, buttons or checkboxes, React might be a bit overkill.
Attachment #8689582 -
Flags: review?(janx) → review+
Comment 5•9 years ago
|
||
> You could create a `this._prefListeners` object on `AboutDebugging`, then in
> the mapped function add `this._prefListeners[pref] = updateCheckbox`, and in
> `destroy()` iterate over `_prefListeners`?
An array of `{pref,updateCheckbox}` objects could work too if you prefer.
Comment 6•9 years ago
|
||
Comment on attachment 8689579 [details] [diff] [review]
Fix preferences listener leak in about:debugging.
Review of attachment 8689579 [details] [diff] [review]:
-----------------------------------------------------------------
Changing to r- because of a problem in the code, but please use the `_prefListeners` approach I suggested in comment 3.
::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +76,3 @@
> element.addEventListener("change", updatePref, false);
> + Services.prefs.addObserver(pref, this.updateCheckbox, false);
> + this.updateCheckbox(pref);
That calls won't work because your proposed `this.updateCheckbox` expects 3 parameters.
Attachment #8689579 -
Flags: feedback+ → review-
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8690832 -
Flags: review?(janx)
Comment 8•9 years ago
|
||
Comment on attachment 8690832 [details] [diff] [review]
Fix preferences listener leak in about:debugging - v2
Review of attachment 8690832 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, thanks!
::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +82,1 @@
> Services.prefs.addObserver(pref, updateCheckbox, false);
Nit: Maybe push the listener *after* it was successfully added as pref observer?
Attachment #8690832 -
Flags: review?(janx) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8689579 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/43de993bc09be7f3c57f374ce1944a88c19504a9
Bug 1226185 - Fix preferences listener leak in about:debugging. r=janx
https://hg.mozilla.org/integration/fx-team/rev/84c4ab45a1eb801bb6f55893a70243a0fdc849bb
Bug 1226185 - Ensure to unmount about:debugging components on tab close. r=janx
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/43de993bc09b
https://hg.mozilla.org/mozilla-central/rev/84c4ab45a1eb
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•