Remote assistance on Chrome OS Part IV - It2MeHost

remoting/host/it2me/it2me_native_messaging_host.cc

Index: remoting/host/it2me/it2me_native_messaging_host.cc
diff --git a/remoting/host/it2me/it2me_native_messaging_host.cc b/remoting/host/it2me/it2me_native_messaging_host.cc
index 24ddc1b8dc38c72475555f4c0dbc00ea915aa62d..b69a4ee979c6d5d33fecbad8a4ef768c351e63a8 100644
--- a/remoting/host/it2me/it2me_native_messaging_host.cc
+++ b/remoting/host/it2me/it2me_native_messaging_host.cc
@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
+#include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringize_macros.h"
 #include "base/threading/thread.h"
@@ -39,19 +40,35 @@ const remoting::protocol::NameMapElement<It2MeHostState> kIt2MeHostStates[] = {
 
 }  // namespace
 
+// static
+scoped_ptr<extensions::NativeMessageHost> It2MeNativeMessagingHost::Create() {
+  scoped_ptr<It2MeHostFactory> factory(new remoting::It2MeHostFactory());
+  scoped_refptr<ChromotingHostContext> context =
+      ChromotingHostContext::Create(new AutoThreadTaskRunner(
+          base::MessageLoopProxy::current(), base::Bind(&base::DoNothing)));
+  scoped_ptr<extensions::NativeMessageHost> host(new It2MeNativeMessagingHost(
+  // Remoting uses AutoThread as its threading primitive, which will
+  // exit itself (by posting a Quit message) when there are no more references
+  // to the task runner.
+  // On ChromeOS, the It2MeNativeMessagingHost is created on the UI
+  // thread of the browser process.  Since we do not want to quit the browser
+  // UI thread just because there are no more tasks in the task_runner,
+  // base::DoNothing is passed in as the quit closure.

[Wez, 2014/10/17 17:57:59]

This comment is in the wrong place, and misses the point.

AutoThreadTaskRunner is a TaskRunner with the special property that it will
continue to process tasks until no references remain, at least. The QuitClosure
we usually pass does the simple thing of stopping the underlying TaskRunner, but
in general the point is that the underlying TaskRunner _must not stop processing
tasks_ until that callback is invoked. So by passing DoNothing you're violating
that contract, and you may find that things break at browser shutdown.

[kelvinp, 2014/10/20 00:21:15]

I can update the comment.

Yes, I agree that this is a hack, but this is the best solution that I can come
across so far to reconcile the two contraditing threading ownership philosophies
in Chrome and remoting.  Chrome has an explicit ownership model in the sense
that those task runners should only be destroyed by the browser thread.  By
definition, the assumptions of the AutoThreadTaskRunner won't hold.

I have tested the shutdown scenarios extensively and I have found no crashes. 
There is only a leak in the url_request_context_getter when the entire chrome
process goes away, which I think it is ok.

+      context,
+      factory.Pass()));
+  return host.Pass();
+}
+
 It2MeNativeMessagingHost::It2MeNativeMessagingHost(
-    scoped_refptr<AutoThreadTaskRunner> task_runner,
+    scoped_refptr<ChromotingHostContext> context,
     scoped_ptr<It2MeHostFactory> factory)
     : client_(NULL),
       factory_(factory.Pass()),
       weak_factory_(this) {
   weak_ptr_ = weak_factory_.GetWeakPtr();
 
-  // Initialize the host context to manage the threads for the it2me host.
-  // The native messaging host, rather than the It2MeHost object, owns and
-  // maintains the lifetime of the host context.
-
-  host_context_.reset(ChromotingHostContext::Create(task_runner).release());
+  host_context_ = context;
+  task_runner_ = host_context_->ui_task_runner();
 
   const ServiceUrls* service_urls = ServiceUrls::GetInstance();
   const bool xmpp_server_valid =
@@ -203,7 +220,7 @@ void It2MeNativeMessagingHost::ProcessConnect(
 #endif  // !defined(NDEBUG)
 
   // Create the It2Me host and start connecting.
-  it2me_host_ = factory_->CreateIt2MeHost(host_context_.get(),
+  it2me_host_ = factory_->CreateIt2MeHost(host_context_,
                                           host_context_->ui_task_runner(),
                                           weak_ptr_,
                                           xmpp_config,