892381 - Implement Disposable type who's instances aren't GC-ed.

Status: RESOLVED FIXED

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

Description

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

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..

Comment 1

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

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

Comment 2

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1284

Pointer to Github pull-request

Comment 3

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

replied in pull

Comment 4

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

(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

Comment 5

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

(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.

Comment 6

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1295

Pointer to Github pull-request

Comment 7

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

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 8

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

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.

Comment 9

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

(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.

Comment 10

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

(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.

Comment 11

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

Attachment: Implement disposables that can't be GC-ed

Comment 12

[[github robot]]

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

Comment 13

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

Attachment: Regression fix on page-mods

Comment 14

[[github robot]]

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