Re: Fwd: [Bug 784193] Thunderbird leaks threads

On 01/02/2013 16:11, Stefan Sitter wrote:
> On 31.01.2013 16:37, Orion Poplawski wrote:
>> Any thoughts?
>
>  From a quick search [1] in the latest Lightning 2.3 source code I think
> there is only one place [2] that creates a thread in
> calICSService::ParseICSAsync() via NS_NewThread().
>
> How to tell if this thread is leaked?

If I understand threads correctly, then when it is no longer required, 
Shutdown() must be called on the thread:

http://hg.mozilla.org/mozilla-central/annotate/847e28c7ba67/xpcom/threads/nsIThread.idl#l30

 From what I can tell, the calendar code does this, for each call to 
ParseICSAsync:

- Creates a worker (nsIRunnable) to parse the ics value
- Creates a new thread and dispatches the worker to it
- When the thread gets the go-ahead, it runs the the parser
- It them dispatches an event back to the main/original thread when it 
has completed.

At no stage is Shutdown called, and a reference to the thread isn't kept 
either.

There's two potential ways to fix this:

1) calICSService creates only one thread per application run, and 
dispatches all ParseICSAsync requests to that thread. The thread then 
can be shutdown once when the application shutdown.

2) calICSService continues creating multiple threads, but probably via 
passing additional arguments/references, keeps track of when thread 
completes and shuts them down appropriately.

The first option is probably slightly better than the second if there's 
an acceptable trade-off that async ics requests are queued up if one is 
still running when another comes in.

Mark.

p.s. if you agree this looks about right, please can someone file a bug 
and stick this into it.

>
> [1]
> http://mxr.mozilla.org/comm-central/search?string=thread&find=/calendar/
>
> [2]
> http://mxr.mozilla.org/comm-central/source/calendar/base/src/calICSService.cpp#1264