Fix potential crashes in NetworkHandler code

chromeos/network/network_state_handler.cc

Index: chromeos/network/network_state_handler.cc
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index 5f4738a6cdfa0335e61aa025d72dfdf589114e36..7396d45b6783c141286fdf527558c567d9df6469 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -67,15 +67,21 @@ std::string ValueAsString(const base::Value& value) {
 const char NetworkStateHandler::kDefaultCheckPortalList[] =
     "ethernet,wifi,cellular";
 
-NetworkStateHandler::NetworkStateHandler() : network_list_sorted_(false) {
-}
+NetworkStateHandler::NetworkStateHandler() {}
 
 NetworkStateHandler::~NetworkStateHandler() {

[oshima, 2016/01/06 22:57:23]

I believe Shutodnw() must have been called at this point? Maybe

DCHECK(did_shutdown_) ?

[stevenjb, 2016/01/06 23:28:31]

So, -usually- Shutdown will get called. However, in unit tests we do not use
NetworkHandler, instead we instantiate individual handlers as needed, which is
why I added the extra Shutdown() call. I can add the test in Shutdown() here
instead and add a comment.

[oshima, 2016/01/07 00:21:12]

Yeah, I prefer to have the same code path on test and real code path.

[stevenjb, 2016/01/07 17:07:47]

So do I.

Unfortunately sometimes that conflicts with the principal of minimizing code in
unit tests. Because of the complexity of the NetworkHandler class we decided to
allow unit tests to instantiate individual networking classes (e.g. just
NetworkStateHandler).

If we call Shutdown() in the destructor the global accessor no longer exists
(the scoped pointer that owns the class is null) so classes that use
NetworkHandler::Get()->network_state_handler() will be unable to access
NetworkStateHandler to remove their observer (e.g.
ProxyConfigServiceImpl::OnShuttingDown). 

However in a unit test that just instantiates a NetworkStateHandler instance,
there is no NetworkHandler so we have to call Shutdown() from here for classes
that rely on OnShuttingDown for their cleanup, e.g. HostResolverImpllChromeos).

[oshima, 2016/01/07 18:14:17]

There are multiple principals (minimizing divergence between test and real code)
and we just need to keep good balance of them, and I think testing what's
actually happening in production is more important, especially this CL is to fix
the shutdown sequence.
I fully understand why it will not necessary for certain unit tests. It's just
my recommendation which may help to catch future regression, as memory problems
aren't always be catch-able in CQ bots. I won't block this since you own this.
lgtm.

-  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_, IsShuttingDown());
+  Shutdown();
   STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
   STLDeleteContainerPointers(device_list_.begin(), device_list_.end());
 }
 
+void NetworkStateHandler::Shutdown() {
+  if (did_shutdown_)
+    return;
+  did_shutdown_ = true;
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_, IsShuttingDown());
+}
+
 void NetworkStateHandler::InitShillPropertyHandler() {
   shill_property_handler_.reset(new internal::ShillPropertyHandler(this));
   shill_property_handler_->Init();
@@ -92,20 +98,27 @@ void NetworkStateHandler::AddObserver(
     NetworkStateHandlerObserver* observer,
     const tracked_objects::Location& from_here) {
   observers_.AddObserver(observer);
-  device_event_log::AddEntry(from_here.file_name(), from_here.line_number(),
-                             device_event_log::LOG_TYPE_NETWORK,
-                             device_event_log::LOG_LEVEL_DEBUG,
-                             "NetworkStateHandler::AddObserver");
+  device_event_log::AddEntry(
+      from_here.file_name(), from_here.line_number(),
+      device_event_log::LOG_TYPE_NETWORK, device_event_log::LOG_LEVEL_DEBUG,
+      base::StringPrintf("NetworkStateHandler::AddObserver: 0x%p", observer));
+
+  LOG(ERROR) << "ADD Observer: " << from_here.file_name() << ":"
+             << from_here.line_number() << ": " << observer;
 }
 
 void NetworkStateHandler::RemoveObserver(
     NetworkStateHandlerObserver* observer,
     const tracked_objects::Location& from_here) {
   observers_.RemoveObserver(observer);
-  device_event_log::AddEntry(from_here.file_name(), from_here.line_number(),
-                             device_event_log::LOG_TYPE_NETWORK,
-                             device_event_log::LOG_LEVEL_DEBUG,
-                             "NetworkStateHandler::RemoveObserver");
+  device_event_log::AddEntry(
+      from_here.file_name(), from_here.line_number(),
+      device_event_log::LOG_TYPE_NETWORK, device_event_log::LOG_LEVEL_DEBUG,
+      base::StringPrintf("NetworkStateHandler::RemoveObserver: 0x%p",
+                         observer));
+
+  LOG(ERROR) << "REM Observer: " << from_here.file_name() << ":"
+             << from_here.line_number() << ": " << observer;
 }
 
 NetworkStateHandler::TechnologyState NetworkStateHandler::GetTechnologyState(