886148 - Disposables are sometimes not disposed.

Status: RESOLVED INVALID

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

Description

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

Try this add-on https://gist.github.com/erikvold/5846016

Use `cfx run -v -o`

Open `about:memory?verbose` hit the GC and CC buttons, then Update

Open a new tab to `about:addons` and disable the add-on

Expected: `dispose` is called and therefore `'disposed!'` is logged

Actual: `dispose` is not called

Comment 1

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

Since Buttons and Sidebars are using Disposables I think this deserves to be P1..

Comment 2

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

Ugh it looks like there is an explicit test that this is how things are supposed to work, which would mean that using a Disposable for Buttons and Sidebars is a bad idea..

Comment 3

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

It looks like using disposables for Panels is a bad idea too, see bug 888688

Comment 4

[Dave Townsend [:mossop]]

I guess the only way to solve this would be to retain references to all disposables until unload making it impossible for them to be GC'ed

Comment 5

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

(In reply to Dave Townsend (:Mossop) from comment #4)
> I guess the only way to solve this would be to retain references to all
> disposables until unload making it impossible for them to be GC'ed

I think you mean just adding a strong ref for unload, and I agree.  I don't see how UI components can be GC'able.. so either that was the assumption or Disposable has a bug.

Comment 6

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Dave Townsend (:Mossop) from comment #4)
> > I guess the only way to solve this would be to retain references to all
> > disposables until unload making it impossible for them to be GC'ed
> 
> I think you mean just adding a strong ref for unload, and I agree.  I don't
> see how UI components can be GC'able.. so either that was the assumption or
> Disposable has a bug.

This test https://github.com/mozilla/addon-sdk/blob/master/test/test-disposable.js#L85

Makes me think the problem was the former.

So perhaps we should create something called Destroyable? that name makes more sense to me anyhow.

Comment 7

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

Erik panel does not has problem with disposable since our panel instance is mapped to an actual panel dom node, and if that one is one can longer be reached it's kind of useless to run any disposal.

But I clearly see how Disposables are confusing & we should fix them.

Comment 8

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

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #7)
> Erik panel does not has problem with disposable since our panel instance is
> mapped to an actual panel dom node, and if that one is one can longer be
> reached it's kind of useless to run any disposal.

Hmm I don't think this is an accurate statement, try: https://github.com/erikvold/page-portals-jetpack

Leave the portal open for a while, then try to close it.

Comment 9

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #7)
> > Erik panel does not has problem with disposable since our panel instance is
> > mapped to an actual panel dom node, and if that one is one can longer be
> > reached it's kind of useless to run any disposal.
> 
> Hmm I don't think this is an accurate statement, try:
> https://github.com/erikvold/page-portals-jetpack
> 
> Leave the portal open for a while, then try to close it.

They are very magical.. and appear to depend on a circular reference if used for a UI component.. otherwise they will leak.

Closing this bug since this appears to be intended behavior.