Closed Bug 1251892 Opened 4 years ago Closed 4 years ago

EventManager unregister code never runs after last listener is removed

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(1 file)

No description provided.
Attachment #8724489 - Flags: review?(lgreco) → review+
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)
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.
https://hg.mozilla.org/mozilla-central/rev/2dc35ef10107
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.