Fix leak of ShutdownDetector.

Fix leak of ShutdownDetector.
For some reason I thought that non-joinable threads would always delete their delegates.  I was wrong.
BUG=http://crbug.com/29675

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34110

[willchan no longer on Chromium, 2009-12-08 20:52:00]

The linux_valgrind bot is being stupid, it keeps timing out, but I'm pretty
confident this fixes the leak.  It's pretty obvious.

[Mark Mentovai, 2009-12-08 20:56:03]

I don't think this is right.  This only deletes the thread object if we take a
signal.  In the normal case, when we don't take a signal, the thread will still
be sitting in its while loop blocked on read when the process exits.

[Mark Mentovai, 2009-12-08 20:57:30]

Suggest:

Close the pipe in BrowserMain when things are shutting down.  Pick the close
(zero-length read) up in the thread object and just return from the thread,
causing it to exit.  Verify whether we actually do need a "delete this" - I'd be
surprised if we did.

[willchan no longer on Chromium, 2009-12-08 21:04:05]

Hm, I think you're pointing out a separate leak.  I'll implement your
suggestion too, but I think we do need the "delete this".  Otherwise,
what's deleting the delegate?

On Tue, Dec 8, 2009 at 12:57 PM,  <mark@chromium.org> wrote:
> Suggest:
>
> Close the pipe in BrowserMain when things are shutting down.  Pick the close
> (zero-length read) up in the thread object and just return from the thread,
> causing it to exit.  Verify whether we actually do need a "delete this" -
> I'd be
> surprised if we did.
>
> http://codereview.chromium.org/460144
>

[Mark Mentovai, 2009-12-08 21:06:03]

William Chan (陈智昌) wrote:
> Hm, I think you're pointing out a separate leak.  I'll implement your
> suggestion too, but I think we do need the "delete this".  Otherwise,
> what's deleting the delegate?

The thread when it exits?  I don't recall what the internals there are.

Mark

[Mark Mentovai, 2009-12-08 21:07:37]

I wrote:
> The thread when it exits?  I don't recall what the internals there are.

I just looked at the platform_thread_posix internals and you're right,
the "delete this" does look necessary.

Mark

[willchan no longer on Chromium, 2009-12-08 22:17:06]

I think I've fixed the normal shutdown path now.  Let me know if this seems fine
to you.

[Mark Mentovai, 2009-12-08 22:21:37]

LG with three changes.

http://codereview.chromium.org/460144/diff/3004/3005
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/460144/diff/3004/3005#newcode174
chrome/browser/browser_main.cc:174: int g_shutdown_pipe_read_fd = -1;
I realized that this variable is unnecessary.  We should get rid of it.  It can
be closed by the thread but still wind up holding a value other than -1. 
Maintaining it as a global doesn't seem useful.

http://codereview.chromium.org/460144/diff/3004/3005#newcode982
chrome/browser/browser_main.cc:982: ret = close(g_shutdown_pipe_write_fd);
HANDLE_EINTR.

I'd also set g_shutdown_pipe_write_fd to -1 here to avoid anyone trying to use
it.