807382 - system/events should remove all alive listeners on unload

Status: RESOLVED FIXED

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

Description

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

afaict an observer added via the system/events module is not automatically removed when the loader unloads, and I can't think of a reason why it would not.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Note that because this behavior, the new selection module (Bug 803027) needs to keep manually trace of the observer added via the system/events. As soon as this bug is fixed, that code should be removed.

Comment 2

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

I had a little conversation on this with Zero over IRC. As this comment outlines:
https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/system/events.js#L74-75

Module system/events keeps weak reference to a listener by default, which means
that listener will never be invoked and will be garbage collected if nothing else
holds a reference. This also means that module does not needs to keep references
of all the registered listeners to unregister them at unload. In addition weak listeners don't even need to be unregistered if nothing else holds their reference.

So only reason why some listeners could be invoked after unload is if the reference to them is kept after unload, which should not be happening. If it does that means we leek memory.

That being said, I'd suggest two immediate solutions:

1. Find out what keeps the reference of listener after unload and make sure it's released so listener can be collected. And if listener forces strong reference it
should not.

2. Implement other module that would wrap system/events that will both register
listeners and make sure to unregister them at unload.

The later option is less preferable as it will use more memory and potentially will leak some too.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #2)
> I had a little conversation on this with Zero over IRC. As this comment
> outlines:
> https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/system/events.
> js#L74-75
> 
> Module system/events keeps weak reference to a listener by default, which
> means
> that listener will never be invoked and will be garbage collected if nothing
> else
> holds a reference. This also means that module does not needs to keep
> references
> of all the registered listeners to unregister them at unload. In addition
> weak listeners don't even need to be unregistered if nothing else holds
> their reference.
> 
> So only reason why some listeners could be invoked after unload is if the
> reference to them is kept after unload, which should not be happening. If it
> does that means we leek memory.

The other reason is if the developer requested a strong reference, in this case I think we should automatically clean up rather than rely on the developer to do it.

Comment 4

[Tomislav Jovanovic :zombie]

Dave, i'm a bit confused by this bug, partly because i can't find solid documentation on sandboxes and what exactly happens when they are "nuked".

can objects from inside the sandbox survive the nuking if something on the outside holds a reference to them, like the observer-service in this case?  (neither answer makes much sense to me)

if not, it seems like there is not much to be gained by removing listeners on unload if they will be destroyed when all addon's sandboxes are nuked anyway. 

and if they can (survive nuking), that means we have leak problems, and should be using weak references in more places..

Comment 5

[Dave Townsend [:mossop]]

(In reply to Tomislav Jovanovic [:zombie] from comment #4)
> Dave, i'm a bit confused by this bug, partly because i can't find solid
> documentation on sandboxes and what exactly happens when they are "nuked".
> 
> can objects from inside the sandbox survive the nuking if something on the
> outside holds a reference to them, like the observer-service in this case? 
> (neither answer makes much sense to me)

Yes ... but they may be horribly broken because they depend on closures and the global that has been nuked.

> and if they can (survive nuking), that means we have leak problems, and
> should be using weak references in more places..

Nuking is a failsafe, we should never be relying on it to stop us leaking.

Comment 6

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1438

Dave, i have two concerns with my solution.

first, i'm not sure this is the best way to test this, as it relies on objects staying alive after loader unload, but i couldn't think of a better approach (also, i verified that the test fails without my patch, so i must be doing something right! ;)

the other thing the test exposes is that with this patch, strong listeners are removed on unload, but week ones aren't, which might be a bit surprising and counter-intuitive (and we can't also remove week listeners, as we can't enumerate them because of the week refs).

Comment 7

[Dave Townsend [:mossop]]

(In reply to Tomislav Jovanovic [:zombie] from comment #6)
> Created attachment 8393818 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1438
> 
> Dave, i have two concerns with my solution.
> 
> first, i'm not sure this is the best way to test this, as it relies on
> objects staying alive after loader unload, but i couldn't think of a better
> approach (also, i verified that the test fails without my patch, so i must
> be doing something right! ;)

That's fine. The objects you are keeping alive weren't created by the loader that you unload so no problem there.

> the other thing the test exposes is that with this patch, strong listeners
> are removed on unload, but week ones aren't, which might be a bit surprising
> and counter-intuitive (and we can't also remove week listeners, as we can't
> enumerate them because of the week refs).

This is a good point and I think we should actually remove the weak listeners too. If we retain a weak reference to every listener then we can just go through, check if they have been GC'ed already and if not unregister them.

    let weakRef = Cu.getWeakReference(foo);

    // later

    let ref = weakRef.get();
    if (ref)
        // Clear the registration.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8393818 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1438

Waiting on an updated patch here

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8393818 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1438

here is the updated PR.  

i had to do a bit of WeakRef gymnastics (courtesy bug 986115), and now we keep references to observers in several ways in several places. maybe this could/should be refactored some, so we end up with one less *Map?

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8393818 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1438

Makes my head hurt but I think this is good aside from the one minor test change.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Is there any reason 'cause is not landed yet? We should merge ASAP 'cause it should fixes bug 985332, and people are complaining about those dead object exceptions.

Comment 12

[Dave Townsend [:mossop]]

Is the patch updated? I can't really tell because github

Comment 13

[Tomislav Jovanovic :zombie]

i was waiting a few days to see if there is any movement on bug 986115, to avoid the WeakRef gymnastics..

i'll ping them again today and see if they expect any progress soon.

anyway, didn't understand this was so urgent, the bug's been sitting still for a year.

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tomislav Jovanovic [:zombie] from comment #13)

> i'll ping them again today and see if they expect any progress soon.

Thanks! The bug 986115 is really annoying, but if it takes too long I would suggest to land this patch as is, and file another bug that depends by bug 986115 to remove the WeakRef gymnastics.

> anyway, didn't understand this was so urgent, the bug's been sitting still
> for a year.

It's because all the dead object exceptions we have now. Probably before we didn't use so heavy the system/events, and probably also due some platform changes; I can't tell. However, it seems that most of those exceptions, if not all of them, are because we rely too much on weakref of system/events, that is not gc'ed as fast as some code expects. Of course we could patch manually all the places where that's happening, but it's probably better – and faster – fix this bug.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #14)
> (In reply to Tomislav Jovanovic [:zombie] from comment #13)
> 
> > i'll ping them again today and see if they expect any progress soon.
> 
> Thanks! The bug 986115 is really annoying, but if it takes too long I would
> suggest to land this patch as is, and file another bug that depends by bug
> 986115 to remove the WeakRef gymnastics.

We should land this anyway. If bug 986115 lands we get to remove some cleanup code

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

Tomislav, do you have the access in github to land this patch? Otherwise I will merge for you; let me know. Thanks!

Comment 17

[Tomislav Jovanovic :zombie]

i need to update one test, and run them again. be done in a bit..

Comment 18

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4f7b9af53a66001fdbdf949f6f0e7b010bd6b0f0
bug 807382 - remove alive listeners on unload

https://github.com/mozilla/addon-sdk/commit/4d33cad73173c6200d3d50e94fd306e7f41f3f56
Merge pull request #1438 from zombie/807382-system-events

bug 807382 - remove alive listeners on unload