940247 - sdk/event/dom module does not remove event listeners causing leaks

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

sdk/event/dom can cause memory leaks because of this.. Also I made bug 940245 about there not being any tests for this module.  I'm not sure how it was r+'d.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

So the leak was introduced here https://github.com/mozilla/addon-sdk/commit/e5fd0a076823e654901120fae592cee8b2786321 for bug 854980

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Irakli can you take a look at this please?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Yes, I'm going to look into this and include you in a review.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, are you working on this one? if not could you please unassign yourself.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

Note: This may be the cause of bug 1114746 and bug 922597

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So of the module not removing event listeners, that's not an accidental. Before that change I had a conversation with platform folks who told me that event listeners registered on dom nodes could not hold a node or document from GC-ed so it was ok not to clean those up.

As of suspicion that this can be cause of bug 1114746 or bug 922597. I highly doubt that since that module isn't used (nor explicitly nor implicitly) neither by `sdk/tabs` nor `sdk/windows`.

Erik what made you conclude that this is causing leaks ?

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> So of the module not removing event listeners, that's not an accidental.
> Before that change I had a conversation with platform folks who told me that
> event listeners registered on dom nodes could not hold a node or document
> from GC-ed so it was ok not to clean those up.

So that is not the issue that I am referring to, the issue is not whether or not the sdk is holding on to dom nodes or tabs when it should not, it's whether or not the dom nodes or tabs are holding on to sdk compartments when those compartments should be gc'd.

Do you think this is not an issue too Irakli?


> As of suspicion that this can be cause of bug 1114746 or bug 922597. I
> highly doubt that since that module isn't used (nor explicitly nor
> implicitly) neither by `sdk/tabs` nor `sdk/windows`.
> 
> Erik what made you conclude that this is causing leaks ?

That might be true, I only said it may be the cause, I didn't look in to whether or not the event/dom is used by sdk/tabs or sdk/windows, I just thought they might be and made a quick note for later.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #7)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #6)
> > So of the module not removing event listeners, that's not an accidental.
> > Before that change I had a conversation with platform folks who told me that
> > event listeners registered on dom nodes could not hold a node or document
> > from GC-ed so it was ok not to clean those up.
> 
> So that is not the issue that I am referring to, the issue is not whether or
> not the sdk is holding on to dom nodes or tabs when it should not, it's
> whether or not the dom nodes or tabs are holding on to sdk compartments when
> those compartments should be gc'd.
> 
> Do you think this is not an issue too Irakli?
>

That can happen on add-on unload, in which case sandboxes and all the associated compartments are
destroyed, there for GC-ed. You may have seen occasionally "DeadObject" warnings in the console,
that what happens with listeners from such sandboxes they are replaced with DeadObject proxies. So 
no that should not be an issue either.

These are a good questions that I should have wrote answers in the comments to make intents and
expectations clear.

> 
> 
> > As of suspicion that this can be cause of bug 1114746 or bug 922597. I
> > highly doubt that since that module isn't used (nor explicitly nor
> > implicitly) neither by `sdk/tabs` nor `sdk/windows`.
> > 
> > Erik what made you conclude that this is causing leaks ?
> 
> That might be true, I only said it may be the cause, I didn't look in to
> whether or not the event/dom is used by sdk/tabs or sdk/windows, I just
> thought they might be and made a quick note for later.

Comment 9

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562