1251892 - EventManager unregister code never runs after last listener is removed

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1251892: Fix EventManager cleanup code. r?rpl

Review commit: https://reviewboard.mozilla.org/r/37043/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37043/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8724489 [details]
MozReview Request: Bug 1251892: Fix EventManager cleanup code. r?rpl

https://reviewboard.mozilla.org/r/37043/#review33685

looks good! (and checking the size of the callbacks set is definitely less error prone that the previous "set/check |this.registered| boolean" method).

::: toolkit/components/extensions/ExtensionUtils.jsm:532
(Diff revision 1)
> +    if (this.callbacks.size) {

Not an issue but: how about about clearing the |this.callbacks| set as well on |close|?

(the context has been closed and so it should not make any real difference, but the extension could be still alive, it shouldn't be wrong to remove the remaining dead objects still referenced inside the callback set)

Comment 3

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/37043/#review33685

> Not an issue but: how about about clearing the |this.callbacks| set as well on |close|?
> 
> (the context has been closed and so it should not make any real difference, but the extension could be still alive, it shouldn't be wrong to remove the remaining dead objects still referenced inside the callback set)

Yeah, that makes sense.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/2dc35ef10107dba0e719c458cbc6bf4a6de70835
Bug 1251892: Fix EventManager cleanup code. r=rpl

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dc35ef10107