Revert 139623 - Replace ScopedThreadProxy with MessageLoopProxy & WeakPtrs.

This affects the following classes:
* ChromotingClient
* ChromotingInstance
* HostUserInterface
* It2MeHostUserInterface

The MessageLoopProxy/WeakPtr combination requires that the WeakPtr is created on the thread referred to by the proxy; code in which that is hard to arrange usually has subtle race-conditions.

TEST=Existing unit-tests, and manual testing.


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

TBR=wez@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10446088

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

4 files changed, 38 insertions, 43 deletions

diff --git a/remoting/client/chromoting_client.cc b/remoting/client/chromoting_client.cc
index 2a3aa56..4dabdd2 100644
--- a/remoting/client/chromoting_client.cc
+++ b/remoting/client/chromoting_client.cc
@@ -41,7 +41,7 @@ ChromotingClient::ChromotingClient(const ClientConfig& config,
       client_done_(client_done),
       packet_being_processed_(false),
       last_sequence_number_(0),
-      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      thread_proxy_(context_->network_message_loop()) {
 }
 
 ChromotingClient::~ChromotingClient() {
@@ -52,9 +52,6 @@ void ChromotingClient::Start(
     scoped_ptr<protocol::TransportFactory> transport_factory) {
   DCHECK(message_loop()->BelongsToCurrentThread());
 
-  // Create a WeakPtr to ourself for to use for all posted tasks.
-  weak_ptr_ = weak_factory_.GetWeakPtr();
-
   scoped_ptr<protocol::Authenticator> authenticator;
   if (config_.use_v1_authenticator) {
     authenticator.reset(new protocol::V1ClientAuthenticator(
@@ -78,7 +75,7 @@ void ChromotingClient::Stop(const base::Closure& shutdown_task) {
   if (!message_loop()->BelongsToCurrentThread()) {
     message_loop()->PostTask(
         FROM_HERE, base::Bind(&ChromotingClient::Stop,
-                              weak_ptr_, shutdown_task));
+                              base::Unretained(this), shutdown_task));
     return;
   }
 
@@ -90,7 +87,7 @@ void ChromotingClient::Stop(const base::Closure& shutdown_task) {
   }
 
   connection_->Disconnect(base::Bind(&ChromotingClient::OnDisconnected,
-                                     weak_ptr_, shutdown_task));
+                                     base::Unretained(this), shutdown_task));
 }
 
 void ChromotingClient::OnDisconnected(const base::Closure& shutdown_task) {
@@ -195,7 +192,7 @@ base::MessageLoopProxy* ChromotingClient::message_loop() {
 void ChromotingClient::OnPacketDone(bool last_packet,
                                     base::Time decode_start) {
   if (!message_loop()->BelongsToCurrentThread()) {
-    message_loop()->PostTask(FROM_HERE, base::Bind(
+    thread_proxy_.PostTask(FROM_HERE, base::Bind(
         &ChromotingClient::OnPacketDone, base::Unretained(this),
         last_packet, decode_start));
     return;
@@ -219,8 +216,8 @@ void ChromotingClient::OnPacketDone(bool last_packet,
 
 void ChromotingClient::Initialize() {
   if (!message_loop()->BelongsToCurrentThread()) {
-    message_loop()->PostTask(FROM_HERE, base::Bind(
-        &ChromotingClient::Initialize, weak_ptr_));
+    thread_proxy_.PostTask(FROM_HERE, base::Bind(
+        &ChromotingClient::Initialize, base::Unretained(this)));
     return;
   }
 
diff --git a/remoting/client/chromoting_client.h b/remoting/client/chromoting_client.h
index 7b90be3..d784b5a 100644
--- a/remoting/client/chromoting_client.h
+++ b/remoting/client/chromoting_client.h
@@ -10,8 +10,8 @@
 #include <list>
 
 #include "base/callback.h"
-#include "base/memory/weak_ptr.h"
 #include "base/time.h"
+#include "remoting/base/scoped_thread_proxy.h"
 #include "remoting/client/client_config.h"
 #include "remoting/client/chromoting_stats.h"
 #include "remoting/client/chromoting_view.h"
@@ -120,9 +120,7 @@ class ChromotingClient : public protocol::ConnectionToHost::HostEventCallback,
   // Keep track of the last sequence number bounced back from the host.
   int64 last_sequence_number_;
 
-  // WeakPtr used to avoid tasks accessing the client after it is deleted.
-  base::WeakPtrFactory<ChromotingClient> weak_factory_;
-  base::WeakPtr<ChromotingClient> weak_ptr_;
+  ScopedThreadProxy thread_proxy_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromotingClient);
 };
diff --git a/remoting/client/plugin/chromoting_instance.cc b/remoting/client/plugin/chromoting_instance.cc
index 28556b4..fec261e 100644
--- a/remoting/client/plugin/chromoting_instance.cc
+++ b/remoting/client/plugin/chromoting_instance.cc
@@ -97,10 +97,7 @@ std::string ConnectionErrorToString(ChromotingInstance::ConnectionError error) {
 // while sending the log message to the UI.
 static bool g_logging_to_plugin = false;
 static bool g_has_logging_instance = false;
-CR_DEFINE_STATIC_LOCAL(
-    scoped_refptr<base::SingleThreadTaskRunner>, g_logging_task_runner, ());
-CR_DEFINE_STATIC_LOCAL(
-    base::WeakPtr<ChromotingInstance>, g_logging_instance, ());
+static ChromotingInstance* g_logging_instance = NULL;
 static logging::LogMessageHandlerFunction g_logging_old_handler = NULL;
 
 static base::LazyInstance<base::Lock>::Leaky
@@ -142,7 +139,7 @@ ChromotingInstance::ChromotingInstance(PP_Instance pp_instance)
       plugin_message_loop_(
           new PluginMessageLoopProxy(&plugin_thread_delegate_)),
       context_(plugin_message_loop_),
-      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      thread_proxy_(new ScopedThreadProxy(plugin_message_loop_)) {
   RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE | PP_INPUTEVENT_CLASS_WHEEL);
   RequestFilteringInputEvents(PP_INPUTEVENT_CLASS_KEYBOARD);
 
@@ -160,6 +157,10 @@ ChromotingInstance::ChromotingInstance(PP_Instance pp_instance)
 ChromotingInstance::~ChromotingInstance() {
   DCHECK(plugin_message_loop_->BelongsToCurrentThread());
 
+  // Detach the log proxy so we don't log anything else to the UI.
+  // This needs to be done before the instance is unregistered for logging.
+  thread_proxy_->Detach();
+
   // Unregister this instance so that debug log messages will no longer be sent
   // to it. This will stop all logging in all Chromoting instances.
   UnregisterLoggingInstance();
@@ -174,7 +175,10 @@ ChromotingInstance::~ChromotingInstance() {
   // Stopping the context shuts down all chromoting threads.
   context_.Stop();
 
-  // Ensure that nothing touches the plugin thread delegate after this point.
+  // Delete |thread_proxy_| before we detach |plugin_message_loop_|,
+  // otherwise ScopedThreadProxy may DCHECK when being destroyed.
+  thread_proxy_.reset();
+
   plugin_message_loop_->Detach();
 }
 
@@ -610,8 +614,7 @@ void ChromotingInstance::RegisterLoggingInstance() {
   // and display them to the user.
   // If multiple plugins are run, then the last one registered will handle all
   // logging for all instances.
-  g_logging_instance = weak_factory_.GetWeakPtr();
-  g_logging_task_runner = plugin_message_loop_;
+  g_logging_instance = this;
   g_has_logging_instance = true;
 }
 
@@ -619,13 +622,12 @@ void ChromotingInstance::UnregisterLoggingInstance() {
   base::AutoLock lock(g_logging_lock.Get());
 
   // Don't unregister unless we're the currently registered instance.
-  if (this != g_logging_instance.get())
+  if (this != g_logging_instance)
     return;
 
   // Unregister this instance for logging.
   g_has_logging_instance = false;
-  g_logging_instance.reset();
-  g_logging_task_runner = NULL;
+  g_logging_instance = NULL;
 
   VLOG(1) << "Unregistering global log handler";
 }
@@ -647,26 +649,24 @@ bool ChromotingInstance::LogToUI(int severity, const char* file, int line,
   // the lock and check |g_logging_instance| unnecessarily. This is not
   // problematic because we always set |g_logging_instance| inside a lock.
   if (g_has_logging_instance) {
-    scoped_refptr<base::SingleThreadTaskRunner> logging_task_runner;
-
-    {
-      base::AutoLock lock(g_logging_lock.Get());
-      // If we're on the logging thread and |g_logging_to_plugin| is set then
-      // this LOG message came from handling a previous LOG message and we
-      // should skip it to avoid an infinite loop of LOG messages.
-      if (!g_logging_task_runner->BelongsToCurrentThread() ||
-          !g_logging_to_plugin) {
-        logging_task_runner = g_logging_task_runner;
-      }
-    }
-
-    if (logging_task_runner.get()) {
+    // Do not LOG anything while holding this lock or else the code will
+    // deadlock while trying to re-get the lock we're already in.
+    base::AutoLock lock(g_logging_lock.Get());
+    if (g_logging_instance &&
+        // If |g_logging_to_plugin| is set and we're on the logging thread, then
+        // this LOG message came from handling a previous LOG message and we
+        // should skip it to avoid an infinite loop of LOG messages.
+        // We don't have a lock around |g_in_processtoui|, but that's OK since
+        // the value is only read/written on the logging thread.
+        (!g_logging_instance->plugin_message_loop_->BelongsToCurrentThread() ||
+         !g_logging_to_plugin)) {
       std::string message = remoting::GetTimestampString();
       message += (str.c_str() + message_start);
-
-      g_logging_task_runner->PostTask(
+      // |thread_proxy_| is safe to use here because we detach it before
+      // tearing down the |g_logging_instance|.
+      g_logging_instance->thread_proxy_->PostTask(
           FROM_HERE, base::Bind(&ChromotingInstance::ProcessLogToUI,
-                                g_logging_instance, message));
+                                base::Unretained(g_logging_instance), message));
     }
   }
 
@@ -681,7 +681,6 @@ void ChromotingInstance::ProcessLogToUI(const std::string& message) {
   // This flag (which is set only here) is used to prevent LogToUI from posting
   // new tasks while we're in the middle of servicing a LOG call. This can
   // happen if the call to LogDebugInfo tries to LOG anything.
-  // Since it is read on the plugin thread, we don't need to lock to set it.
   g_logging_to_plugin = true;
   ChromotingScriptableObject* scriptable_object = GetScriptableObject();
   if (scriptable_object) {
diff --git a/remoting/client/plugin/chromoting_instance.h b/remoting/client/plugin/chromoting_instance.h
index 0f1bdde..a489bde 100644
--- a/remoting/client/plugin/chromoting_instance.h
+++ b/remoting/client/plugin/chromoting_instance.h
@@ -25,6 +25,7 @@
 #endif
 
 #include "ppapi/cpp/private/instance_private.h"
+#include "remoting/base/scoped_thread_proxy.h"
 #include "remoting/client/client_context.h"
 #include "remoting/client/key_event_mapper.h"
 #include "remoting/client/plugin/mac_key_event_processor.h"
@@ -224,7 +225,7 @@ class ChromotingInstance :
   // This wraps a ChromotingScriptableObject in a pp::Var.
   pp::Var instance_object_;
 
-  base::WeakPtrFactory<ChromotingInstance> weak_factory_;
+  scoped_ptr<ScopedThreadProxy> thread_proxy_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromotingInstance);
 };