Fix initial offline state response handling

net/base/network_change_notifier_linux.cc

 // Copyright (c) 2011 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.
 //
 // This implementation of NetworkChangeNotifier's offline state detection
 // depends on D-Bus and NetworkManager, and is known to work on at least
 // GNOME version 2.30.  If D-Bus or NetworkManager are unavailable, this
 // implementation will always behave as if it is online.
 
 #include "net/base/network_change_notifier_linux.h"
@@ skipping 70 matching lines @@
 
   // Must be called by the helper thread's CleanUp() method.
   void CleanUp();
 
   // Implementation of NetworkChangeNotifierLinux::IsCurrentlyOffline().
   // Safe to call from any thread, but will block until Init() has completed.
   bool IsCurrentlyOffline();
 
  private:
   // Callbacks for D-Bus API.
-  void OnStateChanged(dbus::Message* message);
-
-  void OnResponse(dbus::Response* response) {
-    OnStateChanged(response);
-    offline_state_initialized_.Signal();
-  }
-
-  void OnSignaled(dbus::Signal* signal) {
-    OnStateChanged(signal);
-  }
+  void OnResponse(dbus::Response* response);
+  void OnSignaled(dbus::Signal* signal);
 
   void OnConnected(const std::string&, const std::string&, bool success) {
     if (!success) {
       DLOG(WARNING) << "Failed to set up offline state detection";
       offline_state_initialized_.Signal();
     }
   }
 
   // Converts a NetworkManager state uint to a bool.
   static bool StateIsOffline(uint32 state);
@@ skipping 44 matching lines @@
       "StateChanged",
       base::Bind(&NetworkManagerApi::OnSignaled, ptr_factory_.GetWeakPtr()),
       base::Bind(&NetworkManagerApi::OnConnected, ptr_factory_.GetWeakPtr()));
 }
 
 void NetworkManagerApi::CleanUp() {
   DCHECK_EQ(helper_thread_id_, base::PlatformThread::CurrentId());
   ptr_factory_.InvalidateWeakPtrs();
 }
 
-void NetworkManagerApi::OnStateChanged(dbus::Message* message) {
+void NetworkManagerApi::OnResponse(dbus::Response* response) {
   DCHECK_EQ(helper_thread_id_, base::PlatformThread::CurrentId());
-  if (!message) {
+  if (!response) {
     DLOG(WARNING) << "No response received for initial state request";
+    offline_state_initialized_.Signal();
     return;
   }
-  dbus::MessageReader reader(message);
+  dbus::MessageReader reader(response);
+  uint32 state = 0;
+  if (!reader.HasMoreData() || !reader.PopVariantOfUint32(&state)) {

[satorux1, 2011/11/17 17:40:15]

You don't need !reader.HasMoreData(). PopVariantOfUint32() will return false, if
the content is empty.

[adamk, 2011/11/17 18:58:24]

Good to know, done.

+    DLOG(WARNING) << "Unexpected response for NetworkManager State request: "
+                  << response->ToString();
+    offline_state_initialized_.Signal();
+    return;
+  }
+  {
+    base::AutoLock lock(is_offline_lock_);
+    is_offline_ = StateIsOffline(state);
+  }
+  offline_state_initialized_.Signal();

[satorux1, 2011/11/17 17:40:15]

Calling offline_state_initialized_.Signal() from three places looks a bit
error-prone. You might want to introduce OnResponseInternal() to do most of the
stuff, and then change the function to be

  OnResponseInternal(response);
  offline_state_initialized_.Signal();

[adamk, 2011/11/17 18:58:24]

Yeah, I agree; this is covered by the unittests, but if someone were to add
another exit point it'd be bad.  I've renamed the callback to OnInitialResponse
and the parser to HandleResponse.

+}
+
+void NetworkManagerApi::OnSignaled(dbus::Signal* signal) {
+  DCHECK_EQ(helper_thread_id_, base::PlatformThread::CurrentId());
+  dbus::MessageReader reader(signal);
   uint32 state = 0;
   if (!reader.HasMoreData() || !reader.PopUint32(&state)) {

[satorux1, 2011/11/17 17:40:15]

You can remove !reader.HasMoreData() from here too.

[adamk, 2011/11/17 18:58:24]

Done.

-    DLOG(WARNING) << "Unexpected response for NetworkManager State request: "
-                  << message->ToString();
+    DLOG(WARNING) << "Unexpected signal for NetworkManager StateChanged: "
+                  << signal->ToString();
     return;
   }
   bool new_is_offline = StateIsOffline(state);
   {
     base::AutoLock lock(is_offline_lock_);
     if (is_offline_ != new_is_offline)
       is_offline_ = new_is_offline;
     else
       return;
   }
-  if (offline_state_initialized_.IsSignaled())
-    notification_callback_.Run();
+  notification_callback_.Run();
 }
 
 bool NetworkManagerApi::StateIsOffline(uint32 state) {
   switch (state) {
     case NM_LEGACY_STATE_CONNECTED:
     case NM_STATE_CONNECTED_SITE:
     case NM_STATE_CONNECTED_GLOBAL:
       // Definitely connected
       return false;
     case NM_LEGACY_STATE_DISCONNECTED:
@@ skipping 245 matching lines @@
   // Stopping from here allows us to sanity- check that the notifier
   // thread shut down properly.
   notifier_thread_->Stop();
 }
 
 bool NetworkChangeNotifierLinux::IsCurrentlyOffline() const {
   return notifier_thread_->IsCurrentlyOffline();
 }
 
 }  // namespace net