1226185 - Fix leaks in about:debugging

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Alexandre Poirot [:ochameau]]

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!

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Fix preferences listener leak in about:debugging.

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.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Ensure to unmount about:debugging components on tab close

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.

Comment 3

[Jan Keromnes [: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`?

Comment 4

[Jan Keromnes [:janx]]

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.

Comment 5

[Jan Keromnes [:janx]]

> 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

[Jan Keromnes [:janx]]

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.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Fix preferences listener leak in about:debugging - v2

Comment 8

[Jan Keromnes [:janx]]

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?

Comment 9

[Alexandre Poirot [:ochameau]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43de993bc09b
https://hg.mozilla.org/mozilla-central/rev/84c4ab45a1eb