Closed
Bug 498010
Opened 15 years ago
Closed 15 years ago
Threads should release their observers when they are shut down
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
Tracking | Status | |
---|---|---|
status1.9.1 | --- | .4-fixed |
People
(Reporter: bent.mozilla, Assigned: bent.mozilla)
References
Details
Attachments
(1 file)
(deleted),
patch
|
benjamin
:
review+
dveditz
:
approval1.9.1.4+
|
Details | Diff | Splinter Review |
Threads should release their observers when they are shut down. Leaks and cycles can occur otherwise. Bug 472773 exposed such a case on app-initiated restart.
Attachment #383027 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #383027 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 1•15 years ago
|
||
This patch doesn't stop nsAppShell/nsBaseAppShell from leaking on relaunch (or when doing 'firefox-bin -silent'). It also doesn't stop the main thread's nsThread object from leaking, because the browser doesn't create the main thread (it already exists) -- which means that nsThread::ThreadFunc() doesn't run for the main thread.
Comment 2•15 years ago
|
||
This patch shouldn't be landed as is. But the general idea is sound. We just need a revision that also releases the main thread's observer as it shuts down.
Keywords: checkin-needed
Comment 3•15 years ago
|
||
Sorry. My fault. I somehow didn't see (and test) the whole patch :-) I'll comment again once I've managed to do that.
Updated•15 years ago
|
Keywords: checkin-needed
Comment 4•15 years ago
|
||
I've tested again, using the full patch here (attachment 383027 [details] [diff] [review]) plus my patch for bug 472773. Now the nsAppShell/nsBaseAppShell never leaks, even on relaunch or when using 'firefox-bin -silent'. The main thread's nsThread object still leaks on relaunch and using -silent ... as I should have expected. But that's because of bug 497745. Now (with this bug's patch) this leak no longer causes the nsAppShell to leak. Thanks, Ben, for your patch.
Updated•15 years ago
|
Attachment #383027 -
Attachment description: Patch, v1 → Patch, v1
[Checkin: Comment 5]
Comment 5•15 years ago
|
||
Comment on attachment 383027 [details] [diff] [review] Patch, v1 [Checkin: Comment 5 & 10] http://hg.mozilla.org/mozilla-central/rev/21964b89824c
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: wanted1.9.1.x?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing: needs approval]
Target Milestone: --- → mozilla1.9.2a1
Updated•15 years ago
|
status1.9.1:
--- → ?
Flags: wanted1.9.1.x?
Updated•15 years ago
|
Attachment #383027 -
Flags: approval1.9.1.3?
Updated•15 years ago
|
Attachment #383027 -
Flags: approval1.9.1.3? → approval1.9.1.4?
Comment 6•15 years ago
|
||
Comment on attachment 383027 [details] [diff] [review] Patch, v1 [Checkin: Comment 5 & 10] Why is this nominated for the 1.9.1 branch? Doesn't appear to be a security fix, topcrash, regression, or killing babies. Also, since this approval request didn't come from the developer, it's not clear whether this is the appropriate branch patch or not.
Attachment #383027 -
Flags: approval1.9.1.4?
Assignee | ||
Comment 7•15 years ago
|
||
Comment on attachment 383027 [details] [diff] [review] Patch, v1 [Checkin: Comment 5 & 10] (In reply to comment #6) > Why is this nominated for the 1.9.1 branch? It's a leak fix that prevents lots of random orange on tinderbox.
Attachment #383027 -
Flags: approval1.9.1.4?
Updated•15 years ago
|
Attachment #383027 -
Flags: approval1.9.1.4? → approval1.9.1.4+
Comment 8•15 years ago
|
||
Comment on attachment 383027 [details] [diff] [review] Patch, v1 [Checkin: Comment 5 & 10] Thanks. Approved for 1.9.1.4, a=dveditz for release-drivers
Updated•15 years ago
|
Whiteboard: [needs 1.9.1 landing: needs approval] → [needs 1.9.1 landing]
Comment 10•15 years ago
|
||
Comment on attachment 383027 [details] [diff] [review] Patch, v1 [Checkin: Comment 5 & 10] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4eb7bf33094d
Attachment #383027 -
Attachment description: Patch, v1
[Checkin: Comment 5] → Patch, v1
[Checkin: Comment 5 & 10]
Updated•15 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•