Add dbus::ObjectProxy::CallMethodWithErrorCallback

Add CallMethodWithErrorCallback
Add EndToEndAsyncTest.BrokenBus
Fix leak in StartAsyncMethodCall

BUG=chromium-os:27899
TEST=dbus_unittests --gtest_filter="EndToEndAsyncTest.*"

Review URL: https://chromiumcodereview.appspot.com/10121005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133140 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 227 insertions, 11 deletions

diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 9be24e0..a4438e4 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -118,6 +118,28 @@ class EndToEndAsyncTest : public testing::Test {
   }
 
  protected:
+  // Replaces the bus with a broken one.
+  void SetUpBrokenBus() {
+    // Shut down the existing bus.
+    bus_->ShutdownOnDBusThreadAndBlock();
+
+    // Create new bus with invalid address.
+    const char kInvalidAddress[] = "";
+    dbus::Bus::Options bus_options;
+    bus_options.bus_type = dbus::Bus::CUSTOM_ADDRESS;
+    bus_options.address = kInvalidAddress;
+    bus_options.connection_type = dbus::Bus::PRIVATE;
+    bus_options.dbus_thread_message_loop_proxy =
+        dbus_thread_->message_loop_proxy();
+    bus_ = new dbus::Bus(bus_options);
+    ASSERT_TRUE(bus_->HasDBusThread());
+
+    // Create new object proxy.
+    object_proxy_ = bus_->GetObjectProxy(
+        "org.chromium.TestService",
+        dbus::ObjectPath("/org/chromium/TestObject"));
+  }
+
   // Calls the method asynchronously. OnResponse() will be called once the
   // response is received.
   void CallMethod(dbus::MethodCall* method_call,
@@ -128,6 +150,17 @@ class EndToEndAsyncTest : public testing::Test {
                                          base::Unretained(this)));
   }
 
+  // Calls the method asynchronously. OnResponse() will be called once the
+  // response is received without error, otherwise OnError() will be called.
+  void CallMethodWithErrorCallback(dbus::MethodCall* method_call,
+                                   int timeout_ms) {
+    object_proxy_->CallMethodWithErrorCallback(
+        method_call,
+        timeout_ms,
+        base::Bind(&EndToEndAsyncTest::OnResponse, base::Unretained(this)),
+        base::Bind(&EndToEndAsyncTest::OnError, base::Unretained(this)));
+  }
+
   // Wait for the give number of responses.
   void WaitForResponses(size_t num_responses) {
     while (response_strings_.size() < num_responses) {
@@ -150,6 +183,26 @@ class EndToEndAsyncTest : public testing::Test {
     message_loop_.Quit();
   };
 
+  // Wait for the given number of errors.
+  void WaitForErrors(size_t num_errors) {
+    while (error_names_.size() < num_errors) {
+      message_loop_.Run();
+    }
+  }
+
+  // Called when an error is received.
+  void OnError(dbus::ErrorResponse* error) {
+    // |error| will be deleted on exit of the function. Copy the payload to
+    // |error_names_|.
+    if (error) {
+      ASSERT_NE("", error->GetErrorName());
+      error_names_.push_back(error->GetErrorName());
+    } else {
+      error_names_.push_back("");
+    }
+    message_loop_.Quit();
+  }
+
   // Called when the "Test" signal is received, in the main thread.
   // Copy the string payload to |test_signal_string_|.
   void OnTestSignal(dbus::Signal* signal) {
@@ -189,6 +242,7 @@ class EndToEndAsyncTest : public testing::Test {
 
   MessageLoop message_loop_;
   std::vector<std::string> response_strings_;
+  std::vector<std::string> error_names_;
   scoped_ptr<base::Thread> dbus_thread_;
   scoped_refptr<dbus::Bus> bus_;
   dbus::ObjectProxy* object_proxy_;
@@ -217,6 +271,24 @@ TEST_F(EndToEndAsyncTest, Echo) {
   EXPECT_EQ(kHello, response_strings_[0]);
 }
 
+TEST_F(EndToEndAsyncTest, EchoWithErrorCallback) {
+  const char* kHello = "hello";
+
+  // Create the method call.
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(kHello);
+
+  // Call the method.
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+
+  // Check the response.
+  WaitForResponses(1);
+  EXPECT_EQ(kHello, response_strings_[0]);
+  EXPECT_TRUE(error_names_.empty());
+}
+
 // Call Echo method three times.
 TEST_F(EndToEndAsyncTest, EchoThreeTimes) {
   const char* kMessages[] = { "foo", "bar", "baz" };
@@ -241,6 +313,47 @@ TEST_F(EndToEndAsyncTest, EchoThreeTimes) {
   EXPECT_EQ("foo", response_strings_[2]);
 }
 
+TEST_F(EndToEndAsyncTest, BrokenBus) {
+  const char* kHello = "hello";
+
+  // Set up a broken bus.
+  SetUpBrokenBus();
+
+  // Create the method call.
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(kHello);
+
+  // Call the method.
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethod(&method_call, timeout_ms);
+  WaitForResponses(1);
+
+  // Should fail because of the broken bus.
+  ASSERT_EQ("", response_strings_[0]);
+}
+
+TEST_F(EndToEndAsyncTest, BrokenBusWithErrorCallback) {
+  const char* kHello = "hello";
+
+  // Set up a broken bus.
+  SetUpBrokenBus();
+
+  // Create the method call.
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(kHello);
+
+  // Call the method.
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because of the broken bus.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ("", error_names_[0]);
+}
+
 TEST_F(EndToEndAsyncTest, Timeout) {
   const char* kHello = "hello";
 
@@ -258,6 +371,24 @@ TEST_F(EndToEndAsyncTest, Timeout) {
   ASSERT_EQ("", response_strings_[0]);
 }
 
+TEST_F(EndToEndAsyncTest, TimeoutWithErrorCallback) {
+  const char* kHello = "hello";
+
+  // Create the method call.
+  dbus::MethodCall method_call("org.chromium.TestInterface", "SlowEcho");
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(kHello);
+
+  // Call the method with timeout of 0ms.
+  const int timeout_ms = 0;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because of timeout.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ(DBUS_ERROR_NO_REPLY, error_names_[0]);
+}
+
 // Tests calling a method that sends its reply asynchronously.
 TEST_F(EndToEndAsyncTest, AsyncEcho) {
   const char* kHello = "hello";
@@ -287,6 +418,18 @@ TEST_F(EndToEndAsyncTest, NonexistentMethod) {
   ASSERT_EQ("", response_strings_[0]);
 }
 
+TEST_F(EndToEndAsyncTest, NonexistentMethodWithErrorCallback) {
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Nonexistent");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because the method is nonexistent.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ(DBUS_ERROR_UNKNOWN_METHOD, error_names_[0]);
+}
+
 TEST_F(EndToEndAsyncTest, BrokenMethod) {
   dbus::MethodCall method_call("org.chromium.TestInterface", "BrokenMethod");
 
@@ -298,6 +441,18 @@ TEST_F(EndToEndAsyncTest, BrokenMethod) {
   ASSERT_EQ("", response_strings_[0]);
 }
 
+TEST_F(EndToEndAsyncTest, BrokenMethodWithErrorCallback) {
+  dbus::MethodCall method_call("org.chromium.TestInterface", "BrokenMethod");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because the method is broken.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ(DBUS_ERROR_FAILED, error_names_[0]);
+}
+
 TEST_F(EndToEndAsyncTest, EmptyResponseCallback) {
   const char* kHello = "hello";
 
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index 0cbd93e..051d8f1 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -96,6 +96,16 @@ Response* ObjectProxy::CallMethodAndBlock(MethodCall* method_call,
 void ObjectProxy::CallMethod(MethodCall* method_call,
                              int timeout_ms,
                              ResponseCallback callback) {
+  CallMethodWithErrorCallback(method_call, timeout_ms, callback,
+                              base::Bind(&ObjectProxy::OnCallMethodError,
+                                         this,
+                                         callback));
+}
+
+void ObjectProxy::CallMethodWithErrorCallback(MethodCall* method_call,
+                                              int timeout_ms,
+                                              ResponseCallback callback,
+                                              ErrorCallback error_callback) {
   bus_->AssertOnOriginThread();
 
   method_call->SetDestination(service_name_);
@@ -112,6 +122,7 @@ void ObjectProxy::CallMethod(MethodCall* method_call,
                                   timeout_ms,
                                   request_message,
                                   callback,
+                                  error_callback,
                                   start_time);
   // Wait for the response in the D-Bus thread.
   bus_->PostTaskToDBusThread(FROM_HERE, task);
@@ -161,9 +172,11 @@ ObjectProxy::ResponseCallback ObjectProxy::EmptyResponseCallback() {
 ObjectProxy::OnPendingCallIsCompleteData::OnPendingCallIsCompleteData(
     ObjectProxy* in_object_proxy,
     ResponseCallback in_response_callback,
+    ErrorCallback in_error_callback,
     base::TimeTicks in_start_time)
     : object_proxy(in_object_proxy),
       response_callback(in_response_callback),
+      error_callback(in_error_callback),
       start_time(in_start_time) {
 }
 
@@ -173,19 +186,22 @@ ObjectProxy::OnPendingCallIsCompleteData::~OnPendingCallIsCompleteData() {
 void ObjectProxy::StartAsyncMethodCall(int timeout_ms,
                                        DBusMessage* request_message,
                                        ResponseCallback response_callback,
+                                       ErrorCallback error_callback,
                                        base::TimeTicks start_time) {
   bus_->AssertOnDBusThread();
 
   if (!bus_->Connect() || !bus_->SetUpAsyncOperations()) {
-    // In case of a failure, run the callback with NULL response, that
-    // indicates a failure.
+    // In case of a failure, run the error callback with NULL.
     DBusMessage* response_message = NULL;
     base::Closure task = base::Bind(&ObjectProxy::RunResponseCallback,
                                     this,
                                     response_callback,
+                                    error_callback,
                                     start_time,
                                     response_message);
     bus_->PostTaskToOriginThread(FROM_HERE, task);
+
+    dbus_message_unref(request_message);
     return;
   }
 
@@ -196,7 +212,8 @@ void ObjectProxy::StartAsyncMethodCall(int timeout_ms,
   // Prepare the data we'll be passing to OnPendingCallIsCompleteThunk().
   // The data will be deleted in OnPendingCallIsCompleteThunk().
   OnPendingCallIsCompleteData* data =
-      new OnPendingCallIsCompleteData(this, response_callback, start_time);
+      new OnPendingCallIsCompleteData(this, response_callback, error_callback,
+                                      start_time);
 
   // This returns false only when unable to allocate memory.
   const bool success = dbus_pending_call_set_notify(
@@ -213,6 +230,7 @@ void ObjectProxy::StartAsyncMethodCall(int timeout_ms,
 
 void ObjectProxy::OnPendingCallIsComplete(DBusPendingCall* pending_call,
                                           ResponseCallback response_callback,
+                                          ErrorCallback error_callback,
                                           base::TimeTicks start_time) {
   bus_->AssertOnDBusThread();
 
@@ -220,12 +238,14 @@ void ObjectProxy::OnPendingCallIsComplete(DBusPendingCall* pending_call,
   base::Closure task = base::Bind(&ObjectProxy::RunResponseCallback,
                                   this,
                                   response_callback,
+                                  error_callback,
                                   start_time,
                                   response_message);
   bus_->PostTaskToOriginThread(FROM_HERE, task);
 }
 
 void ObjectProxy::RunResponseCallback(ResponseCallback response_callback,
+                                      ErrorCallback error_callback,
                                       base::TimeTicks start_time,
                                       DBusMessage* response_message) {
   bus_->AssertOnOriginThread();
@@ -233,19 +253,13 @@ void ObjectProxy::RunResponseCallback(ResponseCallback response_callback,
   bool method_call_successful = false;
   if (!response_message) {
     // The response is not received.
-    response_callback.Run(NULL);
+    error_callback.Run(NULL);
   } else if (dbus_message_get_type(response_message) ==
              DBUS_MESSAGE_TYPE_ERROR) {
     // This will take |response_message| and release (unref) it.
     scoped_ptr<dbus::ErrorResponse> error_response(
         dbus::ErrorResponse::FromRawMessage(response_message));
-    // Error message may contain the error message as string.
-    dbus::MessageReader reader(error_response.get());
-    std::string error_message;
-    reader.PopString(&error_message);
-    LogMethodCallFailure(error_response->GetErrorName(), error_message);
-    // We don't give the error message to the callback.
-    response_callback.Run(NULL);
+    error_callback.Run(error_response.get());
   } else {
     // This will take |response_message| and release (unref) it.
     scoped_ptr<dbus::Response> response(
@@ -270,6 +284,7 @@ void ObjectProxy::OnPendingCallIsCompleteThunk(DBusPendingCall* pending_call,
   ObjectProxy* self = data->object_proxy;
   self->OnPendingCallIsComplete(pending_call,
                                 data->response_callback,
+                                data->error_callback,
                                 data->start_time);
   delete data;
 }
@@ -429,4 +444,16 @@ void ObjectProxy::LogMethodCallFailure(
              << ": " << error_message;
 }
 
+void ObjectProxy::OnCallMethodError(ResponseCallback response_callback,
+                                    ErrorResponse* error_response) {
+  if (error_response) {
+    // Error message may contain the error message as string.
+    dbus::MessageReader reader(error_response);
+    std::string error_message;
+    reader.PopString(&error_message);
+    LogMethodCallFailure(error_response->GetErrorName(), error_message);
+  }
+  response_callback.Run(NULL);
+}
+
 }  // namespace dbus
diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h
index eb09b8c0..620622f 100644
--- a/dbus/object_proxy.h
+++ b/dbus/object_proxy.h
@@ -21,6 +21,7 @@
 namespace dbus {
 
 class Bus;
+class ErrorResponse;
 class MethodCall;
 class Response;
 class Signal;
@@ -59,6 +60,10 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
     TIMEOUT_INFINITE = 0x7fffffff,
   };
 
+  // Called when an error response is returned or no response is returned.
+  // Used for CallMethodWithErrorCallback().
+  typedef base::Callback<void(ErrorResponse*)> ErrorCallback;
+
   // Called when the response is returned. Used for CallMethod().
   typedef base::Callback<void(Response*)> ResponseCallback;
 
@@ -99,6 +104,26 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
                           int timeout_ms,
                           ResponseCallback callback);
 
+  // Requests to call the method of the remote object.
+  //
+  // |callback| and |error_callback| will be called in the origin thread, once
+  // the method call is complete. As it's called in the origin thread,
+  // |callback| can safely reference objects in the origin thread (i.e.
+  // UI thread in most cases). If the caller is not interested in the response
+  // from the method (i.e. calling a method that does not return a value),
+  // EmptyResponseCallback() can be passed to the |callback| parameter.
+  //
+  // If the method call is successful, a pointer to Response object will
+  // be passed to the callback. If unsuccessful, the error callback will be
+  // called and a pointer to ErrorResponse object will be passed to the error
+  // callback if available, otherwise NULL will be passed.
+  //
+  // Must be called in the origin thread.
+  virtual void CallMethodWithErrorCallback(MethodCall* method_call,
+                                           int timeout_ms,
+                                           ResponseCallback callback,
+                                           ErrorCallback error_callback);
+
   // Requests to connect to the signal from the remote object, replacing
   // any previous |signal_callback| connected to that signal.
   //
@@ -138,11 +163,13 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
   struct OnPendingCallIsCompleteData {
     OnPendingCallIsCompleteData(ObjectProxy* in_object_proxy,
                                 ResponseCallback in_response_callback,
+                                ErrorCallback error_callback,
                                 base::TimeTicks start_time);
     ~OnPendingCallIsCompleteData();
 
     ObjectProxy* object_proxy;
     ResponseCallback response_callback;
+    ErrorCallback error_callback;
     base::TimeTicks start_time;
   };
 
@@ -151,15 +178,18 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
   void StartAsyncMethodCall(int timeout_ms,
                             DBusMessage* request_message,
                             ResponseCallback response_callback,
+                            ErrorCallback error_callback,
                             base::TimeTicks start_time);
 
   // Called when the pending call is complete.
   void OnPendingCallIsComplete(DBusPendingCall* pending_call,
                                ResponseCallback response_callback,
+                               ErrorCallback error_callback,
                                base::TimeTicks start_time);
 
   // Runs the response callback with the given response object.
   void RunResponseCallback(ResponseCallback response_callback,
+                           ErrorCallback error_callback,
                            base::TimeTicks start_time,
                            DBusMessage* response_message);
 
@@ -199,6 +229,10 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
   void LogMethodCallFailure(const base::StringPiece& error_name,
                             const base::StringPiece& error_message) const;
 
+  // Used as ErrorCallback by CallMethod().
+  void OnCallMethodError(ResponseCallback response_callback,
+                         ErrorResponse* error_response);
+
   scoped_refptr<Bus> bus_;
   std::string service_name_;
   ObjectPath object_path_;