1022376 - LoadManager assumes that when a thread exits the runnable, the thread exits (thread leak)

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Needless to say, that's not how NewNamedThread/NewThread works, even if you provide an initial event.  We've dealt with in the past by having the runnable resend itself to MainThread to kill the thread.

Really we should look at the API for NewThread and in particular the "and provide an event" case.  Also, we should consider allowing threads to call Shutdown on themselves, even if behind the scenes (on some OSes?) it has to proxy it to MainThread.  But that's a separate question from this bug.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Properly shut down LoadMonitor threads

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8436545 [details] [diff] [review]
Properly shut down LoadMonitor threads

This is a straight pattern on what we did for AudioStream.cpp's Init thread and Crypto's nsCryptoTask.cpp.  Ugly but it's the working pattern for now.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8436545 [details] [diff] [review]
Properly shut down LoadMonitor threads

Review of attachment 8436545 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5067fbd502e

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5067fbd502e

Comment 6

[Ed Morley [:emorley]]

This push backed out for Android crashes during 408431-1.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android%204.0%20Panda%20mozilla-inbound%20opt%20test%20crashtest&fromchange=c66224db3600&tochange=6cc9ac1f57fa

I saw bug 1022235 comment 8 - however the existing 408431-1.html crashes were on other platforms - Android 4.0 debug m-6 was green before, with these two changesets, it isn't.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc93584a0948

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Relanded with agreement in IRC from edmorely - bug 1022235 is caused by bug 1022509
https://hg.mozilla.org/integration/mozilla-inbound/rev/97ffe21da653

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97ffe21da653