[cros] Make g_browser_process's call on the UI thread.

chrome/browser/extensions/extension_web_socket_proxy_private_api.cc

 // Copyright (c) 2011 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 "chrome/browser/extensions/extension_web_socket_proxy_private_api.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/values.h"
@@ skipping 45 matching lines @@
 #else
   NOTREACHED();
 #endif
 
   timer_.Stop();  // Cancel timeout timer.
   ResolveHost();
 }
 
 void WebSocketProxyPrivate::ResolveHost() {
 #if defined(OS_CHROMEOS)
-  if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) {
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&WebSocketProxyPrivate::ResolveHost, this));
-    return;
-  }
+  IOThread* io_thread = g_browser_process->io_thread();

[stevenjb, 2011/11/28 18:45:39]

One reason that g_browser_process->io_thread() is disallowed from other threads
is that access to the IOThread members are not inherently thread safe. This code
as written could cause a subtle shutdown crash when accessing io_thread from
ResolveHostIOPart().

Furthermore, resolver_ could get destroyed on shutdown after it is reset and
before it is accessed in ResolveHostIOPart() during shutdown. 

resolver_->Resolve() itself does not appear to be blocking, so I'm not sure why
it needs to be called from the IO thread? If it does, resolver_ should get set
on the UI thread, and some mechanism added to prevent resolver_ from being
deleted before it is accessed from the IO thread.

[altimofeev, 2011/11/29 12:30:20]

Please find my answers inline.

On 2011/11/28 18:45:39, Steven Bennetts wrote:
Few points because of which I think the code is correct:
1. There is 'DCHECK(Currently on IO thread)' in IOThread::globals()  getter. So
it is designed to be called on IOThread.
2. In case IOThread was deleted, PostMessage prevents us from running task on IO
thread, thus avoiding usage of the invalid pointer.
3. There are places in the code, where we are passing io_thread pointer (e.g.
profiles code)

Anyway, I'll be glad to hear the right way of getting host_resolver.
I think it cannot. The reason is that WebSocketProxyPrivate inherits from
RefCountedThreadSafe. And there is AddRef() call in RunImpl() method, which is
compensated only in Finalize() method.
 
I haven't found any DCHECKs, but since host_resolver belongs to the IO thread,
it is logically correct to use it on the IO thread too.

+  content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&WebSocketProxyPrivate::ResolveHostIOPart, this, io_thread));
+#endif
+}
+
+void WebSocketProxyPrivate::ResolveHostIOPart(IOThread* io_thread) {
+#if defined(OS_CHROMEOS)
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   DCHECK(resolver_ == NULL);
-  if (g_browser_process) {
-    IOThread* io_thread = g_browser_process->io_thread();
-    if (io_thread && io_thread->globals()) {
-      net::HostResolver* host_resolver =
-          io_thread->globals()->host_resolver.get();
-      if (host_resolver) {
-        resolver_.reset(new net::SingleRequestHostResolver(host_resolver));
-        net::HostResolver::RequestInfo info(net::HostPortPair(
-            hostname_, port_));
-        int result = resolver_->Resolve(info, &addr_,
-            base::Bind(&WebSocketProxyPrivate::OnHostResolution, this),
-            net::BoundNetLog());
-        if (result != net::ERR_IO_PENDING)
-          OnHostResolution(result);
-        return;
-      }
+  if (io_thread && io_thread->globals()) {
+    net::HostResolver* host_resolver =
+        io_thread->globals()->host_resolver.get();
+    if (host_resolver) {
+      resolver_.reset(new net::SingleRequestHostResolver(host_resolver));
+      net::HostResolver::RequestInfo info(net::HostPortPair(
+          hostname_, port_));
+      int result = resolver_->Resolve(info, &addr_,
+          base::Bind(&WebSocketProxyPrivate::OnHostResolution, this),
+          net::BoundNetLog());
+      if (result != net::ERR_IO_PENDING)
+        OnHostResolution(result);
+      return;
     }
   }
   NOTREACHED();
   OnHostResolution(net::ERR_UNEXPECTED);
 #endif
 }
 
 bool WebSocketProxyPrivate::RunImpl() {
   AddRef();
   result_.reset(Value::CreateStringValue(""));
@@ skipping 107 matching lines @@
 void WebSocketProxyPrivateGetPassportForTCPFunction::CustomFinalize() {
 #if defined(OS_CHROMEOS)
   std::string passport =
       browser::InternalAuthGeneration::GeneratePassport(
           "web_socket_proxy", map_) + std::string(":");
   if (ContainsKey(map_, "addr"))
     passport += map_["addr"];
   result_.reset(Value::CreateStringValue(passport));
 #endif
 }