1285373 - unloader.js when() should support weak references

Status: RESOLVED WONTFIX

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently the unloader module's when() function holds the given observer strongly until the addon is unloaded.  This causes us to hold on to a variety of things longer than we should.

For example, event/chrome.js use unloader's when() to remove its nsIObserver.  Its using nsIObserverService's weak reference support, but its not actually working because unload() holds things alive forever.

I'd like to add a second argument unloader's when() function that lets the caller specify they want a weak reference.  Then places like event/chrome.js can use this to get the behavior they want.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Alternatively we could just always hold the references weakly, but I'm not sure if thats safe for all uses of unload().

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: unloader.patch

Here is my current work in progress patch.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow addon-sdk unloader to hold callbacks weakly. r=gabor

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make sdk/event/chrome use weak unload reference. r=gabor

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Test that sdk/event/chrome observer channels can be GC'd. r=gabor

This test succeeds with the P1/P2 patches and fails without them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04c8b20bc67

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01793fbeacfd
P1 Allow addon-sdk unloader to hold callbacks weakly. r=gabor
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa72f37debb
P2 Make sdk/event/chrome use weak unload reference. r=gabor
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1b1ed6f563
P3 Test that sdk/event/chrome observer channels can be GC'd. r=gabor

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01793fbeacfd
https://hg.mozilla.org/mozilla-central/rev/8aa72f37debb
https://hg.mozilla.org/mozilla-central/rev/2d1b1ed6f563

Comment 8

[Mike Kaply [:mkaply]]

The Amazon Assistant team is having some major issues with their add-on and they used mozregression to track it down to this. In particular there are content-script injection issues with page workers.

There are also two tests that started failing when this patch went in as well.

https://bugzilla.mozilla.org/show_bug.cgi?id=1288619
https://bugzilla.mozilla.org/show_bug.cgi?id=1288708

They were turned off for some reason.

Anyone have any thoughts?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I guess you can back out P2 and P3.  I'm not familiar with page-worker, but it must be depending on the chrome observer channel to leak in order for it to function.  It would be preferable to fix that, but I don't have time to unwind the addon-sdk dependency tree here.

Comment 10

[Mike Kaply [:mkaply]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c54bc067d7c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This was backed out in bug 1321133.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I'm not working on this at the moment.  This was never a major leak, so we could just WONTFIX it.  Since addon-sdk is deprecated long term this may be the best approach.

Comment 13

[Mike Kaply [:mkaply]]

I'm WONTFIXing based on your comment. Given what broke when we tried to fix it, and the deprecation, I don't think it's worth it.