Closed
Bug 1285373
Opened 8 years ago
Closed 7 years ago
unloader.js when() should support weak references
Categories
(Add-on SDK Graveyard :: General, defect)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: bkelly, Unassigned)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(3 files, 1 obsolete file)
2.11 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
1016 bytes,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•8 years ago
|
Whiteboard: [MemShrink]
Reporter | ||
Comment 1•8 years ago
|
||
Alternatively we could just always hold the references weakly, but I'm not sure if thats safe for all uses of unload().
Reporter | ||
Comment 2•8 years ago
|
||
Here is my current work in progress patch.
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Reporter | ||
Comment 3•8 years ago
|
||
Attachment #8768956 -
Attachment is obsolete: true
Attachment #8771543 -
Flags: review?(gkrizsanits)
Reporter | ||
Comment 4•8 years ago
|
||
Attachment #8771544 -
Flags: review?(gkrizsanits)
Reporter | ||
Comment 5•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8771543 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
Attachment #8771544 -
Flags: review?(gkrizsanits) → review+
Updated•8 years ago
|
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
Comment 7•8 years ago
|
||
bugherder |
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: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 8•8 years ago
|
||
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?
Reporter | ||
Comment 9•8 years ago
|
||
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 11•8 years ago
|
||
This was backed out in bug 1321133.
Status: RESOLVED → REOPENED
status-firefox50:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla50 → ---
Reporter | ||
Comment 12•8 years ago
|
||
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
Comment 13•7 years ago
|
||
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: 8 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•