Patch pulseaudio memory leak

An earlier check-in (r51786, Issue 2769008) caused a memory leak on the build bots when PulseAudio was initialized on a separate worker thread.  This change calls PulseAudioMixer::InitSync() so the init happens on the calling thread.  For some reason, the init finishes cleanly on one thread, but not the other.

BUG=48553
(and http://code.google.com/p/chromium-os/issues/detail?id=5141)
TEST=Valgrind should not have PulseAudio related memory leaks, suppression 48553, or assertion failures.

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

[davejcool, 2010-07-27 22:37:50]

This patch should stop the PulseAudio memory leak and assert seen on the build
bots.

[scherkus (not reviewing), 2010-07-27 22:44:25]

http://codereview.chromium.org/3026028/diff/2001/3001
File chrome/browser/chromeos/audio_handler.cc (right):

http://codereview.chromium.org/3026028/diff/2001/3001#newcode102
chrome/browser/chromeos/audio_handler.cc:102: DLOG(INFO) << "OnMixerInitialized,
success = " << connected_;
just to sanity check.. this is a singleton that initializes in the constructor

what's the expected behaviour if this fails, seeing as we can't re-create the
object?

[Daniel Erat, 2010-07-27 22:44:45]

I haven't been following this code closely, so I'll defer to others for the
actual review of the fix.

http://codereview.chromium.org/3026028/diff/2001/3001
File chrome/browser/chromeos/audio_handler.cc (right):

http://codereview.chromium.org/3026028/diff/2001/3001#newcode102
chrome/browser/chromeos/audio_handler.cc:102: DLOG(INFO) << "OnMixerInitialized,
success = " << connected_;
The "OnMixerInitialized" in this comment is stale now.

[davejcool, 2010-07-27 23:39:09]

http://codereview.chromium.org/3026028/diff/2001/3001
File chrome/browser/chromeos/audio_handler.cc (right):

http://codereview.chromium.org/3026028/diff/2001/3001#newcode102
chrome/browser/chromeos/audio_handler.cc:102: DLOG(INFO) << "OnMixerInitialized,
success = " << connected_;
If it fails, mixer_ will not have a connection to PulseAudio.  But if any of the
APIs are called to adjust the Volume or Mute, a retry to connect will happen. 
Previously, the init was still initiated when the singleton initialized... with
this change it now just completes before the singleton initialization returns.

An IsValid() could be added that just returns true if mixer_->CheckState() =
READY, so at least the caller could verify things started up OK.

I'm hoping to get to the bottom of why Pulse doesn't play well when initialized
on a separate thread, so this may go back to the way it was.  Either way, an
IsValid() could be useful here since AudioHandler is a singleton.

[scherkus (not reviewing), 2010-07-28 00:49:26]

LGTM w/ daniel's comments + if my comment doesn't force us to change any other
code

http://codereview.chromium.org/3026028/diff/2001/3001
File chrome/browser/chromeos/audio_handler.cc (right):

http://codereview.chromium.org/3026028/diff/2001/3001#newcode102
chrome/browser/chromeos/audio_handler.cc:102: DLOG(INFO) << "OnMixerInitialized,
success = " << connected_;
OK.  As another sanity check... do we know which thread will use the
AudioHandler first?

For example, if it's on the UI thread then we can end up blocking that thread to
create a connection to PulseAudio.

I guess it might be unavoidable if any method call will attempt a reconnect

[davejcool, 2010-07-28 01:39:35]

This just has an IsValid() call added.  But since this change means going back
to having PulseAudio connected to on the UI thread for now, the question is, is
that better than having all these memory leaks and assertion failure?

http://codereview.chromium.org/3026028/diff/2001/3001
File chrome/browser/chromeos/audio_handler.cc (right):

http://codereview.chromium.org/3026028/diff/2001/3001#newcode102
chrome/browser/chromeos/audio_handler.cc:102: DLOG(INFO) << "OnMixerInitialized,
success = " << connected_;
It looks like it's on the UI message loop, since it's called inside
BrowserInit::LaunchBrowser().  I didn't notice any difference in boot-up time
though, but that's probably because a browser window is not opened until you
sign in.  So until I figure out what's causing the startup to fail when not on
the UI thread, it's either we keep having this "mutex" assertion failure on exit
and memory leak, or have the login time possibly slower (same speed as it was
before the 7/7/10 checkin r51786).

It seems to be getting the mutex assert if pulseaudio is running (the destructor
is likely being run from a separate thread).  But that doesn't explain the other
leaks that happen if a connection is unsuccessful, since the cleanup is done on
the same thread right away.

[scherkus (not reviewing), 2010-07-28 17:49:29]

ok LGTM

http://codereview.chromium.org/3026028/diff/9001/5002
File chrome/browser/chromeos/audio_handler.cc (right):

http://codereview.chromium.org/3026028/diff/9001/5002#newcode99
chrome/browser/chromeos/audio_handler.cc:99: if (mixer_->CheckState() ==
PulseAudioMixer::READY)
how about:

return mixer_->CheckState() == PulseAudioMixer::READY;

[davejcool, 2010-07-28 18:22:59]

I think all looks good now.  I'm ready to land this patch once the tree opens.