Fix potential crashes in NetworkHandler code

chromeos/network/network_state_handler.cc

 // Copyright (c) 2012 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/network_state_handler.h"
 
 #include <stddef.h>
 
 #include "base/bind.h"
 #include "base/format_macros.h"
@@ skipping 49 matching lines @@
   base::JSONWriter::WriteWithOptions(
       value, base::JSONWriter::OPTIONS_OMIT_BINARY_VALUES, &vstr);
   return vstr.empty() ? "''" : vstr;
 }
 
 }  // namespace
 
 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.

+  Shutdown();
+  STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
+  STLDeleteContainerPointers(device_list_.begin(), device_list_.end());
 }
 
-NetworkStateHandler::~NetworkStateHandler() {
+void NetworkStateHandler::Shutdown() {
+  if (did_shutdown_)
+    return;
+  did_shutdown_ = true;
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_, IsShuttingDown());
-  STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
-  STLDeleteContainerPointers(device_list_.begin(), device_list_.end());
 }
 
 void NetworkStateHandler::InitShillPropertyHandler() {
   shill_property_handler_.reset(new internal::ShillPropertyHandler(this));
   shill_property_handler_->Init();
 }
 
 // static
 NetworkStateHandler* NetworkStateHandler::InitializeForTest() {
   NetworkStateHandler* handler = new NetworkStateHandler();
   handler->InitShillPropertyHandler();
   return handler;
 }
 
 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(
     const NetworkTypePattern& type) const {
   std::string technology = GetTechnologyForType(type);
   TechnologyState state;
   if (shill_property_handler_->IsTechnologyEnabled(technology))
     state = TECHNOLOGY_ENABLED;
   else if (shill_property_handler_->IsTechnologyEnabling(technology))
     state = TECHNOLOGY_ENABLING;
@@ skipping 877 matching lines @@
   if (type.MatchesType(shill::kTypeBluetooth))
     technologies.push_back(new std::string(shill::kTypeBluetooth));
   if (type.MatchesType(shill::kTypeVPN))
     technologies.push_back(new std::string(shill::kTypeVPN));
 
   CHECK_GT(technologies.size(), 0ul);
   return technologies;
 }
 
 }  // namespace chromeos