If you look at the implementation for Panel and Page Workers, a circular reference is created with the instance and a view in order to prevent the instance from being GC-able, because if it were GC-able then the views would be leaked. This is too confusing imo, very prone to mistakes, and furthermore it is not mentioned in the JEP for Disposables..

Note the counter-intuitive notion that a circular references is required in order to prevent a leak..

Pointer to Github pull-request

replied in pull

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #3) > replied in pull copied from https://github.com/mozilla/addon-sdk/pull/1284#discussion_r7670922 "I'd be down with swapping a default, if that makes a difference. In that case we'll need a better name though." -@gozala

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #4) > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment > #3) > > replied in pull > > copied from > https://github.com/mozilla/addon-sdk/pull/1284#discussion_r7670922 > > "I'd be down with swapping a default, if that makes a difference. In that > case we'll need a better name though." -@gozala Hmm I just remembered that I have written modules which use Disposable so there are certainly add-ons that exist in the wild that will leak (until the addon unloads) if we change the default here.

Pointer to Github pull-request

Comment on attachment 833391 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1295 I'll write tests if I get f+

Comment on attachment 833391 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1295 Erik I thought we agreed that only thing that need to change was one line of code in current implementation of disposables. Which is basically changing given line: https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/core/disposable.js#L33 To something like: disposables.set(instance, handler, !!instance.isWeak); Instead you insist on introducing a whole new class `Destroyable`. If you changed your mind since we talked, please make sure to communicate a reasons, otherwise I can see this becoming frustrating for both of us.

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #8) > Comment on attachment 833391 [details] > Pointer to Github pull request: > https://github.com/mozilla/addon-sdk/pull/1295 > > Erik I thought we agreed that only thing that need to change was one line of > code in current implementation of disposables. Which is basically changing > given line: > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/core/disposable. > js#L33 > > To something like: > > disposables.set(instance, handler, !!instance.isWeak); > > Instead you insist on introducing a whole new class `Destroyable`. If you > changed your mind since we talked, please make sure to communicate a > reasons, otherwise I can see this becoming frustrating for both of us. Irakli please read comment 5, I've provided the answer to your question already you just need to read my comments. And yes it is frustrating when comments are not read.

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #9) > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment > #8) > > Comment on attachment 833391 [details] > > Pointer to Github pull request: > > https://github.com/mozilla/addon-sdk/pull/1295 > > > > Erik I thought we agreed that only thing that need to change was one line of > > code in current implementation of disposables. Which is basically changing > > given line: > > > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/core/disposable. > > js#L33 > > > > To something like: > > > > disposables.set(instance, handler, !!instance.isWeak); > > > > Instead you insist on introducing a whole new class `Destroyable`. If you > > changed your mind since we talked, please make sure to communicate a > > reasons, otherwise I can see this becoming frustrating for both of us. > > Irakli please read comment 5, I've provided the answer to your question > already you just need to read my comments. And yes it is frustrating when > comments are not read. I'm sorry I have not noticed that comment. (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5) > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #4) > > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment > > #3) > > > replied in pull > > > > copied from > > https://github.com/mozilla/addon-sdk/pull/1284#discussion_r7670922 > > > > "I'd be down with swapping a default, if that makes a difference. In that > > case we'll need a better name though." -@gozala > > Hmm I just remembered that I have written modules which use Disposable so > there are certainly add-ons that exist in the wild that will leak (until the > addon unloads) if we change the default here. Alright, let's not switch the default then & introduce optional field that let's one opt-out from automatic GC behavior as suggested in pull request. If you insist on new class, fine, I give up. Never the less, we should avoid completely new implementation instead we should still do introduce field as suggested in Comment 8 and new class can just override that default: const Disposable = Class({ implements: [require("sdk/core/disposable").Disposable], isWeak: false }) I think it would be best if new Disposable would live under `sdk/ui/disposable`. So it will be more or less if we switched the default.

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/257646436ab05f1243ff2001bb08999d700a93f3 Merge pull request #1443 from Gozala/bug/disposables@892381 Bug 892381 - Implement Disposables that can't be GC-ed. r=@erikvold

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/a35dc50b9d7b7d9890aefa25920ff0cfa0dfd24d Merge pull request #1445 from Gozala/bug/disposables-fix@892381 Bug 892381 - Implement dispose method on PageMode that is inoved on unload. r=@zer0