Closed
Bug 886148
Opened 11 years ago
Closed 11 years ago
Disposables are sometimes not disposed.
Categories
(Add-on SDK Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: evold, Unassigned)
References
Details
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
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(rFobic)
Priority: -- → P2
Reporter | ||
Comment 1•11 years ago
|
||
Since Buttons and Sidebars are using Disposables I think this deserves to be P1..
Flags: needinfo?(dtownsend+bugmail)
Reporter | ||
Comment 2•11 years ago
|
||
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..
Flags: needinfo?(zer0)
Reporter | ||
Comment 3•11 years ago
|
||
It looks like using disposables for Panels is a bad idea too, see bug 888688
Comment 4•11 years ago
|
||
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
Flags: needinfo?(dtownsend+bugmail)
Priority: P2 → --
Reporter | ||
Comment 5•11 years ago
|
||
(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.
Reporter | ||
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
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.
Flags: needinfo?(rFobic)
Reporter | ||
Comment 8•11 years ago
|
||
(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.
Reporter | ||
Comment 9•11 years ago
|
||
(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.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(zer0)
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•