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)

defect
Not set
normal

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)

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)
Attachment #383027 - Flags: review?(benjamin) → review+
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.
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
Sorry.  My fault.  I somehow didn't see (and test) the whole patch :-)

I'll comment again once I've managed to do that.
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.
Attachment #383027 - Attachment description: Patch, v1 → Patch, v1 [Checkin: Comment 5]
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
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Attachment #383027 - Flags: approval1.9.1.3?
Attachment #383027 - Flags: approval1.9.1.3? → approval1.9.1.4?
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?
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?
Attachment #383027 - Flags: approval1.9.1.4? → approval1.9.1.4+
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
Whiteboard: [needs 1.9.1 landing: needs approval] → [needs 1.9.1 landing]
Serge, can you land this?
Keywords: checkin-needed
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]
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing]
You need to log in before you can comment on or make changes to this bug.