Use plugin main thread for network code on the client side

BUG=None
TEST=Everything still works.

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

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

8 files changed, 62 insertions, 34 deletions

diff --git a/remoting/client/client_context.cc b/remoting/client/client_context.cc
index f085778..e1bca5e 100644
--- a/remoting/client/client_context.cc
+++ b/remoting/client/client_context.cc
@@ -8,8 +8,7 @@ namespace remoting {
 
 ClientContext::ClientContext(base::MessageLoopProxy* main_message_loop_proxy)
     : main_message_loop_proxy_(main_message_loop_proxy),
-      decode_thread_("ChromotingClientDecodeThread"),
-      network_thread_("ChromotingClientNetworkThread") {
+      decode_thread_("ChromotingClientDecodeThread") {
 }
 
 ClientContext::~ClientContext() {
@@ -18,13 +17,10 @@ ClientContext::~ClientContext() {
 void ClientContext::Start() {
   // Start all the threads.
   decode_thread_.Start();
-  network_thread_.StartWithOptions(
-      base::Thread::Options(MessageLoop::TYPE_IO, 0));
 }
 
 void ClientContext::Stop() {
   // Stop all the threads.
-  network_thread_.Stop();
   decode_thread_.Stop();
 }
 
@@ -37,7 +33,7 @@ MessageLoop* ClientContext::decode_message_loop() {
 }
 
 base::MessageLoopProxy* ClientContext::network_message_loop() {
-  return network_thread_.message_loop_proxy();
+  return main_message_loop_proxy_;
 }
 
 }  // namespace remoting
diff --git a/remoting/client/client_context.h b/remoting/client/client_context.h
index e64303f..41e5b16 100644
--- a/remoting/client/client_context.h
+++ b/remoting/client/client_context.h
@@ -32,8 +32,6 @@ class ClientContext {
   // A thread that handles all decode operations.
   base::Thread decode_thread_;
 
-  base::Thread network_thread_;
-
   DISALLOW_COPY_AND_ASSIGN(ClientContext);
 };
 
diff --git a/remoting/client/plugin/chromoting_instance.cc b/remoting/client/plugin/chromoting_instance.cc
index 50be681..878289c 100644
--- a/remoting/client/plugin/chromoting_instance.cc
+++ b/remoting/client/plugin/chromoting_instance.cc
@@ -456,7 +456,8 @@ void ChromotingInstance::ProcessLogToUI(const std::string& message) {
 
 pp::Var ChromotingInstance::GetInstanceObject() {
   if (instance_object_.is_undefined()) {
-    ChromotingScriptableObject* object = new ChromotingScriptableObject(this);
+    ChromotingScriptableObject* object =
+        new ChromotingScriptableObject(this, plugin_message_loop_);
     object->Init();
 
     // The pp::Var takes ownership of object here.
diff --git a/remoting/client/plugin/chromoting_scriptable_object.cc b/remoting/client/plugin/chromoting_scriptable_object.cc
index b8b9584..7970785 100644
--- a/remoting/client/plugin/chromoting_scriptable_object.cc
+++ b/remoting/client/plugin/chromoting_scriptable_object.cc
@@ -5,6 +5,7 @@
 #include "remoting/client/plugin/chromoting_scriptable_object.h"
 
 #include "base/logging.h"
+#include "base/message_loop_proxy.h"
 // TODO(wez): Remove this when crbug.com/86353 is complete.
 #include "ppapi/cpp/private/var_private.h"
 #include "remoting/base/auth_token_util.h"
@@ -41,8 +42,10 @@ const char kRoundTripLatencyAttribute[] = "roundTripLatency";
 }  // namespace
 
 ChromotingScriptableObject::ChromotingScriptableObject(
-    ChromotingInstance* instance)
-    : instance_(instance) {
+    ChromotingInstance* instance, base::MessageLoopProxy* plugin_message_loop)
+    : instance_(instance),
+      plugin_message_loop_(plugin_message_loop),
+      ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
 }
 
 ChromotingScriptableObject::~ChromotingScriptableObject() {
@@ -282,6 +285,24 @@ void ChromotingScriptableObject::AddMethod(const std::string& name,
 }
 
 void ChromotingScriptableObject::SignalConnectionInfoChange() {
+  plugin_message_loop_->PostTask(
+      FROM_HERE, task_factory_.NewRunnableMethod(
+          &ChromotingScriptableObject::DoSignalConnectionInfoChange));
+}
+
+void ChromotingScriptableObject::SignalDesktopSizeChange() {
+  plugin_message_loop_->PostTask(
+      FROM_HERE, task_factory_.NewRunnableMethod(
+          &ChromotingScriptableObject::DoSignalDesktopSizeChange));
+}
+
+void ChromotingScriptableObject::SignalLoginChallenge() {
+  plugin_message_loop_->PostTask(
+      FROM_HERE, task_factory_.NewRunnableMethod(
+          &ChromotingScriptableObject::DoSignalLoginChallenge));
+}
+
+void ChromotingScriptableObject::DoSignalConnectionInfoChange() {
   Var exception;
   VarPrivate cb = GetProperty(Var(kConnectionInfoUpdate), &exception);
 
@@ -293,7 +314,8 @@ void ChromotingScriptableObject::SignalConnectionInfoChange() {
     LOG(ERROR) << "Exception when invoking connectionInfoUpdate JS callback.";
 }
 
-void ChromotingScriptableObject::SignalDesktopSizeChange() {
+
+void ChromotingScriptableObject::DoSignalDesktopSizeChange() {
   Var exception;
   VarPrivate cb = GetProperty(Var(kDesktopSizeUpdate), &exception);
 
@@ -307,7 +329,7 @@ void ChromotingScriptableObject::SignalDesktopSizeChange() {
   }
 }
 
-void ChromotingScriptableObject::SignalLoginChallenge() {
+void ChromotingScriptableObject::DoSignalLoginChallenge() {
   Var exception;
   VarPrivate cb = GetProperty(Var(kLoginChallenge), &exception);
 
diff --git a/remoting/client/plugin/chromoting_scriptable_object.h b/remoting/client/plugin/chromoting_scriptable_object.h
index 5a5127a..e34f2e1 100644
--- a/remoting/client/plugin/chromoting_scriptable_object.h
+++ b/remoting/client/plugin/chromoting_scriptable_object.h
@@ -115,11 +115,15 @@
 #include <string>
 #include <vector>
 
+#include "base/task.h"
 #include "base/memory/weak_ptr.h"
-
 #include "ppapi/cpp/dev/scriptable_object_deprecated.h"
 #include "ppapi/cpp/var.h"
 
+namespace base {
+class MessageLoopProxy;
+};  // namespace base
+
 namespace remoting {
 
 class ChromotingInstance;
@@ -144,7 +148,9 @@ class ChromotingScriptableObject
     : public pp::deprecated::ScriptableObject,
       public base::SupportsWeakPtr<ChromotingScriptableObject> {
  public:
-  explicit ChromotingScriptableObject(ChromotingInstance* instance);
+  ChromotingScriptableObject(
+      ChromotingInstance* instance,
+      base::MessageLoopProxy* plugin_message_loop);
   virtual ~ChromotingScriptableObject();
 
   virtual void Init();
@@ -205,13 +211,17 @@ class ChromotingScriptableObject
   void AddAttribute(const std::string& name, pp::Var attribute);
   void AddMethod(const std::string& name, MethodHandler handler);
 
-  // This should be called to signal the JS code that the connection status has
-  // changed.
   void SignalConnectionInfoChange();
-
-  // Signal the JS code that the desktop size has changed.
   void SignalDesktopSizeChange();
 
+  // Calls to these methods are posted to the plugin thread from
+  // corresponding Signal*() methods. They actually call JavaScript
+  // code. This is necessary becase JavaScript needs to be called with
+  // clean stack - JavaScript event handlers may destroy the plugin.
+  void DoSignalConnectionInfoChange();
+  void DoSignalDesktopSizeChange();
+  void DoSignalLoginChallenge();
+
   pp::Var DoConnect(const std::vector<pp::Var>& args, pp::Var* exception);
   pp::Var DoDisconnect(const std::vector<pp::Var>& args, pp::Var* exception);
 
@@ -235,6 +245,11 @@ class ChromotingScriptableObject
   scoped_refptr<PepperXmppProxy> xmpp_proxy_;
 
   ChromotingInstance* instance_;
+
+  scoped_refptr<base::MessageLoopProxy> plugin_message_loop_;
+  ScopedRunnableMethodFactory<ChromotingScriptableObject> task_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromotingScriptableObject);
 };
 
 }  // namespace remoting
diff --git a/remoting/protocol/connection_to_host.cc b/remoting/protocol/connection_to_host.cc
index 3b82499..8113366 100644
--- a/remoting/protocol/connection_to_host.cc
+++ b/remoting/protocol/connection_to_host.cc
@@ -177,7 +177,6 @@ void ConnectionToHost::OnSessionStateChange(
 
   switch (state) {
     case Session::FAILED:
-      state_ = STATE_FAILED;
       CloseOnError();
       break;
 
diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index 1f79b8e..3a906e2 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -55,7 +55,6 @@ JingleSession::JingleSession(
     : jingle_session_manager_(jingle_session_manager),
       local_cert_(local_cert),
       state_(INITIALIZING),
-      closed_(false),
       closing_(false),
       cricket_session_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
@@ -90,16 +89,9 @@ void JingleSession::Init(cricket::Session* cricket_session) {
 void JingleSession::CloseInternal(int result, bool failed) {
   DCHECK(CalledOnValidThread());
 
-  if (!closed_ && !closing_) {
+  if (state_ != FAILED && state_ != CLOSED && !closing_) {
     closing_ = true;
 
-    // Inform the StateChangeCallback, so calling code knows not to touch any
-    // channels.
-    if (failed)
-      SetState(FAILED);
-    else
-      SetState(CLOSED);
-
     control_channel_socket_.reset();
     event_channel_socket_.reset();
     STLDeleteContainerPairSecondPointers(channel_connectors_.begin(),
@@ -111,7 +103,13 @@ void JingleSession::CloseInternal(int result, bool failed) {
       cricket_session_->SignalState.disconnect(this);
     }
 
-    closed_ = true;
+    // Inform the StateChangeCallback, so calling code knows not to
+    // touch any channels. Needs to be done in the end because the
+    // session may be deleted in response to this event.
+    if (failed)
+      SetState(FAILED);
+    else
+      SetState(CLOSED);
   }
 }
 
@@ -124,7 +122,7 @@ cricket::Session* JingleSession::ReleaseSession() {
   DCHECK(CalledOnValidThread());
 
   // Session may be destroyed only after it is closed.
-  DCHECK(closed_);
+  DCHECK(state_ == FAILED || state_ == CLOSED);
 
   cricket::Session* session = cricket_session_;
   if (cricket_session_)
@@ -250,7 +248,7 @@ void JingleSession::OnSessionState(
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(cricket_session_, session);
 
-  if (closed_) {
+  if (state_ == FAILED || state_ == CLOSED) {
     // Don't do anything if we already closed.
     return;
   }
@@ -474,7 +472,7 @@ void JingleSession::SetState(State new_state) {
     DCHECK_NE(state_, FAILED);
 
     state_ = new_state;
-    if (!closed_ && state_change_callback_.get())
+    if (state_change_callback_.get())
       state_change_callback_->Run(new_state);
   }
 }
diff --git a/remoting/protocol/jingle_session.h b/remoting/protocol/jingle_session.h
index ee0a80e..288720d 100644
--- a/remoting/protocol/jingle_session.h
+++ b/remoting/protocol/jingle_session.h
@@ -150,7 +150,6 @@ class JingleSession : public protocol::Session,
   State state_;
   scoped_ptr<StateChangeCallback> state_change_callback_;
 
-  bool closed_;
   bool closing_;
 
   // JID of the other side. Set when the connection is initialized,