In an attempt to clear up the Issue 48553 memory leak during init, I re-ordered some of the

startup calls, most important being to call pa_threaded_mainloop_start() right away instead
of after further calls, and creating pa_context_ while the lock was held.

BUG=48553
TEST=See if Valgrind stops suppressing 48553

Review URL: http://codereview.chromium.org/2948013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53081 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 86 insertions, 61 deletions

diff --git a/chrome/browser/chromeos/pulse_audio_mixer.cc b/chrome/browser/chromeos/pulse_audio_mixer.cc
index 53c23d0..a091c8c 100644
--- a/chrome/browser/chromeos/pulse_audio_mixer.cc
+++ b/chrome/browser/chromeos/pulse_audio_mixer.cc
@@ -156,13 +156,38 @@ void PulseAudioMixer::DoInit(InitDoneCallback* callback) {
 }
 
 void PulseAudioMixer::DoGetVolume(GetVolumeCallback* callback,
-                                            void* user) {
+                                  void* user) {
   callback->Run(GetVolumeDb(), user);
   delete callback;
 }
 
+struct ConnectToPulseCallbackData {
+  PulseAudioMixer* instance;
+  bool connect_done;
+};
+
+// static
+void PulseAudioMixer::ConnectToPulseCallbackThunk(
+    pa_context* context, void* userdata) {
+  ConnectToPulseCallbackData* data =
+      static_cast<ConnectToPulseCallbackData*>(userdata);
+  data->instance->OnConnectToPulseCallback(context, &data->connect_done);
+}
+
+void PulseAudioMixer::OnConnectToPulseCallback(
+    pa_context* context, bool* connect_done) {
+  pa_context_state_t state = pa_context_get_state(context);
+  if (state == PA_CONTEXT_READY ||
+      state == PA_CONTEXT_FAILED ||
+      state == PA_CONTEXT_TERMINATED) {
+    // Connection process has reached a terminal state. Wake PulseAudioInit().
+    *connect_done = true;
+    pa_threaded_mainloop_signal(pa_mainloop_, 0);
+  }
+}
+
 bool PulseAudioMixer::PulseAudioInit() {
-  pa_context_state_t state;
+  pa_context_state_t state = PA_CONTEXT_FAILED;
 
   while (true) {
     // Create connection to default server.
@@ -171,48 +196,61 @@ bool PulseAudioMixer::PulseAudioInit() {
       LOG(ERROR) << "Can't create PulseAudio mainloop";
       break;
     }
-    pa_mainloop_api* pa_mlapi = pa_threaded_mainloop_get_api(pa_mainloop_);
-    if (!pa_mlapi) {
-      LOG(ERROR) << "Can't get PulseAudio mainloop api";
-      break;
-    }
-    // This one takes the most time if run at app startup.
-    pa_context_ = pa_context_new(pa_mlapi, "ChromeAudio");
-    if (!pa_context_) {
-      LOG(ERROR) << "Can't create new PulseAudio context";
-      break;
-    }
-    pa_context_set_state_callback(pa_context_, ContextStateCallback,
-                                  pa_mainloop_);
 
-    if (pa_context_connect(pa_context_, NULL,
-                           PA_CONTEXT_NOAUTOSPAWN, NULL) != 0) {
-      LOG(ERROR) << "Can't start connection to PulseAudio sound server";
+    if (pa_threaded_mainloop_start(pa_mainloop_) != 0) {
+      LOG(ERROR) << "Can't start PulseAudio mainloop";
       break;
     }
 
     pa_threaded_mainloop_lock(pa_mainloop_);
 
-    if (pa_threaded_mainloop_start(pa_mainloop_) < 0) {
-      pa_threaded_mainloop_unlock(pa_mainloop_);
-      break;
-    }
-
-    // Wait until we have a completed connection or fail.
-    do {
-      pa_threaded_mainloop_wait(pa_mainloop_);
-      state = pa_context_get_state(pa_context_);
-      if (state == PA_CONTEXT_FAILED) {
-        LOG(ERROR) << "PulseAudio context connection failed";
+    while (true) {
+      pa_mainloop_api* pa_mlapi = pa_threaded_mainloop_get_api(pa_mainloop_);
+      if (!pa_mlapi) {
+        LOG(ERROR) << "Can't get PulseAudio mainloop api";
         break;
       }
-      if (state == PA_CONTEXT_TERMINATED) {
-        LOG(ERROR) << "PulseAudio connection terminated early";
+      // This one takes the most time if run at app startup.
+      pa_context_ = pa_context_new(pa_mlapi, "ChromeAudio");
+      if (!pa_context_) {
+        LOG(ERROR) << "Can't create new PulseAudio context";
         break;
       }
-    } while (state != PA_CONTEXT_READY);
+
+      ConnectToPulseCallbackData data;
+      data.instance = this;
+      data.connect_done = false;
+
+      pa_context_set_state_callback(pa_context_,
+                                    &ConnectToPulseCallbackThunk,
+                                    &data);
+
+      if (pa_context_connect(pa_context_, NULL,
+                             PA_CONTEXT_NOAUTOSPAWN, NULL) != 0) {
+        LOG(ERROR) << "Can't start connection to PulseAudio sound server";
+      } else {
+        // Wait until we have a completed connection or fail.
+        do {
+          pa_threaded_mainloop_wait(pa_mainloop_);
+        } while (!data.connect_done);
+
+        state = pa_context_get_state(pa_context_);
+
+        if (state == PA_CONTEXT_FAILED) {
+          LOG(ERROR) << "PulseAudio context connection failed";
+        } else if (state == PA_CONTEXT_TERMINATED) {
+          LOG(ERROR) << "PulseAudio connection terminated early";
+        } else if (state != PA_CONTEXT_READY) {
+          LOG(ERROR) << "Unknown problem connecting to PulseAudio";
+        }
+      }
+
+      pa_context_set_state_callback(pa_context_, NULL, NULL);
+      break;
+    }
 
     pa_threaded_mainloop_unlock(pa_mainloop_);
+
     if (state != PA_CONTEXT_READY)
       break;
 
@@ -225,28 +263,28 @@ bool PulseAudioMixer::PulseAudioInit() {
 
   // Failed startup sequence, clean up now.
   PulseAudioFree();
-
   return false;
 }
 
 void PulseAudioMixer::PulseAudioFree() {
-  if (pa_mainloop_) {
-    DCHECK_NE(mixer_state_, UNINITIALIZED);
-    pa_threaded_mainloop_lock(pa_mainloop_);
-    mixer_state_ = SHUTTING_DOWN;
-    pa_threaded_mainloop_unlock(pa_mainloop_);
-    pa_threaded_mainloop_stop(pa_mainloop_);
-  }
+  if (!pa_mainloop_)
+    return;
+
+  DCHECK_NE(mixer_state_, UNINITIALIZED);
+  mixer_state_ = SHUTTING_DOWN;
+
   if (pa_context_) {
-    pa_context_set_state_callback(pa_context_, NULL, NULL);
+    pa_threaded_mainloop_lock(pa_mainloop_);
     pa_context_disconnect(pa_context_);
     pa_context_unref(pa_context_);
+    pa_threaded_mainloop_unlock(pa_mainloop_);
     pa_context_ = NULL;
   }
-  if (pa_mainloop_) {
-    pa_threaded_mainloop_free(pa_mainloop_);
-    pa_mainloop_ = NULL;
-  }
+
+  pa_threaded_mainloop_stop(pa_mainloop_);
+  pa_threaded_mainloop_free(pa_mainloop_);
+  pa_mainloop_ = NULL;
+
   mixer_state_ = UNINITIALIZED;
 }
 
@@ -342,19 +380,5 @@ void PulseAudioMixer::GetAudioInfoCallback(pa_context* unused,
   pa_threaded_mainloop_signal(cb_data->mainloop, 0);
 }
 
-// static
-void PulseAudioMixer::ContextStateCallback(pa_context* c, void* userdata) {
-  pa_threaded_mainloop* mainloop = static_cast<pa_threaded_mainloop*>(userdata);
-  switch (pa_context_get_state(c)) {
-    case PA_CONTEXT_READY:
-    case PA_CONTEXT_TERMINATED:
-    case PA_CONTEXT_FAILED:
-      pa_threaded_mainloop_signal(mainloop, 0);
-      break;
-    default:
-      break;
-  }
-}
-
 }  // namespace chromeos
 
diff --git a/chrome/browser/chromeos/pulse_audio_mixer.h b/chrome/browser/chromeos/pulse_audio_mixer.h
index c74563a..223fe33 100644
--- a/chrome/browser/chromeos/pulse_audio_mixer.h
+++ b/chrome/browser/chromeos/pulse_audio_mixer.h
@@ -63,6 +63,9 @@ class PulseAudioMixer {
   void DoInit(InitDoneCallback* callback);
   void DoGetVolume(GetVolumeCallback* callback, void* user);
 
+  static void ConnectToPulseCallbackThunk(pa_context* c, void* userdata);
+  void OnConnectToPulseCallback(pa_context* c, bool* connect_done);
+
   // This goes through sequence of connecting to the default PulseAudio server.
   // We will block until we either have a valid connection or something failed.
   // If a connection is lost for some reason, delete and recreate the object.
@@ -98,8 +101,6 @@ class PulseAudioMixer {
                                    int eol,
                                    void* userdata);
 
-  static void ContextStateCallback(pa_context* c, void* userdata);
-
   // The PulseAudio index of the main device being used.
   mutable int device_id_;