Closed Bug 1226185 Opened 6 years ago Closed 6 years ago

Fix leaks in about:debugging

Categories

(DevTools :: about:debugging, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 1 obsolete file)

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!
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)
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 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 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+
> 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 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-
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+
Attachment #8689579 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/43de993bc09b
https://hg.mozilla.org/mozilla-central/rev/84c4ab45a1eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.