Closed Bug 1285373 Opened 3 years ago Closed 3 years ago

unloader.js when() should support weak references

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: bkelly, Unassigned)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04c8b20bc67
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 bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/01793fbeacfd
https://hg.mozilla.org/mozilla-central/rev/8aa72f37debb
https://hg.mozilla.org/mozilla-central/rev/2d1b1ed6f563
Status: ASSIGNED → RESOLVED
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.

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?
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.
Status: RESOLVED → REOPENED
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.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.