need a easier to use alternative to system/events api

RESOLVED WONTFIX

Status

Add-on SDK
General
P3
normal
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: erikvold, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

From what I understand add-on devs are supposed to use system/events now instead of observer-service now, but the two apis work very differently, the former requires far deeper understanding because it requires awareness of GC and weak refs which most web devs do not have or can easily forget.

I think most people on our team have stepped in this trap and wrote code that was either gc'd too early or code that leaks because of this.

The reason I keep hearing for this change is that it makes the unload time faster, which hasn't been measured (tho I agree with the theory) so we have no idea what the true value is, and it is glaringly obvious that our code is far more complex to read now because of this api.

I think we should learn from the system/events and observer-service module and create a new easy to use module where one does not need to think about GC most importantly.
Flags: needinfo?(rFobic)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #0)
> From what I understand add-on devs are supposed to use system/events now
> instead of observer-service now, but the two apis work very differently, the
> former requires far deeper understanding because it requires awareness of GC
> and weak refs which most web devs do not have or can easily forget.
>

These modules are not high level APIs and I don't think users should mess with them unless they understand how they work. Yes weak refs and weak maps are very
new abilities that JS engines started to expose and they may be little hard to grok at first, but I'm sure after team get's more experience we'll establish
patterns that would help us.


> 
> I think most people on our team have stepped in this trap and wrote code
> that was either gc'd too early or code that leaks because of this.
>

I'm sure we'll get better at this, once we get more experience.
 
>
> The reason I keep hearing for this change is that it makes the unload time
> faster, which hasn't been measured (tho I agree with the theory) so we have
> no idea what the true value is, and it is glaringly obvious that our code is
> far more complex to read now because of this api.
>

1. Doing nothing is faster than doing some things, although I agree measuring
   how much we gain is useful. Either way it's proportional to amount of,
   listeners that is proportional to amount of tabs, panels, etc... Which will
   be down to 0 regardless of amount of tabs / panels, etc...

2. I don't agree it's far more complex, it's matter of associating listener with
   a target it's going to update. If that's module level then just create listener
   in the module scope. If it's associated with some promise / api object
   associate listener with it, using weak map or direct reference. 

> 
> I think we should learn from the system/events and observer-service module
> and create a new easy to use module where one does not need to think about
> GC most importantly.

I'm open to ideas if you have them, but right now it's easiest to think less about GC and more about what listener relates to, which is either some return value and you can associate it with like this:

https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/event/utils.js#L31-L34


Or it's relates to the module functionality in which case you just don't inline
listener and define it in the module scope instead:

https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/panel/events.js#L19-L20


This is just logical connection of things. Doing manual cleanups and keeping things in memory for the lifetime of the add-on is not a good idea. I'm sure with
more experience this will make more sense and we'll also end with some utility modules to make this even easier.
Flags: needinfo?(rFobic)
Created attachment 738128 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/946

Pointer to Github pull-request
Comment on attachment 738128 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/946

This needs tests still, and I haven't even tried it yet so there maybe bugs, but I just want to get an api review first.
Attachment #738128 - Flags: review?(rFobic)
Summary: system/events api is a problem → need a easier to use alternative to system/events api
Assignee: nobody → evold
Comment on attachment 738128 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/946

I disagree with proposal as it chooses to optimize amount of thinking required over smaller memory footprint or faster unloads. I have pointed out in more details why
this is a bad idea: https://gist.github.com/Gozala/5399293
Attachment #738128 - Flags: review?(rFobic) → review-
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0) 
> This is just logical connection of things. Doing manual cleanups and keeping
> things in memory for the lifetime of the add-on is not a good idea.

There are many use cases when this is exactly what is needed, and having an easy to use api would be nice instead of having to reinvent the wheel every time we need one.

Here are some common issues with using system/events:

1. Adding a listener that sticks around until my add-on unloads.

With system/events I have to do this:

    require('sdk/system/events').on('event', function() {});

Wait that doesn't work, it gets gc'd too early.. lemme try this:

    function listener() {}
    require('sdk/system/events').on('event', listener);

Wait that doesn't work, it gets gc'd too late (after my add-on unloads), lemme try:

    function listener() {}
    require('sdk/system/events').on('event', listener);
    require('unload').when(function() require('sdk/system/events').off('event', listener));

There we go, that is how it's done with system/events.

With system/observer, do this:

    require('sdk/system/observer').on('event', function() {});

2.  Add a listener that is removed once it hears the first event, or on unload:

With system/events:

    require('sdk/system/events').on('event', function listener() {
      require('sdk/system/events').off('event', listener)
    });

Oops wait that won't be removed on unload, it is gc'd after unload, hmm using once will have the same issues too.. let me try this:

    function listener() {
      require('sdk/system/events').off('event', listener);
    }
    require('sdk/system/events').on('event', listener);
    require('unload').when(function() require('sdk/system/events').off('event', listener));

Darn now there is a ref to `listener` until my add-on unloads.. with system/events which isn't suppose to do that.. hmmmmm, is this even possible?  Ahh yes it is! like this:

    let unloaded = false;
    function listener() {
      require('sdk/system/events').off('event', listener);
      if (unloaded) return;
    }
    require('sdk/system/events').on('event', listener);
    require('unload').when(function() unloaded = true);

That was easy :/

With system/observer, do this:
    
    require('sdk/system/events').once('event', function listener() {});

Done.

Updated

5 years ago
Flags: needinfo?(dtownsend+bugmail)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #1)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0) 
> > This is just logical connection of things. Doing manual cleanups and keeping
> > things in memory for the lifetime of the add-on is not a good idea.
> 
> There are many use cases when this is exactly what is needed, and having an
> easy to use api would be nice instead of having to reinvent the wheel every
> time we need one.
> 
> Here are some common issues with using system/events:
> 
> 1. Adding a listener that sticks around until my add-on unloads.
> 
> With system/events I have to do this:
> 
>     require('sdk/system/events').on('event', function() {});
> 
> Wait that doesn't work, it gets gc'd too early.. lemme try this:
> 
>     function listener() {}
>     require('sdk/system/events').on('event', listener);
> 
> Wait that doesn't work, it gets gc'd too late (after my add-on unloads),
> lemme try:

If that is the case, open a bug and we'll have to fix that. That should work, just fine.
As I suggested on the IRC we could probably simplify system/events to address specific issues you're having. It seems like following will solve most of the issues:


on("event-name", function() {
  // ...
}, module);

That will keep listener alive for the lifetime of the module / add-on.


function Class() {
  on("event-name", function() {
    // ...
  }, this);
}

This will keep listener alive for the lifetime of the instance.

on("event-name", function() {
  // ...
}, true);


This will keep strong ref to a listener and it manually needs to
be removed.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #1)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0) 
> > This is just logical connection of things. Doing manual cleanups and keeping
> > things in memory for the lifetime of the add-on is not a good idea.
> 
> There are many use cases when this is exactly what is needed, and having an
> easy to use api would be nice instead of having to reinvent the wheel every
> time we need one.
> 
> Here are some common issues with using system/events:
> 
> 1. Adding a listener that sticks around until my add-on unloads.
> 
> With system/events I have to do this:
> 
>     require('sdk/system/events').on('event', function() {});

Doesn't this work?

    require('sdk/system/events').on('event', function() {}, true);

> Wait that doesn't work, it gets gc'd too early.. lemme try this:
> 
>     function listener() {}
>     require('sdk/system/events').on('event', listener);
> 
> Wait that doesn't work, it gets gc'd too late (after my add-on unloads),

Too late? Is that actually a problem as long as it gets gc'd?

> lemme try:
> 
>     function listener() {}
>     require('sdk/system/events').on('event', listener);
>     require('unload').when(function()
> require('sdk/system/events').off('event', listener));

If system/events isn't automatically removing listeners from the observer service when the add-on is unloaded then that is a bug.

> There we go, that is how it's done with system/events.
> 
> With system/observer, do this:
> 
>     require('sdk/system/observer').on('event', function() {});
> 
> 2.  Add a listener that is removed once it hears the first event, or on
> unload:
> 
> With system/events:
> 
>     require('sdk/system/events').on('event', function listener() {
>       require('sdk/system/events').off('event', listener)
>     });
> 
> Oops wait that won't be removed on unload, it is gc'd after unload, hmm
> using once will have the same issues too.. let me try this:

Yeah we should fix the remove on unload then you can just use once for this.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Dave Townsend (:Mossop) from comment #8)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > comment #1)
> > > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0) 
> > > This is just logical connection of things. Doing manual cleanups and keeping
> > > things in memory for the lifetime of the add-on is not a good idea.
> > 
> > There are many use cases when this is exactly what is needed, and having an
> > easy to use api would be nice instead of having to reinvent the wheel every
> > time we need one.
> > 
> > Here are some common issues with using system/events:
> > 
> > 1. Adding a listener that sticks around until my add-on unloads.
> > 
> > With system/events I have to do this:
> > 
> >     require('sdk/system/events').on('event', function() {});
> 
> Doesn't this work?

This does work it's just since listener is inlined nothing else can refer to it there for it will be GC-ed immediately.

> 
>     require('sdk/system/events').on('event', function() {}, true);

This will work, although listener will not be automatically removed, even on unload. Now if that is a problem we can modify implementation such that internally still weak ref will be used, but we'll map in weak map module to
a listener, this way it will be GC-ed once add-on is unloaded.

> 
> > Wait that doesn't work, it gets gc'd too early.. lemme try this:
> > 
> >     function listener() {}
> >     require('sdk/system/events').on('event', listener);
> > 
> > Wait that doesn't work, it gets gc'd too late (after my add-on unloads),
> 
> Too late? Is that actually a problem as long as it gets gc'd?
>

It could be a problem if that is a case. Erik if you can reproduce-able case for this let's make a separate bug and we should be able to fix that.

> 
> > lemme try:
> > 
> >     function listener() {}
> >     require('sdk/system/events').on('event', listener);
> >     require('unload').when(function()
> > require('sdk/system/events').off('event', listener));
> 
> If system/events isn't automatically removing listeners from the observer
> service when the add-on is unloaded then that is a bug.
>

It does not, but only if third argument is `true` (which should probably never be). That could be fixed as pointed out above, although I'm more tempted to remove third argument entirely. 

> 
> > There we go, that is how it's done with system/events.
> > 
> > With system/observer, do this:
> > 
> >     require('sdk/system/observer').on('event', function() {});
> > 
> > 2.  Add a listener that is removed once it hears the first event, or on
> > unload:
> > 
> > With system/events:
> > 
> >     require('sdk/system/events').on('event', function listener() {
> >       require('sdk/system/events').off('event', listener)
> >     });
> > 
> > Oops wait that won't be removed on unload, it is gc'd after unload, hmm
> > using once will have the same issues too.. let me try this:
> 
> Yeah we should fix the remove on unload then you can just use once for this.


Erik also please note that all the new code does use different pattern of one top level event listener that grabs our API object by event.target and does things to it. It's GC-ed at unload and no removal is even required.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #7)
> As I suggested on the IRC we could probably simplify system/events to
> address specific issues you're having. It seems like following will solve
> most of the issues:
> 
> 
> on("event-name", function() {
>   // ...
> }, module);
> 
> That will keep listener alive for the lifetime of the module / add-on.
> 
> 
> function Class() {
>   on("event-name", function() {
>     // ...
>   }, this);
> }
> 
> This will keep listener alive for the lifetime of the instance.
> 
> on("event-name", function() {
>   // ...
> }, true);
> 
> 
> This will keep strong ref to a listener and it manually needs to
> be removed.

all of these examples include a 3rd argument which is nothing like addEventListener of the web, I want an easy to use api signature, not a complex one.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #9)
> (In reply to Dave Townsend (:Mossop) from comment #8)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > > comment #1)
> > > > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0) 
> > > > This is just logical connection of things. Doing manual cleanups and keeping
> > > > things in memory for the lifetime of the add-on is not a good idea.
> > > 
> > > There are many use cases when this is exactly what is needed, and having an
> > > easy to use api would be nice instead of having to reinvent the wheel every
> > > time we need one.
> > > 
> > > Here are some common issues with using system/events:
> > > 
> > > 1. Adding a listener that sticks around until my add-on unloads.
> > > 
> > > With system/events I have to do this:
> > > 
> > >     require('sdk/system/events').on('event', function() {});
> > 
> > Doesn't this work?
> 
> This does work it's just since listener is inlined nothing else can refer to
> it there for it will be GC-ed immediately.

isn't that worse than not working at all?

> > 
> >     require('sdk/system/events').on('event', function() {}, true);
> 
> This will work, although listener will not be automatically removed, even on
> unload. Now if that is a problem we can modify implementation such that
> internally still weak ref will be used, but we'll map in weak map module to
> a listener, this way it will be GC-ed once add-on is unloaded.

That just adds complexity to the api imo.

> > > Wait that doesn't work, it gets gc'd too early.. lemme try this:
> > > 
> > >     function listener() {}
> > >     require('sdk/system/events').on('event', listener);
> > > 
> > > Wait that doesn't work, it gets gc'd too late (after my add-on unloads),
> > 
> > Too late? Is that actually a problem as long as it gets gc'd?
> >
> 
> It could be a problem if that is a case. Erik if you can reproduce-able case
> for this let's make a separate bug and we should be able to fix that.

Yes this is a problem, the pattern can easily cause leaks.  See use case above, just add an emit of 'event' during unload.

> > > lemme try:
> > > 
> > >     function listener() {}
> > >     require('sdk/system/events').on('event', listener);
> > >     require('unload').when(function()
> > > require('sdk/system/events').off('event', listener));
> > 
> > If system/events isn't automatically removing listeners from the observer
> > service when the add-on is unloaded then that is a bug.
> >
> 
> It does not, but only if third argument is `true` 

Where does it do that?

> (which should probably
> never be). That could be fixed as pointed out above, although I'm more
> tempted to remove third argument entirely. 

Sounds good to me, it just seems to add needless complexity.

> > > There we go, that is how it's done with system/events.
> > > 
> > > With system/observer, do this:
> > > 
> > >     require('sdk/system/observer').on('event', function() {});
> > > 
> > > 2.  Add a listener that is removed once it hears the first event, or on
> > > unload:
> > > 
> > > With system/events:
> > > 
> > >     require('sdk/system/events').on('event', function listener() {
> > >       require('sdk/system/events').off('event', listener)
> > >     }, true);
> > > 
> > > Oops wait that won't be removed on unload, it is gc'd after unload, hmm
> > > using once will have the same issues too.. let me try this:
> > 
> > Yeah we should fix the remove on unload then you can just use once for this.
> 
> 
> Erik also please note that all the new code does use different pattern of
> one top level event listener that grabs our API object by event.target and
> does things to it. It's GC-ed at unload and no removal is even required.

The first sentence I do not understand, and the second one seems to contradict what you've said previously in this thread.

Updated

5 years ago
Priority: -- → P3
Assignee: evold → nobody
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> Comment on attachment 738128 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/946
> 
> I disagree with proposal as it chooses to optimize amount of thinking
> required over smaller memory footprint or faster unloads. I have pointed out
> in more details why
> this is a bad idea: https://gist.github.com/Gozala/5399293

Where is the "common pattern" mentioned here used in the SDK on a class without a destroy method?
Priority: P3 → --
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #12)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #4)
> > Comment on attachment 738128 [details]
> > Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/946
> > 
> > I disagree with proposal as it chooses to optimize amount of thinking
> > required over smaller memory footprint or faster unloads. I have pointed out
> > in more details why
> > this is a bad idea: https://gist.github.com/Gozala/5399293
> 
> Where is the "common pattern" mentioned here used in the SDK on a class
> without a destroy method?

ie where calling the destroy method is not required to do other cleanups? (like removing views)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0)
> > I think most people on our team have stepped in this trap and wrote code
> > that was either gc'd too early or code that leaks because of this.
> >
> 
> I'm sure we'll get better at this, once we get more experience.

or we'll introduce leaks like bug 892341..

Updated

5 years ago
Flags: needinfo?(rFobic)
Priority: -- → P3
For example add-on's hidden window is removed entirely on unload which includes all the iframes and add-on dom with it. As of DOM elements I would rather have a different API for registering DOM elements that can be removed from tree on add-on unload without doing that manually per API.
Flags: needinfo?(rFobic)
docs are going to mdn, this bug is pointless now.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.