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;
+static scoped_refptr<base::MessageLoopProxy> g_logging_message_loop;

[Sergey Ulanov, 2012/05/29 18:58:30]

This adds a static finalizer. Use CR_DEFINE_STATIC_LOCAL() here.

[Sergey Ulanov, 2012/05/29 18:58:30]

SingleThreadTaskRunner please.

[Wez, 2012/05/30 00:18:39]

Done.

[Wez, 2012/05/30 00:18:39]

Done.

+static base::WeakPtr<ChromotingInstance> g_logging_instance;

[Sergey Ulanov, 2012/05/29 18:58:30]

This adds a static finalizer. Use CR_DEFINE_STATIC_LOCAL() to avoid it.

[Wez, 2012/05/30 00:18:39]

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();
 
   // 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_message_loop = 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_message_loop = 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_message_loop.get() &&
         // 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_message_loop->BelongsToCurrentThread() ||

[Sergey Ulanov, 2012/05/29 18:58:30]

Races are still possible in this code. The logging thread may reset
|g_logging_message_loop| after we verify g_logging_message_loop.get() and this
line. One way to solve it is to copy refptr to the stack and then use the copy
here. Or maybe add a lock, then |g_logging_message_loop| wouldn't have to be
global.

[Wez, 2012/05/30 00:18:39]

There is already a lock held at this point (g_logging_lock).  It would make
sense to grab the task runner reference in the lock and log outside it, though.

          !g_logging_to_plugin)) {
       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_message_loop->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