Cleanup client shutdown sequence.

BUG=None
TEST=Client doesn't crash when reloading the tab.

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

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

10 files changed, 104 insertions, 61 deletions

diff --git a/remoting/client/chromoting_client.cc b/remoting/client/chromoting_client.cc
index 8161089..d7eee8e 100644
--- a/remoting/client/chromoting_client.cc
+++ b/remoting/client/chromoting_client.cc
@@ -4,6 +4,7 @@
 
 #include "remoting/client/chromoting_client.h"
 
+#include "base/bind.h"
 #include "base/message_loop.h"
 #include "remoting/base/tracer.h"
 #include "remoting/client/chromoting_view.h"
@@ -57,17 +58,22 @@ void ChromotingClient::Start(scoped_refptr<XmppProxy> xmpp_proxy) {
   }
 }
 
-void ChromotingClient::Stop() {
+void ChromotingClient::Stop(const base::Closure& shutdown_task) {
   if (message_loop() != MessageLoop::current()) {
     message_loop()->PostTask(
-        FROM_HERE,
-        NewRunnableMethod(this, &ChromotingClient::Stop));
+        FROM_HERE, base::Bind(&ChromotingClient::Stop,
+                              base::Unretained(this), shutdown_task));
     return;
   }
 
-  connection_->Disconnect();
+  connection_->Disconnect(base::Bind(&ChromotingClient::OnDisconnected,
+                                     base::Unretained(this), shutdown_task));
+}
 
+void ChromotingClient::OnDisconnected(const base::Closure& shutdown_task) {
   view_->TearDown();
+
+  shutdown_task.Run();
 }
 
 void ChromotingClient::ClientDone() {
diff --git a/remoting/client/chromoting_client.h b/remoting/client/chromoting_client.h
index f485e97..67bda81 100644
--- a/remoting/client/chromoting_client.h
+++ b/remoting/client/chromoting_client.h
@@ -51,7 +51,7 @@ class ChromotingClient : public protocol::ConnectionToHost::HostEventCallback,
   virtual ~ChromotingClient();
 
   void Start(scoped_refptr<XmppProxy> xmpp_proxy);
-  void Stop();
+  void Stop(const base::Closure& shutdown_task);
   void ClientDone();
 
   // Return the stats recorded by this client.
@@ -109,6 +109,8 @@ class ChromotingClient : public protocol::ConnectionToHost::HostEventCallback,
   // the packet will start to be processed.
   void OnPacketDone(bool last_packet, base::Time decode_start);
 
+  void OnDisconnected(const base::Closure& shutdown_task);
+
   // The following are not owned by this class.
   ClientConfig config_;
   ClientContext* context_;
diff --git a/remoting/client/plugin/chromoting_instance.cc b/remoting/client/plugin/chromoting_instance.cc
index c1d7548..459c701 100644
--- a/remoting/client/plugin/chromoting_instance.cc
+++ b/remoting/client/plugin/chromoting_instance.cc
@@ -7,9 +7,11 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stringprintf.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/task.h"
 #include "base/threading/thread.h"
 // TODO(sergeyu): We should not depend on renderer here. Instead P2P
@@ -55,8 +57,13 @@ ChromotingInstance::ChromotingInstance(PP_Instance pp_instance)
 }
 
 ChromotingInstance::~ChromotingInstance() {
+  DCHECK(CurrentlyOnPluginThread());
+
   if (client_.get()) {
-    client_->Stop();
+    base::WaitableEvent done_event(true, false);
+    client_->Stop(base::Bind(&base::WaitableEvent::Signal,
+                             base::Unretained(&done_event)));
+    done_event.Wait();
   }
 
   // Stopping the context shutdown all chromoting threads. This is a requirement
@@ -142,7 +149,11 @@ void ChromotingInstance::Disconnect() {
 
   logger_.Log(logging::LOG_INFO, "Disconnecting from host.");
   if (client_.get()) {
-    client_->Stop();
+    // TODO(sergeyu): Should we disconnect asynchronously?
+    base::WaitableEvent done_event(true, false);
+    client_->Stop(base::Bind(&base::WaitableEvent::Signal,
+                             base::Unretained(&done_event)));
+    done_event.Wait();
   }
 
   GetScriptableObject()->SetConnectionInfo(STATUS_CLOSED, QUALITY_UNKNOWN);
diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index ec9e8a2..b1ddb5b 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -639,8 +639,8 @@ void ChromotingHost::ShutdownJingleClient() {
 
   // Disconnect from the talk network.
   if (jingle_client_) {
-    jingle_client_->Close(NewRunnableMethod(
-        this, &ChromotingHost::ShutdownSignallingDisconnected));
+    jingle_client_->Close(base::Bind(
+        &ChromotingHost::ShutdownSignallingDisconnected, this));
   } else {
     ShutdownRecorder();
   }
diff --git a/remoting/jingle_glue/jingle_client.cc b/remoting/jingle_glue/jingle_client.cc
index 79688a9..d363b27 100644
--- a/remoting/jingle_glue/jingle_client.cc
+++ b/remoting/jingle_glue/jingle_client.cc
@@ -4,6 +4,7 @@
 
 #include "remoting/jingle_glue/jingle_client.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
@@ -51,12 +52,12 @@ void JingleClient::Init() {
     initialized_ = true;
   }
 
-  message_loop()->PostTask(
+  message_loop_->PostTask(
       FROM_HERE, NewRunnableMethod(this, &JingleClient::DoInitialize));
 }
 
 void JingleClient::DoInitialize() {
-  DCHECK_EQ(message_loop(), MessageLoop::current());
+  DCHECK_EQ(message_loop_, MessageLoop::current());
 
   if (!network_manager_.get()) {
     VLOG(1) << "Creating talk_base::NetworkManager.";
@@ -111,40 +112,32 @@ void JingleClient::DoStartSession() {
   }
 }
 
-void JingleClient::Close() {
-  Close(NULL);
-}
-
-void JingleClient::Close(Task* closed_task) {
+void JingleClient::Close(const base::Closure& closed_task) {
   {
     base::AutoLock auto_lock(state_lock_);
     // If the client is already closed then don't close again.
     if (closed_) {
-      if (closed_task)
-        message_loop_->PostTask(FROM_HERE, closed_task);
+      message_loop_->PostTask(FROM_HERE, closed_task);
       return;
     }
-    closed_task_.reset(closed_task);
-    closed_ = true;
   }
 
-  message_loop()->PostTask(
-      FROM_HERE, NewRunnableMethod(this, &JingleClient::DoClose));
+  message_loop_->PostTask(
+      FROM_HERE, NewRunnableMethod(this, &JingleClient::DoClose, closed_task));
 }
 
-void JingleClient::DoClose() {
-  DCHECK_EQ(message_loop(), MessageLoop::current());
-  DCHECK(closed_);
+void JingleClient::DoClose(const base::Closure& closed_task) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
 
   session_manager_.reset();
-  signal_strategy_->EndSession();
-  // TODO(ajwong): SignalStrategy should drop all resources at EndSession().
-  signal_strategy_ = NULL;
-
-  if (closed_task_.get()) {
-    closed_task_->Run();
-    closed_task_.reset();
+  if (signal_strategy_) {
+    signal_strategy_->EndSession();
+    // TODO(ajwong): SignalStrategy should drop all resources at EndSession().
+    signal_strategy_ = NULL;
   }
+
+  closed_ = true;
+  closed_task.Run();
 }
 
 std::string JingleClient::GetFullJid() {
@@ -161,7 +154,7 @@ MessageLoop* JingleClient::message_loop() {
 }
 
 cricket::SessionManager* JingleClient::session_manager() {
-  DCHECK_EQ(message_loop(), MessageLoop::current());
+  DCHECK_EQ(message_loop_, MessageLoop::current());
   return session_manager_.get();
 }
 
diff --git a/remoting/jingle_glue/jingle_client.h b/remoting/jingle_glue/jingle_client.h
index 1000c35..d0b140b5 100644
--- a/remoting/jingle_glue/jingle_client.h
+++ b/remoting/jingle_glue/jingle_client.h
@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -65,11 +66,10 @@ class JingleClient : public base::RefCountedThreadSafe<JingleClient>,
   // |callback| specifies callback object for the client and must not be NULL.
   void Init();
 
-  // Closes XMPP connection and stops the thread. Must be called before the
-  // object is destroyed. If specified, |closed_task| is executed after the
-  // connection is successfully closed.
-  void Close();
-  void Close(Task* closed_task);
+  // Closes XMPP connection and stops the thread. Must be called
+  // before the object is destroyed. |closed_task| is executed after
+  // the connection is successfully closed.
+  void Close(const base::Closure& closed_task);
 
   // Returns JID with resource ID. Empty string is returned if full JID is not
   // known yet, i.e. authentication hasn't finished.
@@ -95,7 +95,7 @@ class JingleClient : public base::RefCountedThreadSafe<JingleClient>,
 
   void DoInitialize();
   void DoStartSession();
-  void DoClose();
+  void DoClose(const base::Closure& closed_task);
 
   // Updates current state of the connection. Must be called only in
   // the jingle thread.
diff --git a/remoting/jingle_glue/jingle_client_unittest.cc b/remoting/jingle_glue/jingle_client_unittest.cc
index 9c50678..952aa36 100644
--- a/remoting/jingle_glue/jingle_client_unittest.cc
+++ b/remoting/jingle_glue/jingle_client_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
 #include "base/synchronization/waitable_event.h"
 #include "remoting/jingle_glue/jingle_client.h"
 #include "remoting/jingle_glue/jingle_thread.h"
@@ -66,7 +67,10 @@ TEST_F(JingleClientTest, OnStateChanged) {
       buzz::XmppEngine::STATE_OPENING, &state_changed_event));
   state_changed_event.Wait();
 
-  client_->Close();
+  base::WaitableEvent closed_event(true, false);
+  client_->Close(base::Bind(&base::WaitableEvent::Signal,
+                            base::Unretained(&closed_event)));
+  closed_event.Wait();
 
   thread_.Stop();
 }
@@ -74,7 +78,11 @@ TEST_F(JingleClientTest, OnStateChanged) {
 TEST_F(JingleClientTest, Close) {
   EXPECT_CALL(callback_, OnStateChange(_, _))
       .Times(0);
-  client_->Close();
+  base::WaitableEvent closed_event(true, false);
+  client_->Close(base::Bind(&base::WaitableEvent::Signal,
+                            base::Unretained(&closed_event)));
+  closed_event.Wait();
+
   // Verify that the channel doesn't call callback anymore.
   thread_.message_loop()->PostTask(FROM_HERE, NewRunnableFunction(
       &JingleClientTest::ChangeState, signal_strategy_.get(),
@@ -85,19 +93,16 @@ TEST_F(JingleClientTest, Close) {
 
 TEST_F(JingleClientTest, ClosedTask) {
   bool closed = false;
-  client_->Close(NewRunnableFunction(&JingleClientTest::OnClosed,
-                                     &closed));
+  client_->Close(base::Bind(&JingleClientTest::OnClosed, &closed));
   thread_.Stop();
   EXPECT_TRUE(closed);
 }
 
 TEST_F(JingleClientTest, DoubleClose) {
   bool closed1 = false;
-  client_->Close(NewRunnableFunction(&JingleClientTest::OnClosed,
-                                     &closed1));
+  client_->Close(base::Bind(&JingleClientTest::OnClosed, &closed1));
   bool closed2 = false;
-  client_->Close(NewRunnableFunction(&JingleClientTest::OnClosed,
-                                     &closed2));
+  client_->Close(base::Bind(&JingleClientTest::OnClosed, &closed2));
   thread_.Stop();
   EXPECT_TRUE(closed1 && closed2);
 }
diff --git a/remoting/protocol/connection_to_host.cc b/remoting/protocol/connection_to_host.cc
index b8f4340..074f2b3 100644
--- a/remoting/protocol/connection_to_host.cc
+++ b/remoting/protocol/connection_to_host.cc
@@ -4,6 +4,7 @@
 
 #include "remoting/protocol/connection_to_host.h"
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/message_loop.h"
 #include "remoting/base/constants.h"
@@ -79,18 +80,20 @@ void ConnectionToHost::Connect(scoped_refptr<XmppProxy> xmpp_proxy,
   host_public_key_ = host_public_key;
 }
 
-void ConnectionToHost::Disconnect() {
+void ConnectionToHost::Disconnect(const base::Closure& shutdown_task) {
   if (MessageLoop::current() != message_loop_) {
     message_loop_->PostTask(
-        FROM_HERE, NewRunnableMethod(this, &ConnectionToHost::Disconnect));
+        FROM_HERE, base::Bind(&ConnectionToHost::Disconnect,
+                              base::Unretained(this), shutdown_task));
     return;
   }
 
   if (session_) {
     session_->Close(
-        NewRunnableMethod(this, &ConnectionToHost::OnDisconnected));
+        NewRunnableMethod(this, &ConnectionToHost::OnDisconnected,
+                          shutdown_task));
   } else {
-    OnDisconnected();
+    OnDisconnected(shutdown_task);
   }
 }
 
@@ -122,25 +125,42 @@ void ConnectionToHost::InitSession() {
       NewCallback(this, &ConnectionToHost::OnSessionStateChange));
 }
 
-void ConnectionToHost::OnDisconnected() {
+void ConnectionToHost::OnDisconnected(const base::Closure& shutdown_task) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
   session_ = NULL;
 
   if (session_manager_) {
     session_manager_->Close(
-        NewRunnableMethod(this, &ConnectionToHost::OnServerClosed));
+        NewRunnableMethod(this, &ConnectionToHost::OnServerClosed,
+                          shutdown_task));
   } else {
-    OnServerClosed();
+    OnServerClosed(shutdown_task);
   }
 }
 
-void ConnectionToHost::OnServerClosed() {
+void ConnectionToHost::OnServerClosed(const base::Closure& shutdown_task) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
   session_manager_ = NULL;
+
   if (jingle_client_) {
-    jingle_client_->Close();
-    jingle_client_ = NULL;
+    jingle_client_->Close(base::Bind(&ConnectionToHost::OnJingleClientClosed,
+                                     base::Unretained(this), shutdown_task));
+  } else {
+    OnJingleClientClosed(shutdown_task);
   }
 }
 
+void ConnectionToHost::OnJingleClientClosed(
+    const base::Closure& shutdown_task) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  signal_strategy_.reset();
+
+  shutdown_task.Run();
+}
+
 const SessionConfig* ConnectionToHost::config() {
   return session_->config();
 }
diff --git a/remoting/protocol/connection_to_host.h b/remoting/protocol/connection_to_host.h
index 554b760..e0511a6 100644
--- a/remoting/protocol/connection_to_host.h
+++ b/remoting/protocol/connection_to_host.h
@@ -78,7 +78,8 @@ class ConnectionToHost : public JingleClient::Callback {
                        HostEventCallback* event_callback,
                        ClientStub* client_stub,
                        VideoStub* video_stub);
-  virtual void Disconnect();
+
+  virtual void Disconnect(const base::Closure& shutdown_task);
 
   virtual const SessionConfig* config();
 
@@ -113,8 +114,9 @@ class ConnectionToHost : public JingleClient::Callback {
 
   // Used by Disconnect() to disconnect chromoting connection, stop chromoting
   // server, and then disconnect XMPP connection.
-  void OnDisconnected();
-  void OnServerClosed();
+  void OnDisconnected(const base::Closure& shutdown_task);
+  void OnServerClosed(const base::Closure& shutdown_task);
+  void OnJingleClientClosed(const base::Closure& shutdown_task);
 
   // Internal state of the connection.
   State state_;
diff --git a/remoting/protocol/protocol_test_client.cc b/remoting/protocol/protocol_test_client.cc
index 39abd70..e19aa5f42 100644
--- a/remoting/protocol/protocol_test_client.cc
+++ b/remoting/protocol/protocol_test_client.cc
@@ -14,6 +14,7 @@ extern "C" {
 #include <list>
 
 #include "base/at_exit.h"
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/test/mock_chrome_application_mac.h"
 #include "base/time.h"
@@ -268,7 +269,10 @@ void ProtocolTestClient::Run(const std::string& username,
     closed_event_.Wait();
   }
 
-  client_->Close();
+  base::WaitableEvent closed_event(true, false);
+  client_->Close(base::Bind(&base::WaitableEvent::Signal,
+                            base::Unretained(&closed_event)));
+  closed_event.Wait();
   jingle_thread.Stop();
 }