Provide Shill IP Address to myIpAddress()

chromeos/network/host_resolver_impl_chromeos.cc

+// 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
+
+namespace chromeos {
+
+class HostResolverImplChromeOS::NetworkObserver
+    : public chromeos::NetworkStateHandlerObserver {
+ public:
+  NetworkObserver(scoped_refptr<base::MessageLoopProxy> owner_message_loop,
+                  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_;
+  NetworkStateHandler* network_state_handler_;
+  base::WeakPtrFactory<NetworkObserver> weak_ptr_factory_owner_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkObserver);
+};
+
+// HostResolverImplChromeOS
+
+// TODO(stevenjb)/eroman: Consider re-factoring the HostResolverImpl
+// construction to reduce duplicate code.

[eroman, 2014/06/09 22:37:11]

I still think the right abstraction is to compose. There are 8 virtual methods
that need to be forwarded, but the code to do so is very straight forward.

I am concerned that duplicating the construction parameters for HostResolverImpl
is a mistake waiting to happen. Someone might refactor HostResolverImpl and not
know to update HostResovlerImplChromeOS causing it to now behave differently.
Rather than have to rely on someone doing the right thing, I prefer if we are
using HostResolverImpl directly.

[stevenjb, 2014/06/10 22:04:55]

While I understand your concern, I think composition here would be a mistake. If
a virtual method were added to HostResolver with a default implementation (it is
already not a pure interface), but with a different implementation in
HostResolverImpl, this class would compile but do the wrong thing (calling the
default implementation instead of forwarding).

I would rather attempt to re-factor the constructor instead of using composition
here, I will take a pass at that and see what you think.

+// static
+scoped_ptr<net::HostResolver> HostResolverImplChromeOS::CreateSystemResolver(
+    const Options& options,
+    net::NetLog* net_log) {
+  scoped_ptr<net::HostCache> cache;
+  if (options.enable_caching)
+    cache = net::HostCache::CreateDefaultCache();
+  return scoped_ptr<net::HostResolver>(new HostResolverImplChromeOS(
+      NetworkHandler::Get()->message_loop(),
+      NetworkHandler::Get()->network_state_handler(),
+      cache.Pass(),
+      options.GetDispatcherLimits(),
+      ProcTaskParams(NULL, options.max_retry_attempts),
+      net_log));
+}
+
+// static
+scoped_ptr<net::HostResolver>
+HostResolverImplChromeOS::CreateHostResolverForTest(
+    scoped_refptr<base::MessageLoopProxy> network_handler_message_loop,
+    NetworkStateHandler* network_state_handler) {
+  Options options;
+  scoped_ptr<net::HostCache> cache = net::HostCache::CreateDefaultCache();
+  return scoped_ptr<net::HostResolver>(new HostResolverImplChromeOS(
+      network_handler_message_loop,
+      network_state_handler,
+      cache.Pass(),
+      options.GetDispatcherLimits(),
+      ProcTaskParams(NULL, options.max_retry_attempts),
+      NULL));
+}
+
+HostResolverImplChromeOS::HostResolverImplChromeOS(
+    scoped_refptr<base::MessageLoopProxy> network_handler_message_loop,
+    NetworkStateHandler* network_state_handler,
+    scoped_ptr<net::HostCache> cache,
+    const net::PrioritizedDispatcher::Limits& job_limits,
+    const ProcTaskParams& proc_params,
+    net::NetLog* net_log)
+    : HostResolverImpl(cache.Pass(), job_limits, proc_params, net_log),
+      network_handler_message_loop_(network_handler_message_loop),
+      weak_ptr_factory_ui_thread_(this) {
+  network_handler_message_loop->PostTask(
+      FROM_HERE,
+      base::Bind(&HostResolverImplChromeOS::CreateNetworkObserver,
+                 weak_ptr_factory_ui_thread_.GetWeakPtr(),
+                 base::MessageLoopProxy::current(),
+                 network_state_handler));
+}
+
+HostResolverImplChromeOS::~HostResolverImplChromeOS() {
+  NetworkObserver* network_observer = network_observer_.release();
+  if (!network_handler_message_loop_->DeleteSoon(FROM_HERE, network_observer))
+    delete network_observer;

[eroman, 2014/06/09 22:37:11]

I don't think this is correct. Deleting network_observer will cause
NetworkStateHandler::RemoveObserver() to be called, which calls into a
non-thread-safe observer list, from potentially the wrong thread.

Not sure how to best handle this. A destruction observer on
network_handler_message_loop_ could work (although gross). Or maybe add an
explicit shutdown method from which it can de-register.

What is the network_handler_message_loop anyway? Is it the UI thread?

[stevenjb, 2014/06/10 22:04:55]

This will only occur if the message loops were no longer running, in which case
it should be safe to remove the observer across thread boundaries, but I agree
it's not a great solution.

I considered a message loop destruction observer, but I also thought it seemed
pretty ugly and awkward.

I guess the best thing to do is to add an explicit Shutdown() method for this
class. I can add it to the interface and have the default implementation do
nothing (other wise we would have to do an ugly static cast).

Yes, network_handler_message_loop is the UI thread, but I didn't think there was
any reason this class should need to know that.

[eroman, 2014/06/12 04:13:37]

This is still a problem, let me lay out my concerns in some more detail:

(A) Since the network handler message loop is the UI message loop, this "delete"
statement is not reachable. The only time DeleteSoon() returns false is if the
underlying message loop was already deleted. And the UI message loop will always
outlive the IO thread so this will not happen.

(B) Even if it were possible for DeleteSoon() to return false, it is still
incorrect to delete the observer here, since it causes a non-threadsafe observer
list (owned by NetworkHandler) to be entered. Imagine if other code was doing
the same from another thread and deciding it is safe to remove observers because
the message loop that networkhandler runs on is gone -- there would now be a
race inside that observerlist. Relying on no other code doing this is not safe.
(That said, this is entirely theoretical since we know this can't happen).

(C) The leak that happens during shutdown should be easy to reproduce. Here is
how it would happen (I have had to debug similar things in the past so I can
tell from experience why this is an anti-pattern):

  (1) The UI's run loop finishes running, starting shutdown
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

  (2) The IO thread is stopped.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

  (3) As part of the IO thread's destruction, your new destructor
~HostResolverImplChromeOS() is invoked.

  (4) The IO thread posts a Delete task to the IO thread
   ====> THIS TASK IS NOT GOING TO BE RUN
   The UI message loop already stopped running in step (1). Any new tasks enqued
to it will simply be deleted by step (5). Moreover, the way that DeleteSoon() is
implemented, the object is only deleted if the task is run, and not when
destroying the callback.
(https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...)

  (5) Back on the UI thread, once the IO thread has finished running the main
message loop is deleted. Deleting the message loop does not run the tasks, it
simply deletes them
(https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...).
Deleting the DeleteSoon task causes the object to be leaked.

============================

OK, so if we agree on the problem, then there are a couple of solutions I can
suggest:

  (A) Make NetworkStateHandler use a ObserverListThreadSafe rather than an
ObserverList. You could now register/unregister from the calling thread and not
have to bounce things around with the ::NetworkObserver subclass.

  (B) Make your own version of DeleteSoon() which actually owns the object, for
instance:
         PostTask(Bind(&NoOpFunction, Owned(network_obsever));
  The advantage of this incantation is that network_observer is now deleted when
the underlying Callback is deleted, so even if it isn't run it will still delete
network_observer. The iffy part about this solution, is if the PostTask were to
return false for some reason, the Callback will be deleted from the wrong thread
(the current one).

Unfortunately these kinds of dependencies between IO and UI thread are pretty
gnarly when considering shutdown. Don't mean to be annoying on this issue, it is
just that I have personally debugged some subtle bugs that can result from it,
and wouldn't wish it on anyone else :)

+}
+
+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,
+                                      net::RequestPriority priority,
+                                      net::AddressList* addresses,
+                                      const net::CompletionCallback& callback,
+                                      RequestHandle* out_req,
+                                      const net::BoundNetLog& source_net_log) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (ResolveLocalIPAddress(info, addresses))
+    return net::OK;
+  return net::HostResolverImpl::Resolve(
+      info, priority, addresses, callback, out_req, source_net_log);
+}
+
+bool HostResolverImplChromeOS::ResolveLocalIPAddress(
+    const RequestInfo& info,
+    net::AddressList* addresses) {
+  if (info.hostname() != net::GetHostName() || !network_observer_)
+    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()) {
+    net::IPAddressNumber ipv6;
+    if (net::ParseIPLiteralToNumber(ipv6_address, &ipv6))
+      addresses->push_back(net::IPEndPoint(ipv6, 0));
+  }
+  std::string ipv4_address = network_observer_->ipv4_address();
+  if (!ipv4_address.empty()) {
+    net::IPAddressNumber ipv4;
+    if (net::ParseIPLiteralToNumber(ipv4_address, &ipv4))
+      addresses->push_back(net::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 chromeos