Change setters of dbus::Message to return false instead of aborting on errors

With this change, we can safely return error for invalid object path and service name.
It still crashes on invalid method name and interface name if we use MethodCall::MethodCall for setting those parameters.

BUG=128967
TEST=dbus_unittests

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

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

dbus/end_to_end_async_unittest.cc

@@ -453,6 +453,44 @@ TEST_F(EndToEndAsyncTest, BrokenMethodWithErrorCallback) {
   ASSERT_EQ(DBUS_ERROR_FAILED, error_names_[0]);
 }
 
+TEST_F(EndToEndAsyncTest, InvalidObjectPath) {
+  // Trailing '/' is only allowed for the root path.
+  const dbus::ObjectPath invalid_object_path("/org/chromium/TestObject/");
+
+  // Replace object proxy with new one.
+  object_proxy_ = bus_->GetObjectProxy("org.chromium.TestService",
+                                       invalid_object_path);
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because of the invalid path.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ("", error_names_[0]);
+}
+
+TEST_F(EndToEndAsyncTest, InvalidServiceName) {
+  // Bus name cannot contain '/'.
+  const std::string invalid_service_name = ":1/2";
+
+  // Replace object proxy with new one.
+  object_proxy_ = bus_->GetObjectProxy(
+      invalid_service_name, dbus::ObjectPath("org.chromium.TestObject"));
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because of the invalid bus name.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ("", error_names_[0]);
+}
+
 TEST_F(EndToEndAsyncTest, EmptyResponseCallback) {
   const char* kHello = "hello";
 

dbus/end_to_end_sync_unittest.cc

@@ -104,3 +104,35 @@ TEST_F(EndToEndSyncTest, BrokenMethod) {
       object_proxy_->CallMethodAndBlock(&method_call, timeout_ms));
   ASSERT_FALSE(response.get());
 }
+
+TEST_F(EndToEndSyncTest, InvalidObjectPath) {
+  // Trailing '/' is only allowed for the root path.
+  const dbus::ObjectPath invalid_object_path("/org/chromium/TestObject/");
+
+  // Replace object proxy with new one.
+  object_proxy_ = client_bus_->GetObjectProxy("org.chromium.TestService",
+                                              invalid_object_path);
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  scoped_ptr<dbus::Response> response(
+      object_proxy_->CallMethodAndBlock(&method_call, timeout_ms));
+  ASSERT_FALSE(response.get());
+}
+
+TEST_F(EndToEndSyncTest, InvalidServiceName) {
+  // Bus name cannot contain '/'.
+  const std::string invalid_service_name = ":1/2";
+
+  // Replace object proxy with new one.
+  object_proxy_ = client_bus_->GetObjectProxy(
+      invalid_service_name, dbus::ObjectPath("org.chromium.TestObject"));
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  scoped_ptr<dbus::Response> response(
+      object_proxy_->CallMethodAndBlock(&method_call, timeout_ms));
+  ASSERT_FALSE(response.get());
+}

dbus/exported_object.cc

@@ -90,7 +90,7 @@ void ExportedObject::ExportMethod(const std::string& interface_name,
 void ExportedObject::SendSignal(Signal* signal) {
   // For signals, the object path should be set to the path to the sender
   // object, which is this exported object here.
-  signal->SetPath(object_path_);
+  CHECK(signal->SetPath(object_path_));
 
   // Increment the reference count so we can safely reference the
   // underlying signal message until the signal sending is complete. This

dbus/message.cc

@@ -249,40 +249,28 @@ std::string Message::ToString() {
   return headers + "\n" + ToStringInternal("", &reader);
 }
 
-void Message::SetDestination(const std::string& destination) {
-  const bool success = dbus_message_set_destination(raw_message_,
-                                                    destination.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetDestination(const std::string& destination) {
+  return dbus_message_set_destination(raw_message_, destination.c_str());
 }
 
-void Message::SetPath(const ObjectPath& path) {
-  const bool success = dbus_message_set_path(raw_message_,
-                                             path.value().c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetPath(const ObjectPath& path) {
+  return dbus_message_set_path(raw_message_, path.value().c_str());
 }
 
-void Message::SetInterface(const std::string& interface) {
-  const bool success = dbus_message_set_interface(raw_message_,
-                                                  interface.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetInterface(const std::string& interface) {
+  return dbus_message_set_interface(raw_message_, interface.c_str());
 }
 
-void Message::SetMember(const std::string& member) {
-  const bool success = dbus_message_set_member(raw_message_,
-                                               member.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetMember(const std::string& member) {
+  return dbus_message_set_member(raw_message_, member.c_str());
 }
 
-void Message::SetErrorName(const std::string& error_name) {
-  const bool success = dbus_message_set_error_name(raw_message_,
-                                                   error_name.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetErrorName(const std::string& error_name) {
+  return dbus_message_set_error_name(raw_message_, error_name.c_str());
 }
 
-void Message::SetSender(const std::string& sender) {
-  const bool success = dbus_message_set_sender(raw_message_,
-                                               sender.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetSender(const std::string& sender) {
+  return dbus_message_set_sender(raw_message_, sender.c_str());
 }
 
 void Message::SetSerial(uint32 serial) {
@@ -345,8 +333,8 @@ MethodCall::MethodCall(const std::string& interface_name,
     : Message() {
   Init(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
 
-  SetInterface(interface_name);
-  SetMember(method_name);
+  CHECK(SetInterface(interface_name));
+  CHECK(SetMember(method_name));
 }
 
 MethodCall::MethodCall() : Message() {
@@ -368,8 +356,8 @@ Signal::Signal(const std::string& interface_name,
     : Message() {
   Init(dbus_message_new(DBUS_MESSAGE_TYPE_SIGNAL));
 
-  SetInterface(interface_name);
-  SetMember(method_name);
+  CHECK(SetInterface(interface_name));
+  CHECK(SetMember(method_name));
 }
 
 Signal::Signal() : Message() {

dbus/message.h

@@ -90,12 +90,12 @@ class Message {
   DBusMessage* raw_message() { return raw_message_; }
 
   // Sets the destination, the path, the interface, the member, etc.
-  void SetDestination(const std::string& destination);
-  void SetPath(const ObjectPath& path);
-  void SetInterface(const std::string& interface);
-  void SetMember(const std::string& member);
-  void SetErrorName(const std::string& error_name);
-  void SetSender(const std::string& sender);
+  bool SetDestination(const std::string& destination);
+  bool SetPath(const ObjectPath& path);
+  bool SetInterface(const std::string& interface);
+  bool SetMember(const std::string& member);
+  bool SetErrorName(const std::string& error_name);
+  bool SetSender(const std::string& sender);
   void SetSerial(uint32 serial);
   void SetReplySerial(uint32 reply_serial);
   // SetSignature() does not exist as we cannot do it.

dbus/message_unittest.cc

@@ -565,12 +565,12 @@ TEST(MessageTest, GetAndSetHeaders) {
   EXPECT_EQ(0U, message->GetSerial());
   EXPECT_EQ(0U, message->GetReplySerial());
 
-  message->SetDestination("org.chromium.destination");
-  message->SetPath(dbus::ObjectPath("/org/chromium/path"));
-  message->SetInterface("org.chromium.interface");
-  message->SetMember("member");
-  message->SetErrorName("org.chromium.error");
-  message->SetSender(":1.2");
+  EXPECT_TRUE(message->SetDestination("org.chromium.destination"));
+  EXPECT_TRUE(message->SetPath(dbus::ObjectPath("/org/chromium/path")));
+  EXPECT_TRUE(message->SetInterface("org.chromium.interface"));
+  EXPECT_TRUE(message->SetMember("member"));
+  EXPECT_TRUE(message->SetErrorName("org.chromium.error"));
+  EXPECT_TRUE(message->SetSender(":1.2"));
   message->SetSerial(123);
   message->SetReplySerial(456);
 
@@ -583,3 +583,33 @@ TEST(MessageTest, GetAndSetHeaders) {
   EXPECT_EQ(123U, message->GetSerial());
   EXPECT_EQ(456U, message->GetReplySerial());
 }
+
+TEST(MessageTest, SetInvalidHeaders) {
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  EXPECT_EQ("", message->GetDestination());
+  EXPECT_EQ(dbus::ObjectPath(""), message->GetPath());
+  EXPECT_EQ("", message->GetInterface());
+  EXPECT_EQ("", message->GetMember());
+  EXPECT_EQ("", message->GetErrorName());
+  EXPECT_EQ("", message->GetSender());
+
+  // Empty element between periods.
+  EXPECT_FALSE(message->SetDestination("org..chromium"));
+  // Trailing '/' is only allowed for the root path.
+  EXPECT_FALSE(message->SetPath(dbus::ObjectPath("/org/chromium/")));
+  // Interface name cannot contain '/'.
+  EXPECT_FALSE(message->SetInterface("org/chromium/interface"));
+  // Member name cannot begin with a digit.
+  EXPECT_FALSE(message->SetMember("1member"));
+  // Error name cannot begin with a period.
+  EXPECT_FALSE(message->SetErrorName(".org.chromium.error"));
+  // Disallowed characters.
+  EXPECT_FALSE(message->SetSender("?!#*"));
+
+  EXPECT_EQ("", message->GetDestination());
+  EXPECT_EQ(dbus::ObjectPath(""), message->GetPath());
+  EXPECT_EQ("", message->GetInterface());
+  EXPECT_EQ("", message->GetMember());
+  EXPECT_EQ("", message->GetErrorName());
+  EXPECT_EQ("", message->GetSender());
+}

dbus/object_proxy.cc

@@ -63,11 +63,11 @@ Response* ObjectProxy::CallMethodAndBlock(MethodCall* method_call,
                                           int timeout_ms) {
   bus_->AssertOnDBusThread();
 
-  if (!bus_->Connect())
+  if (!bus_->Connect() ||
+      !method_call->SetDestination(service_name_) ||
+      !method_call->SetPath(object_path_))
     return NULL;
 
-  method_call->SetDestination(service_name_);
-  method_call->SetPath(object_path_);
   DBusMessage* request_message = method_call->raw_message();
 
   ScopedDBusError error;
@@ -108,15 +108,28 @@ void ObjectProxy::CallMethodWithErrorCallback(MethodCall* method_call,
                                               ErrorCallback error_callback) {
   bus_->AssertOnOriginThread();
 
-  method_call->SetDestination(service_name_);
-  method_call->SetPath(object_path_);
+  const base::TimeTicks start_time = base::TimeTicks::Now();
+
+  if (!method_call->SetDestination(service_name_) ||
+      !method_call->SetPath(object_path_)) {
+    // In case of a failure, run the error callback with NULL.
+    DBusMessage* response_message = NULL;
+    base::Closure task = base::Bind(&ObjectProxy::RunResponseCallback,
+                                    this,
+                                    callback,
+                                    error_callback,
+                                    start_time,
+                                    response_message);
+    bus_->PostTaskToOriginThread(FROM_HERE, task);
+    return;
+  }
+
   // Increment the reference count so we can safely reference the
   // underlying request message until the method call is complete. This
   // will be unref'ed in StartAsyncMethodCall().
   DBusMessage* request_message = method_call->raw_message();
   dbus_message_ref(request_message);
 
-  const base::TimeTicks start_time = base::TimeTicks::Now();
   base::Closure task = base::Bind(&ObjectProxy::StartAsyncMethodCall,
                                   this,
                                   timeout_ms,