Provide Shill IP Address to myIpAddress()

chromeos/network/host_resolver_impl_chromeos.cc

Index: chromeos/network/host_resolver_impl_chromeos.cc
diff --git a/chromeos/network/host_resolver_impl_chromeos.cc b/chromeos/network/host_resolver_impl_chromeos.cc
new file mode 100644
index 0000000000000000000000000000000000000000..16af7d6a51966e1bcd0ee522f30eae57d76c3339
--- /dev/null
+++ b/chromeos/network/host_resolver_impl_chromeos.cc
@@ -0,0 +1,222 @@
+// Copyright (c) 2014 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 "chromeos/network/host_resolver_impl_chromeos.h"
+
+#include "base/message_loop/message_loop_proxy.h"
+#include "base/values.h"
+#include "chromeos/network/device_state.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_state_handler_observer.h"
+#include "net/base/address_list.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_util.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+// HostResolverImplChromeOS::NetworkStateHandlerObserver
+
+using chromeos::DeviceState;
+using chromeos::NetworkState;
+
+class net::HostResolverImplChromeOS::NetworkObserver
+    : public chromeos::NetworkStateHandlerObserver {
+ public:
+  NetworkObserver(scoped_refptr<base::MessageLoopProxy> owner_message_loop,
+                  chromeos::NetworkStateHandler* network_state_handler)
+      : owner_message_loop_(owner_message_loop),
+        network_state_handler_(network_state_handler),
+        weak_ptr_factory_owner_thread_(this) {
+    network_state_handler_->AddObserver(this, FROM_HERE);
+    DefaultNetworkChanged(network_state_handler_->DefaultNetwork());
+  }
+
+  virtual ~NetworkObserver() {
+    network_state_handler_->RemoveObserver(this, FROM_HERE);
+  }
+
+  // NetworkStateHandlerObserver
+  virtual void DefaultNetworkChanged(const NetworkState* network) OVERRIDE {
+    if (!network) {
+      DVLOG(2) << "DefaultNetworkChanged: No Network.";
+      CallSetIpAddressOnOwnerThread("", "");
+      return;
+    }
+    std::string ipv4_address, ipv6_address;
+    const DeviceState* device_state =
+        network_state_handler_->GetDeviceState(network->device_path());
+    if (!device_state) {
+      LOG(ERROR) << "DefaultNetworkChanged: Network missing device: "
+                 << network->path();
+      CallSetIpAddressOnOwnerThread("", "");
+      return;
+    }
+    for (base::DictionaryValue::Iterator iter(device_state->ip_configs());
+         !iter.IsAtEnd(); iter.Advance()) {
+      const base::DictionaryValue* ip_config;
+      if (!iter.value().GetAsDictionary(&ip_config)) {
+        LOG(ERROR) << "Badly formatted IPConfigs: " << network->path();
+        continue;
+      }
+      std::string method, address;
+      if (ip_config->GetString(shill::kMethodProperty, &method) &&
+          ip_config->GetString(shill::kAddressProperty, &address)) {
+        if (method == shill::kTypeIPv4 || method == shill::kTypeDHCP)
+          ipv4_address = address;
+        else if (method == shill::kTypeIPv6 || method == shill::kTypeDHCP6)
+          ipv6_address = address;
+      } else {
+        LOG(ERROR) << "DefaultNetworkChanged: IPConfigs missing properties: "
+                   << network->path();
+      }
+    }
+    DVLOG(2) << "DefaultNetworkChanged: " << network->name()
+             << " IPv4: " << ipv4_address << " IPv6: " << ipv6_address;
+    CallSetIpAddressOnOwnerThread(ipv4_address, ipv6_address);
+  }
+
+  // These may be safely accessed by the owner thread.
+  std::string ipv4_address() const {
+    return ipv4_address_;
+  }
+  std::string ipv6_address() const {
+    return ipv6_address_;
+  }
+
+ private:
+  void CallSetIpAddressOnOwnerThread(const std::string& ipv4_address,
+                                     const std::string& ipv6_address) {
+    owner_message_loop_->PostTask(
+        FROM_HERE,
+        base::Bind(&NetworkObserver::SetIPAddresses,
+                   weak_ptr_factory_owner_thread_.GetWeakPtr(),
+                   ipv4_address, ipv6_address));
+  }
+
+  void SetIPAddresses(const std::string& ipv4_address,
+                      const std::string& ipv6_address) {
+    ipv4_address_ = ipv4_address;
+    ipv6_address_ = ipv6_address;
+  }
+
+  std::string ipv4_address_;
+  std::string ipv6_address_;
+  scoped_refptr<base::MessageLoopProxy> owner_message_loop_;
+  chromeos::NetworkStateHandler* network_state_handler_;
+  base::WeakPtrFactory<NetworkObserver> weak_ptr_factory_owner_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkObserver);
+};
+
+namespace net {
+
+// HostResolverImplChromeOS
+
+// static
+scoped_ptr<HostResolver> HostResolverImplChromeOS::CreateSystemResolver(
+    const Options& options,
+    NetLog* net_log) {
+  scoped_ptr<HostCache> cache;
+  if (options.enable_caching)
+    cache = HostCache::CreateDefaultCache();
+  return scoped_ptr<HostResolver>(new HostResolverImplChromeOS(
+      chromeos::NetworkHandler::Get()->message_loop(),
+      chromeos::NetworkHandler::Get()->network_state_handler(),
+      cache.Pass(),
+      options.GetDispatcherLimits(),
+      ProcTaskParams(NULL, options.max_retry_attempts),

[eroman, 2014/06/05 22:58:24]

Can you use composition rather than inheritance, to avoid duplicating these
construction details, and needing to expose the dispatcher limits?

i.e. HostResolveImplChromeOS implements HostResolver, and owns a HostResolver
passed into it (which will likely have been constructed via
net::HostResolver::CreateSystemResolver().

[stevenjb, 2014/06/06 16:06:03]

The problem with that is that HostResolveImplChromeOS would have to implement
all eight HostResolver virtual methods (all but one of which would just be
forwarded to the owned HostResolverImpl).

While I agree that this is a bit clunky, I'd rather make a TODO to clean up the
HostResolver construction methods which I think could simplify this a lot (but I
didn't particularly want to stomp around in that code for this CL).

+      net_log));
+}
+
+// static
+scoped_ptr<HostResolver> HostResolverImplChromeOS::CreateHostResolverForTest(
+    scoped_refptr<base::MessageLoopProxy> ui_message_loop,
+    chromeos::NetworkStateHandler* network_state_handler) {
+  Options options;
+  scoped_ptr<HostCache> cache = HostCache::CreateDefaultCache();
+  return scoped_ptr<HostResolver>(new HostResolverImplChromeOS(
+      ui_message_loop,
+      network_state_handler,
+      cache.Pass(),
+      options.GetDispatcherLimits(),
+      ProcTaskParams(NULL, options.max_retry_attempts),
+      NULL));
+}
+
+HostResolverImplChromeOS::HostResolverImplChromeOS(
+    scoped_refptr<base::MessageLoopProxy> ui_message_loop,
+    chromeos::NetworkStateHandler* network_state_handler,
+    scoped_ptr<HostCache> cache,
+    const PrioritizedDispatcher::Limits& job_limits,
+    const ProcTaskParams& proc_params,
+    NetLog* net_log)
+    : HostResolverImpl(cache.Pass(), job_limits, proc_params, net_log),
+      ui_message_loop_(ui_message_loop),
+      weak_ptr_factory_ui_thread_(this) {
+  ui_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&HostResolverImplChromeOS::CreateNetworkObserver,
+                 weak_ptr_factory_ui_thread_.GetWeakPtr(),
+                 base::MessageLoopProxy::current(),
+                 network_state_handler));
+}
+
+HostResolverImplChromeOS::~HostResolverImplChromeOS() {
+  ui_message_loop_->DeleteSoon(

[eroman, 2014/06/05 22:58:24]

Note that this pattern can cause leaks during shutdown.

(1) browser shutdown starts, stopping UI message loop
(2) IO thread is stopped and joined
(3) Host resolver is destroyed from IO thread
(4) A task is posted to the UI loop to delete network_observer_ --> This doesn't
run.

[stevenjb, 2014/06/06 16:06:03]

Ugh, I didn't realize that IOThread::globals_ was getting destroyed after the
message loops were stopped. Nice catch. This is going to have to be a bit more
complicated in order to be robust, I think I need to add a Shutdown message from
NetworkHandler, but that seems worthwhile anyway. Will fix.

[stevenjb, 2014/06/06 17:24:18]

I realized quickly that a Shutdown message after the MessageLoop is stopped is
not going to be very helpful :)

It turns out that DeleteSoon returns true or false, so I think the simplest /
safest thing to do is to delete network_observer_ here if DeleteSoon returns
false. If the message loops are stopped, that will be safe.

+      FROM_HERE,
+      network_observer_.release());
+}
+
+void HostResolverImplChromeOS::CreateNetworkObserver(
+    scoped_refptr<base::MessageLoopProxy> io_message_loop,
+    chromeos::NetworkStateHandler* network_state_handler) {
+  network_observer_.reset(
+      new NetworkObserver(io_message_loop, network_state_handler));
+}
+
+int HostResolverImplChromeOS::Resolve(const RequestInfo& info,
+                                      RequestPriority priority,
+                                      AddressList* addresses,
+                                      const CompletionCallback& callback,
+                                      RequestHandle* out_req,
+                                      const BoundNetLog& source_net_log) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (ResolveLocalIPAddress(info, addresses))
+    return net::OK;
+  return HostResolverImpl::Resolve(
+      info, priority, addresses, callback, out_req, source_net_log);
+}
+
+bool HostResolverImplChromeOS::ResolveLocalIPAddress(const RequestInfo& info,
+                                                     AddressList* addresses) {
+  if (info.hostname() != GetHostName() || !network_observer_)

[eroman, 2014/06/05 22:58:24]

Is there going to be any other consequences of this change?

GetHostName() will be 127.0.0.1 right? So if a user was navigating to
http://127.0.0.1/ directly, that would now have a different meaning then before,
where an AddressList of 127.0.0.1 would have been used.

The alternative would be to have some indication in info on whether localhost is
requested.

[stevenjb, 2014/06/06 16:06:03]

I'm not familiar enough with network internals to know what the right thing to
do here is. I am happy to add the boolean if that is more correct. Who would
know?

[stevenjb, 2014/06/06 17:34:11]

So, on systems other than Chrome OS, isn't a localhost lookup going to resolve
to the actual IP address? I thought that was the whole point of this; I don't
see any special handling of the request in ProxyResolverV8Tracing other than
host_port.set_host(GetHostName()).

+    return false;
+
+  // Use IPConfig data for localhost address lookup.
+  addresses->clear();
+  std::string ipv6_address = network_observer_->ipv6_address();
+  if (info.address_family() != net::ADDRESS_FAMILY_IPV4 &&
+      !ipv6_address.empty()) {
+    IPAddressNumber ipv6;
+    if (ParseIPLiteralToNumber(ipv6_address, &ipv6))
+      addresses->push_back(IPEndPoint(ipv6, 0));
+  }
+  std::string ipv4_address = network_observer_->ipv4_address();
+  if (!ipv4_address.empty()) {
+    IPAddressNumber ipv4;
+    if (ParseIPLiteralToNumber(ipv4_address, &ipv4))
+      addresses->push_back(IPEndPoint(ipv4, 0));
+  }
+  DVLOG(2) << "ResolveLocalIPAddress("
+           << static_cast<int>(info.address_family()) << "): "
+           << addresses->size()
+           << " IPv4: " << ipv4_address << " IPv6: " << ipv6_address;
+  if (addresses->empty())
+    return false;
+  addresses->SetDefaultCanonicalName();
+  return true;
+}
+
+}  // namespace net