Replace ScopedThreadProxy with MessageLoopProxy & WeakPtrs.

remoting/client/plugin/chromoting_instance.cc

 // 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.
 
 #include "remoting/client/plugin/chromoting_instance.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 79 matching lines @@
   return "";
 }
 
 }  // namespace
 
 // This flag blocks LOGs to the UI if we're already in the middle of logging
 // to the UI. This prevents a potential infinite loop if we encounter an error
 // while sending the log message to the UI.
 static bool g_logging_to_plugin = false;
 static bool g_has_logging_instance = false;
-static ChromotingInstance* g_logging_instance = NULL;
+CR_DEFINE_STATIC_LOCAL(scoped_refptr<base::SingleThreadTaskRunner>,
+                       g_logging_task_runner, ());
+CR_DEFINE_STATIC_LOCAL(base::WeakPtr<ChromotingInstance>, g_logging_instance,

[Sergey Ulanov, 2012/05/30 00:30:37]

nit: Might be more readable if formatted as follows:
CR_DEFINE_STATIC_LOCAL(
    base::WeakPtr<ChromotingInstance>, g_logging_instance, ());

[Wez, 2012/05/30 00:54:02]

Done.

+                       ());
 static logging::LogMessageHandlerFunction g_logging_old_handler = NULL;
 
 static base::LazyInstance<base::Lock>::Leaky
     g_logging_lock = LAZY_INSTANCE_INITIALIZER;
 
 // String sent in the "hello" message to the plugin to describe features.
 const char ChromotingInstance::kApiFeatures[] =
     "highQualityScaling injectKeyEvent sendClipboardItem remapKey trapKey "
     "notifyClientDimensions pauseVideo";
 
@@ skipping 21 matching lines @@
 
   return true;
 }
 
 ChromotingInstance::ChromotingInstance(PP_Instance pp_instance)
     : pp::InstancePrivate(pp_instance),
       initialized_(false),
       plugin_message_loop_(
           new PluginMessageLoopProxy(&plugin_thread_delegate_)),
       context_(plugin_message_loop_),
-      thread_proxy_(new ScopedThreadProxy(plugin_message_loop_)) {
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE | PP_INPUTEVENT_CLASS_WHEEL);
   RequestFilteringInputEvents(PP_INPUTEVENT_CLASS_KEYBOARD);
 
   // Resister this instance to handle debug log messsages.
   RegisterLoggingInstance();
 
   // Send hello message.
   scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue());
   data->SetInteger("apiVersion", kApiVersion);
   data->SetString("apiFeatures", kApiFeatures);
   data->SetInteger("apiMinVersion", kApiMinMessagingVersion);
   PostChromotingMessage("hello", data.Pass());
 }
 
 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();
+  //thread_proxy_->Detach();

[Sergey Ulanov, 2012/05/30 00:30:37]

Did you mean to make this part for this CL?
I'm not sure this is always safe. The thread proxy may outlive this object, but
the delegate that is passed to it is destroyed shortly after this destructor
returns.

[Wez, 2012/05/30 00:54:02]

We now identify the ChromotingInstance to direct logging to via a WeakPtr, so
once the WeakFactory is deleted, any queued events become no-ops - there's no
need to explicitly invalidate/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();
 
   if (client_.get()) {
     base::WaitableEvent done_event(true, false);
     client_->Stop(base::Bind(&base::WaitableEvent::Signal,
                              base::Unretained(&done_event)));
     done_event.Wait();
   }
 
   // Stopping the context shuts down all chromoting threads.
   context_.Stop();
 
-  // Delete |thread_proxy_| before we detach |plugin_message_loop_|,
-  // otherwise ScopedThreadProxy may DCHECK when being destroyed.
-  thread_proxy_.reset();
-
+  // Ensure that nothing touches the plugin thread delegate after this point.
   plugin_message_loop_->Detach();
 }
 
 bool ChromotingInstance::Init(uint32_t argc,
                               const char* argn[],
                               const char* argv[]) {
   CHECK(!initialized_);
   initialized_ = true;
 
   VLOG(1) << "Started ChromotingInstance::Init";
@@ skipping 415 matching lines @@
   logging::SetLogMessageHandler(&LogToUI);
 }
 
 void ChromotingInstance::RegisterLoggingInstance() {
   base::AutoLock lock(g_logging_lock.Get());
 
   // Register this instance as the one that will handle all logging calls
   // 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 = this;
+  g_logging_instance = weak_factory_.GetWeakPtr();
+  g_logging_task_runner = plugin_message_loop_;
   g_has_logging_instance = true;
 }
 
 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)
+  if (this != g_logging_instance.get())
     return;
 
   // Unregister this instance for logging.
   g_has_logging_instance = false;
-  g_logging_instance = NULL;
+  g_logging_instance.reset();
+  g_logging_task_runner = NULL;
 
   VLOG(1) << "Unregistering global log handler";
 }
 
 // static
 bool ChromotingInstance::LogToUI(int severity, const char* file, int line,
                                  size_t message_start,
                                  const std::string& str) {
   // Note that we're reading |g_has_logging_instance| outside of a lock.
   // This lockless read is done so that we don't needlessly slow down global
   // logging with a lock for each log message.
   //
   // This lockless read is safe because:
   //
   // Misreading a false value (when it should be true) means that we'll simply
   // skip processing a few log messages.
   //
   // Misreading a true value (when it should be false) means that we'll take
   // 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) {
-    // 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)) {
+    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 action was generated while processing a previous one, in which
+      // case we

[Wez, 2012/05/30 00:54:02]

Gah, uploaded mid-edit!

+      // 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.
+      if (!g_logging_task_runner->BelongsToCurrentThread() ||
+          !g_logging_to_plugin) {
+        logging_task_runner = g_logging_task_runner;
+      }
+    }
+
+    if (logging_task_runner.get()) {
       std::string message = remoting::GetTimestampString();
       message += (str.c_str() + message_start);
-      // |thread_proxy_| is safe to use here because we detach it before
-      // tearing down the |g_logging_instance|.
-      g_logging_instance->thread_proxy_->PostTask(
+
+      g_logging_task_runner->PostTask(
           FROM_HERE, base::Bind(&ChromotingInstance::ProcessLogToUI,
-                                base::Unretained(g_logging_instance), message));
+                                g_logging_instance, message));
     }
   }
 
   if (g_logging_old_handler)
     return (g_logging_old_handler)(severity, file, line, message_start, str);
   return false;
 }
 
 void ChromotingInstance::ProcessLogToUI(const std::string& message) {
   DCHECK(plugin_message_loop_->BelongsToCurrentThread());
@@ skipping 12 matching lines @@
   }
   g_logging_to_plugin = false;
 }
 
 bool ChromotingInstance::IsConnected() {
   return host_connection_.get() &&
     (host_connection_->state() == protocol::ConnectionToHost::CONNECTED);
 }
 
 }  // namespace remoting