Don't crash in ~HostNPScriptObject() when the object wasn't initialized.

BUG=112656

Review URL: https://chromiumcodereview.appspot.com/9582031

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

8 files changed, 61 insertions, 68 deletions

diff --git a/remoting/host/chromoting_host_context.cc b/remoting/host/chromoting_host_context.cc
index 6f02943..d66a4f2 100644
--- a/remoting/host/chromoting_host_context.cc
+++ b/remoting/host/chromoting_host_context.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -23,20 +23,10 @@ ChromotingHostContext::ChromotingHostContext(
 ChromotingHostContext::~ChromotingHostContext() {
 }
 
-void ChromotingHostContext::Start() {
+bool ChromotingHostContext::Start() {
   // Start all the threads.
-  main_thread_.Start();
-  encode_thread_.Start();
-  jingle_thread_.Start();
-  desktop_thread_.Start();
-}
-
-void ChromotingHostContext::Stop() {
-  // Stop all the threads.
-  jingle_thread_.Stop();
-  encode_thread_.Stop();
-  main_thread_.Stop();
-  desktop_thread_.Stop();
+  return main_thread_.Start() && encode_thread_.Start() &&
+      jingle_thread_.Start() && desktop_thread_.Start();
 }
 
 JingleThread* ChromotingHostContext::jingle_thread() {
diff --git a/remoting/host/chromoting_host_context.h b/remoting/host/chromoting_host_context.h
index 8b6a7d7..92db811 100644
--- a/remoting/host/chromoting_host_context.h
+++ b/remoting/host/chromoting_host_context.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -22,12 +22,12 @@ class ChromotingHostContext {
   explicit ChromotingHostContext(base::MessageLoopProxy* ui_message_loop);
   virtual ~ChromotingHostContext();
 
-  // TODO(ajwong): Move the Start/Stop methods out of this class. Then
+  // TODO(ajwong): Move the Start method out of this class. Then
   // create a static factory for construction, and destruction.  We
-  // should be able to remove the need for virtual functions below with that
-  // design, while preserving the relative simplicity of this API.
-  virtual void Start();
-  virtual void Stop();
+  // should be able to remove the need for virtual functions below
+  // with that design, while preserving the relative simplicity of
+  // this API.
+  virtual bool Start();
 
   virtual JingleThread* jingle_thread();
 
diff --git a/remoting/host/chromoting_host_context_unittest.cc b/remoting/host/chromoting_host_context_unittest.cc
index 3d857b3..59a2ec5 100644
--- a/remoting/host/chromoting_host_context_unittest.cc
+++ b/remoting/host/chromoting_host_context_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -20,11 +20,6 @@ TEST(ChromotingHostContextTest, StartAndStop) {
   EXPECT_TRUE(context.jingle_thread());
   EXPECT_TRUE(context.main_message_loop());
   EXPECT_TRUE(context.encode_message_loop());
-  context.Stop();
-
-  // Expect all the threads are stopped.
-  EXPECT_FALSE(context.main_thread_.IsRunning());
-  EXPECT_FALSE(context.encode_thread_.IsRunning());
 }
 
 }  // namespace remoting
diff --git a/remoting/host/host_mock_objects.h b/remoting/host/host_mock_objects.h
index f1b440f..9136f3f 100644
--- a/remoting/host/host_mock_objects.h
+++ b/remoting/host/host_mock_objects.h
@@ -81,7 +81,7 @@ class MockChromotingHostContext : public ChromotingHostContext {
   MockChromotingHostContext();
   virtual ~MockChromotingHostContext();
 
-  MOCK_METHOD0(Start, void());
+  MOCK_METHOD0(Start, bool());
   MOCK_METHOD0(Stop, void());
   MOCK_METHOD0(jingle_thread, JingleThread*());
   MOCK_METHOD0(ui_message_loop, base::MessageLoopProxy*());
diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc
index 705b6eb..d25f89c 100644
--- a/remoting/host/plugin/host_script_object.cc
+++ b/remoting/host/plugin/host_script_object.cc
@@ -67,7 +67,6 @@ HostNPScriptObject::HostNPScriptObject(
       np_thread_id_(base::PlatformThread::CurrentId()),
       plugin_message_loop_proxy_(
           new PluginMessageLoopProxy(plugin_thread_delegate)),
-      host_context_(plugin_message_loop_proxy_),
       failed_login_attempts_(0),
       daemon_controller_(DaemonController::Create()),
       disconnected_event_(true, false),
@@ -97,25 +96,32 @@ HostNPScriptObject::~HostNPScriptObject() {
     nat_policy_.reset();
   }
 
-  // Disconnect synchronously. We cannot disconnect asynchronously
-  // here because |host_context_| needs to be stopped on the plugin
-  // thread, but the plugin thread may not exist after the instance
-  // is destroyed.
-  disconnected_event_.Reset();
-  DisconnectInternal();
-  disconnected_event_.Wait();
+  if (host_context_.get()) {
+    // Disconnect synchronously. We cannot disconnect asynchronously
+    // here because |host_context_| needs to be stopped on the plugin
+    // thread, but the plugin thread may not exist after the instance
+    // is destroyed.
+    disconnected_event_.Reset();
+    DisconnectInternal();
+    disconnected_event_.Wait();
 
-  // Stop all threads.
-  host_context_.Stop();
+    // Stops all threads.
+    host_context_.reset();
+  }
 }
 
 bool HostNPScriptObject::Init() {
   DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread());
   VLOG(2) << "Init";
-  // TODO(wez): This starts a bunch of threads, which might fail.
-  host_context_.Start();
+
+  host_context_.reset(new ChromotingHostContext(plugin_message_loop_proxy_));
+  if (!host_context_->Start()) {
+    host_context_.reset();
+    return false;
+  }
+
   nat_policy_.reset(
-      policy_hack::NatPolicy::Create(host_context_.network_message_loop()));
+      policy_hack::NatPolicy::Create(host_context_->network_message_loop()));
   nat_policy_->StartWatching(
       base::Bind(&HostNPScriptObject::OnNatPolicyUpdate,
                  base::Unretained(this)));
@@ -334,7 +340,7 @@ bool HostNPScriptObject::Enumerate(std::vector<std::string>* values) {
 }
 
 void HostNPScriptObject::OnAccessDenied(const std::string& jid) {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
 
   ++failed_login_attempts_;
   if (failed_login_attempts_ == kMaxLoginAttempts) {
@@ -343,7 +349,7 @@ void HostNPScriptObject::OnAccessDenied(const std::string& jid) {
 }
 
 void HostNPScriptObject::OnClientAuthenticated(const std::string& jid) {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
 
   if (state_ == kDisconnecting) {
     // Ignore the new connection if we are disconnecting.
@@ -359,13 +365,13 @@ void HostNPScriptObject::OnClientAuthenticated(const std::string& jid) {
 }
 
 void HostNPScriptObject::OnClientDisconnected(const std::string& jid) {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
   client_username_.clear();
   DisconnectInternal();
 }
 
 void HostNPScriptObject::OnShutdown() {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
 
   register_request_.reset();
   log_to_server_.reset();
@@ -420,8 +426,8 @@ bool HostNPScriptObject::Connect(const NPVariant* args,
 void HostNPScriptObject::ReadPolicyAndConnect(const std::string& uid,
                                               const std::string& auth_token,
                                               const std::string& auth_service) {
-  if (!host_context_.network_message_loop()->BelongsToCurrentThread()) {
-    host_context_.network_message_loop()->PostTask(
+  if (!host_context_->network_message_loop()->BelongsToCurrentThread()) {
+    host_context_->network_message_loop()->PostTask(
         FROM_HERE, base::Bind(
             &HostNPScriptObject::ReadPolicyAndConnect, base::Unretained(this),
             uid, auth_token, auth_service));
@@ -446,8 +452,8 @@ void HostNPScriptObject::FinishConnectMainThread(
     const std::string& uid,
     const std::string& auth_token,
     const std::string& auth_service) {
-  if (host_context_.main_message_loop() != MessageLoop::current()) {
-    host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+  if (host_context_->main_message_loop() != MessageLoop::current()) {
+    host_context_->main_message_loop()->PostTask(FROM_HERE, base::Bind(
         &HostNPScriptObject::FinishConnectMainThread, base::Unretained(this),
         uid, auth_token, auth_service));
     return;
@@ -458,7 +464,7 @@ void HostNPScriptObject::FinishConnectMainThread(
   // TODO(sergeyu): Fix DesktopEnvironment so that it can be created
   // on either the UI or the network thread so that we can avoid
   // jumping to the main thread here.
-  desktop_environment_.reset(DesktopEnvironment::Create(&host_context_));
+  desktop_environment_.reset(DesktopEnvironment::Create(host_context_.get()));
 
   FinishConnectNetworkThread(uid, auth_token, auth_service);
 }
@@ -467,8 +473,8 @@ void HostNPScriptObject::FinishConnectNetworkThread(
     const std::string& uid,
     const std::string& auth_token,
     const std::string& auth_service) {
-  if (!host_context_.network_message_loop()->BelongsToCurrentThread()) {
-    host_context_.network_message_loop()->PostTask(FROM_HERE, base::Bind(
+  if (!host_context_->network_message_loop()->BelongsToCurrentThread()) {
+    host_context_->network_message_loop()->PostTask(FROM_HERE, base::Bind(
         &HostNPScriptObject::FinishConnectNetworkThread, base::Unretained(this),
         uid, auth_token, auth_service));
     return;
@@ -491,7 +497,7 @@ void HostNPScriptObject::FinishConnectNetworkThread(
 
   // Create XMPP connection.
   scoped_ptr<SignalStrategy> signal_strategy(
-      new XmppSignalStrategy(host_context_.jingle_thread(), uid,
+      new XmppSignalStrategy(host_context_->jingle_thread(), uid,
                              auth_token, auth_service));
 
   // Request registration of the host for support.
@@ -508,13 +514,13 @@ void HostNPScriptObject::FinishConnectNetworkThread(
   // Create the Host.
   LOG(INFO) << "NAT state: " << nat_traversal_enabled_;
   host_ = new ChromotingHost(
-      &host_context_, signal_strategy_.get(), desktop_environment_.get(),
+      host_context_.get(), signal_strategy_.get(), desktop_environment_.get(),
       protocol::NetworkSettings(nat_traversal_enabled_));
   host_->AddStatusObserver(this);
   log_to_server_.reset(
       new LogToServer(host_, ServerLogEntry::IT2ME, signal_strategy_.get()));
   it2me_host_user_interface_.reset(
-      new It2MeHostUserInterface(host_.get(), &host_context_));
+      new It2MeHostUserInterface(host_.get(), host_context_.get()));
   it2me_host_user_interface_->Init();
 
   {
@@ -605,8 +611,8 @@ bool HostNPScriptObject::StopDaemon(const NPVariant* args,
 }
 
 void HostNPScriptObject::DisconnectInternal() {
-  if (!host_context_.network_message_loop()->BelongsToCurrentThread()) {
-    host_context_.network_message_loop()->PostTask(
+  if (!host_context_->network_message_loop()->BelongsToCurrentThread()) {
+    host_context_->network_message_loop()->PostTask(
         FROM_HERE, base::Bind(&HostNPScriptObject::DisconnectInternal,
                               base::Unretained(this)));
     return;
@@ -634,7 +640,7 @@ void HostNPScriptObject::DisconnectInternal() {
       // synchronously, bug SignalStrategy::Listener handlers are not
       // allowed to destroy SignalStrategy, so post task to call
       // Shutdown() later.
-      host_context_.network_message_loop()->PostTask(
+      host_context_->network_message_loop()->PostTask(
           FROM_HERE, base::Bind(
               &ChromotingHost::Shutdown, host_,
               base::Bind(&HostNPScriptObject::OnShutdownFinished,
@@ -643,14 +649,14 @@ void HostNPScriptObject::DisconnectInternal() {
 }
 
 void HostNPScriptObject::OnShutdownFinished() {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
 
   disconnected_event_.Signal();
 }
 
 void HostNPScriptObject::OnNatPolicyUpdate(bool nat_traversal_enabled) {
-  if (!host_context_.network_message_loop()->BelongsToCurrentThread()) {
-    host_context_.network_message_loop()->PostTask(
+  if (!host_context_->network_message_loop()->BelongsToCurrentThread()) {
+    host_context_->network_message_loop()->PostTask(
         FROM_HERE,
         base::Bind(&HostNPScriptObject::OnNatPolicyUpdate,
                    base::Unretained(this), nat_traversal_enabled));
@@ -683,7 +689,7 @@ void HostNPScriptObject::OnReceivedSupportID(
     bool success,
     const std::string& support_id,
     const base::TimeDelta& lifetime) {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
 
   if (!success) {
     SetState(kError);
@@ -709,7 +715,7 @@ void HostNPScriptObject::OnReceivedSupportID(
 }
 
 void HostNPScriptObject::SetState(State state) {
-  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
+  DCHECK(host_context_->network_message_loop()->BelongsToCurrentThread());
   switch (state_) {
     case kDisconnected:
       DCHECK(state == kStarting ||
diff --git a/remoting/host/plugin/host_script_object.h b/remoting/host/plugin/host_script_object.h
index b72df51..e410292 100644
--- a/remoting/host/plugin/host_script_object.h
+++ b/remoting/host/plugin/host_script_object.h
@@ -194,7 +194,7 @@ class HostNPScriptObject : public HostStatusObserver {
   base::PlatformThreadId np_thread_id_;
   scoped_refptr<PluginMessageLoopProxy> plugin_message_loop_proxy_;
 
-  ChromotingHostContext host_context_;
+  scoped_ptr<ChromotingHostContext> host_context_;
   HostKeyPair host_key_pair_;
   scoped_ptr<SignalStrategy> signal_strategy_;
   scoped_ptr<RegisterSupportHostRequest> register_request_;
diff --git a/remoting/jingle_glue/jingle_thread.cc b/remoting/jingle_glue/jingle_thread.cc
index 742e3b1..74a5a7b 100644
--- a/remoting/jingle_glue/jingle_thread.cc
+++ b/remoting/jingle_glue/jingle_thread.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -153,9 +153,11 @@ JingleThread::~JingleThread() {
     Stop();
 }
 
-void JingleThread::Start() {
-  Thread::Start();
+bool JingleThread::Start() {
+  if (!Thread::Start())
+    return false;
   started_event_.Wait();
+  return true;
 }
 
 void JingleThread::Run() {
diff --git a/remoting/jingle_glue/jingle_thread.h b/remoting/jingle_glue/jingle_thread.h
index 0d6018e..7c3aba0 100644
--- a/remoting/jingle_glue/jingle_thread.h
+++ b/remoting/jingle_glue/jingle_thread.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -47,7 +47,7 @@ class JingleThread : public talk_base::Thread {
   JingleThread();
   virtual ~JingleThread();
 
-  void Start();
+  bool Start();
 
   // Main function for the thread. Should not be called directly.
   virtual void Run() OVERRIDE;