Cleanup: Pass std::string as const reference from chromeos/

chromeos/network/network_connection_handler.cc

 // Copyright (c) 2013 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_connection_handler.h"
 
 #include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 638 matching lines @@
     error = kErrorConnecting;
   } else {
     NET_LOG_ERROR("Connect Failure, Shill error: " + dbus_error_name,
                   service_path);
     error = kErrorConnectFailed;
   }
   InvokeConnectErrorCallback(service_path, error_callback, error);
 }
 
 void NetworkConnectionHandler::CheckPendingRequest(
-    const std::string service_path) {
+    const std::string& service_path) {
   ConnectRequest* request = GetPendingRequest(service_path);
   DCHECK(request);
   if (request->connect_state == ConnectRequest::CONNECT_REQUESTED)
     return;  // Request has not started, ignore update
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
   if (!network)
     return;  // NetworkState may not be be updated yet.
 
   if (network->IsConnectingState()) {
@@ skipping 30 matching lines @@
   } else {
     error_name = kErrorConnectFailed;
     if (network->connection_state() != shill::kStateFailure) {
       NET_LOG_ERROR("Unexpected State: " + network->connection_state(),
                     service_path);
     }
   }
 
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   InvokeConnectErrorCallback(service_path, error_callback, error_name);

[stevenjb, 2015/09/21 22:23:39]

We *REALLY* need to be careful about this sort of change. PLEASE audit any non
trivial cases more carefully. The minor optimizations are not (IMHO) worth
introducing crash bugs.

This line will crash if we do not copy service_path, that is why it was written
this way in the first place. CheckPendingRequest() may get called with
pending_request_[]->service_path, so erasing the request in the line above can
make |service_path| invalid if it is a reference.

In fairness a comment in the header is in order here, so please do add a comment
for this methods, thanks!

[ki.stfu, 2015/09/21 22:55:57]

Agreed. Done.

 }
 
 void NetworkConnectionHandler::CheckAllPendingRequests() {
   for (std::map<std::string, ConnectRequest>::iterator iter =
            pending_requests_.begin(); iter != pending_requests_.end(); ++iter) {
     CheckPendingRequest(iter->first);
   }
 }
 
 // Connect callbacks
@@ skipping 51 matching lines @@
 
 void NetworkConnectionHandler::HandleShillDisconnectSuccess(
     const std::string& service_path,
     const base::Closure& success_callback) {
   NET_LOG_EVENT("Disconnect Request Sent", service_path);
   if (!success_callback.is_null())
     success_callback.Run();
 }
 
 }  // namespace chromeos