Closed Bug 1285373 Opened 3 years ago Closed 3 years ago

unloader.js when() should support weak references


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

Not set


(Not tracked)



(Reporter: bkelly, Unassigned)



(Whiteboard: [MemShrink:P2])


(3 files, 1 obsolete file)

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.
Whiteboard: [MemShrink]
Alternatively we could just always hold the references weakly, but I'm not sure if thats safe for all uses of unload().
Attached patch unloader.patch (obsolete) — Splinter Review
Here is my current work in progress patch.
Whiteboard: [MemShrink] → [MemShrink:P2]
Attachment #8768956 - Attachment is obsolete: true
Attachment #8771543 - Flags: review?(gkrizsanits)
This test succeeds with the P1/P2 patches and fails without them.
Attachment #8771545 - Flags: review?(gkrizsanits)
Attachment #8771543 - Flags: review?(gkrizsanits) → review+
Attachment #8771544 - Flags: review?(gkrizsanits) → review+
Attachment #8771545 - Flags: review?(gkrizsanits) → review+
Pushed by
P1 Allow addon-sdk unloader to hold callbacks weakly. r=gabor
P2 Make sdk/event/chrome use weak unload reference. r=gabor
P3 Test that sdk/event/chrome observer channels can be GC'd. r=gabor
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.

They were turned off for some reason.

Anyone have any thoughts?
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.
Blocks: 1321133
Depends on: 1288619
Depends on: 1288708
This was backed out in bug 1321133.
Resolution: FIXED → ---
Target Milestone: mozilla50 → ---
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.
Assignee: bkelly → nobody
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.
Closed: 3 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.