Bug 768306 - Patch 1: Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread; r=mrbkap
authorKyle Machulis <kyle@nonpolynomial.com>
Wed, 08 Aug 2012 10:46:39 -0700
changeset 101833 8045e57e00dd2e6875006499f63026532ae6246f
parent 101832 2195e7d3d8a33990bf2efa9ce26f118ee10d0729
child 101834 0e2a5e7e2423ccd66e8ffff68d468ec548a0cf9b
push id23253
push userryanvm@gmail.com
push dateThu, 09 Aug 2012 01:21:41 +0000
treeherdermozilla-central@257777cf58fe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs768306
milestone17.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 768306 - Patch 1: Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread; r=mrbkap
dom/bluetooth/BluetoothService.cpp
dom/bluetooth/BluetoothService.h
dom/bluetooth/linux/BluetoothDBusService.cpp
ipc/dbus/DBusUtils.cpp
ipc/dbus/DBusUtils.h
ipc/dbus/RawDBusConnection.h
--- a/dom/bluetooth/BluetoothService.cpp
+++ b/dom/bluetooth/BluetoothService.cpp
@@ -17,44 +17,47 @@
 #include "mozilla/Services.h"
 #include "mozilla/LazyIdleThread.h"
 #include "mozilla/Util.h"
 
 using namespace mozilla;
 
 USING_BLUETOOTH_NAMESPACE
 
-nsRefPtr<BluetoothService> gBluetoothService;
-nsCOMPtr<nsIThread> gToggleBtThread;
-int gPendingInitCount = 0;
-bool gInShutdown = false;
+static nsRefPtr<BluetoothService> gBluetoothService;
+static bool gInShutdown = false;
 
 NS_IMPL_ISUPPORTS1(BluetoothService, nsIObserver)
 
 class ToggleBtAck : public nsRunnable
 {
 public:
+  ToggleBtAck(bool aEnabled) :
+    mEnabled(aEnabled)
+  {
+  }
+  
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread());
-    gPendingInitCount--;
-    
-    if (gPendingInitCount) {
-      return NS_OK;
+
+    if (!mEnabled || gInShutdown) {
+      nsCOMPtr<nsIThread> t;
+      gBluetoothService->mBluetoothCommandThread.swap(t);
+      t->Shutdown();
     }
     
     if (gInShutdown) {
       gBluetoothService = nullptr;
     }
 
-    nsCOMPtr<nsIThread> t;
-    gToggleBtThread.swap(t);
-    t->Shutdown();
     return NS_OK;
   }
+
+  bool mEnabled;
 };
 
 class ToggleBtTask : public nsRunnable
 {
 public:
   ToggleBtTask(bool aEnabled,
                BluetoothReplyRunnable* aRunnable)
     : mEnabled(aEnabled),
@@ -77,17 +80,17 @@ public:
       if (NS_FAILED(gBluetoothService->StopInternal())) {        
         replyError.AssignLiteral("Bluetooth service not available - We should never reach this point!");
       }
     }
 
     // Always has to be called since this is where we take care of our reference
     // count for runnables. If there's an error, replyError won't be empty, so
     // consider our status flipped.
-    nsCOMPtr<nsIRunnable> ackTask = new ToggleBtAck();
+    nsCOMPtr<nsIRunnable> ackTask = new ToggleBtAck(mEnabled);
     if (NS_FAILED(NS_DispatchToMainThread(ackTask))) {
       NS_WARNING("Failed to dispatch to main thread!");
     }
     
     if (!mRunnable) {
       return NS_OK;
     }
     
@@ -163,27 +166,26 @@ BluetoothService::StartStopBluetooth(Blu
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   // If we're shutting down, bail early.
   if (gInShutdown && aStart) {
     NS_ERROR("Start called while in shutdown!");
     return NS_ERROR_FAILURE;
   }
-  if (!gToggleBtThread) {
-    nsresult rv = NS_NewNamedThread("BluetoothCtrl",
-                                    getter_AddRefs(gToggleBtThread));
+  if (!mBluetoothCommandThread) {
+    nsresult rv = NS_NewNamedThread("BluetoothCmd",
+                                    getter_AddRefs(mBluetoothCommandThread));
     NS_ENSURE_SUCCESS(rv, rv);
   }
   nsCOMPtr<nsIRunnable> r = new ToggleBtTask(aStart, aResultRunnable);
-  if (NS_FAILED(gToggleBtThread->Dispatch(r, NS_DISPATCH_NORMAL))) {
+  if (NS_FAILED(mBluetoothCommandThread->Dispatch(r, NS_DISPATCH_NORMAL))) {
     NS_WARNING("Cannot dispatch firmware loading task!");
     return NS_ERROR_FAILURE;
   }
-  gPendingInitCount++;
   return NS_OK;
 }
 
 nsresult
 BluetoothService::Start(BluetoothReplyRunnable* aResultRunnable)
 {
   return StartStopBluetooth(aResultRunnable, true);
 }
--- a/dom/bluetooth/BluetoothService.h
+++ b/dom/bluetooth/BluetoothService.h
@@ -187,16 +187,29 @@ public:
                 const nsAString& aDeviceAddress,
                 nsAString& aDevicePath) = 0;
 
   virtual int
   GetDeviceServiceChannelInternal(const nsAString& aObjectPath,
                                   const nsAString& aPattern,
                                   int aAttributeId) = 0;
 
+  /**
+   * Due to the fact that some operations require multiple calls, a
+   * CommandThread is created that can run blocking, platform-specific calls
+   * where either no asynchronous equivilent exists, or else where multiple
+   * asynchronous calls would require excessive runnable bouncing between main
+   * thread and IO thread.
+   *
+   * For instance, when we retrieve an Adapter object, we would like it to come
+   * with all of its properties filled in and registered as an agent, which
+   * requires a minimum of 3 calls to platform specific code on some platforms.
+   *
+   */
+  nsCOMPtr<nsIThread> mBluetoothCommandThread;
 protected:
   BluetoothService()
   {
     mBluetoothSignalObserverTable.Init();
   }
 
   virtual ~BluetoothService()
   {
--- a/dom/bluetooth/linux/BluetoothDBusService.cpp
+++ b/dom/bluetooth/linux/BluetoothDBusService.cpp
@@ -144,125 +144,135 @@ public:
       NS_WARNING("BluetoothService not available!");
       return NS_ERROR_FAILURE;
     }    
     return bs->DistributeSignal(mSignal);
   }  
 };
 
 bool
-IsDBusMessageError(DBusMessage* aMsg, nsAString& aError)
+IsDBusMessageError(DBusMessage* aMsg, DBusError* aErr, nsAString& aErrorStr)
 {
+  if(aErr && dbus_error_is_set(aErr)) {
+    aErrorStr = NS_ConvertUTF8toUTF16(aErr->message);
+    LOG_AND_FREE_DBUS_ERROR(aErr);
+    return true;
+  }
+  
   DBusError err;
   dbus_error_init(&err);
   if (dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_ERROR) {
     const char* error_msg;
     if (!dbus_message_get_args(aMsg, &err, DBUS_TYPE_STRING,
                                &error_msg, DBUS_TYPE_INVALID) ||
         !error_msg) {
       if (dbus_error_is_set(&err)) {
-        aError = NS_ConvertUTF8toUTF16(err.message);
+        aErrorStr = NS_ConvertUTF8toUTF16(err.message);
         LOG_AND_FREE_DBUS_ERROR(&err);
         return true;
+      } else {
+        aErrorStr.AssignLiteral("Unknown Error");
+        return true;
       }
     } else {
-      aError = NS_ConvertUTF8toUTF16(error_msg);
+      aErrorStr = NS_ConvertUTF8toUTF16(error_msg);
       return true;
     }
   }
   return false;
 }
 
 void
 DispatchBluetoothReply(BluetoothReplyRunnable* aRunnable,
-                       const BluetoothValue& aValue, const nsAString& aError)
+                       const BluetoothValue& aValue, const nsAString& aErrorStr)
 {
   // Reply will be deleted by the runnable after running on main thread
   BluetoothReply* reply;
-  if (!aError.IsEmpty()) {
-    nsString err(aError);
+  if (!aErrorStr.IsEmpty()) {
+    nsString err(aErrorStr);
     reply = new BluetoothReply(BluetoothReplyError(err));
-  }
-  else {
+  } else {
     reply = new BluetoothReply(BluetoothReplySuccess(aValue));
   }
   
   aRunnable->SetReply(reply);
   if (NS_FAILED(NS_DispatchToMainThread(aRunnable))) {
     NS_WARNING("Failed to dispatch to main thread!");
   }
 }
 
 void
-UnpackObjectPathMessage(DBusMessage* aMsg, BluetoothValue& aValue,
-                        nsAString& aErrorStr)
+UnpackObjectPathMessage(DBusMessage* aMsg, DBusError* aErr,
+                        BluetoothValue& aValue, nsAString& aErrorStr)
 {
   DBusError err;
   dbus_error_init(&err);
-  if (!IsDBusMessageError(aMsg, aErrorStr)) {
+  if (!IsDBusMessageError(aMsg, aErr, aErrorStr)) {
     NS_ASSERTION(dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN,
                  "Got dbus callback that's not a METHOD_RETURN!");
     const char* object_path;
     if (!dbus_message_get_args(aMsg, &err, DBUS_TYPE_OBJECT_PATH,
                                &object_path, DBUS_TYPE_INVALID) ||
         !object_path) {
       if (dbus_error_is_set(&err)) {
         aErrorStr = NS_ConvertUTF8toUTF16(err.message);
         LOG_AND_FREE_DBUS_ERROR(&err);
       }
     } else {
       aValue = NS_ConvertUTF8toUTF16(object_path);
     }
   }
 }
 
-typedef void (*UnpackFunc)(DBusMessage*, BluetoothValue&, nsAString&);
+typedef void (*UnpackFunc)(DBusMessage*, DBusError*, BluetoothValue&, nsAString&);
 
 void
 RunDBusCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable,
                 UnpackFunc aFunc)
 {
   MOZ_ASSERT(!NS_IsMainThread());
   nsRefPtr<BluetoothReplyRunnable> replyRunnable =
     dont_AddRef(static_cast< BluetoothReplyRunnable* >(aBluetoothReplyRunnable));
 
   NS_ASSERTION(replyRunnable, "Callback reply runnable is null!");
 
   nsString replyError;
   BluetoothValue v;
-  aFunc(aMsg, v, replyError);
+  aFunc(aMsg, nsnull, v, replyError);
   DispatchBluetoothReply(replyRunnable, v, replyError);  
 }
 
 void
 GetObjectPathCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable)
 {
-  RunDBusCallback(aMsg, aBluetoothReplyRunnable, UnpackObjectPathMessage);
+  RunDBusCallback(aMsg, aBluetoothReplyRunnable,
+                  UnpackObjectPathMessage);
 }
 
 void
-UnpackVoidMessage(DBusMessage* aMsg, BluetoothValue& aValue,
+UnpackVoidMessage(DBusMessage* aMsg, DBusError* aErr, BluetoothValue& aValue,
                   nsAString& aErrorStr)
 {
   DBusError err;
   dbus_error_init(&err);
-  if (!IsDBusMessageError(aMsg, aErrorStr) &&
+  if (!IsDBusMessageError(aMsg, aErr, aErrorStr) &&
       dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN &&
       !dbus_message_get_args(aMsg, &err, DBUS_TYPE_INVALID)) {
     if (dbus_error_is_set(&err)) {
       aErrorStr = NS_ConvertUTF8toUTF16(err.message);
       LOG_AND_FREE_DBUS_ERROR(&err);
     }
   }
 }
 
 void
 GetVoidCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable)
 {
-  RunDBusCallback(aMsg, aBluetoothReplyRunnable, UnpackVoidMessage);
+  RunDBusCallback(aMsg, aBluetoothReplyRunnable,
+                  UnpackVoidMessage);
 }
 
 bool
 GetProperty(DBusMessageIter aIter, Properties* aPropertyTypes,
             int aPropertyTypeLen, int* aPropIndex,
             InfallibleTArray<BluetoothNamedValue>& aProperties)
 {
   DBusMessageIter prop_val, array_val_iter;
@@ -376,72 +386,83 @@ ParseProperties(DBusMessageIter* aIter,
       NS_WARNING("Can't create property!");
       return;
     }
   } while (dbus_message_iter_next(&dict));
 
   aValue = props;
 }
 
-void UnpackPropertiesMessage(DBusMessage* aMsg, BluetoothValue& aValue,
-                             nsAString& aErrorStr, Properties* aPropertyTypes,
-                             const int aPropertyTypeLen)
+void
+UnpackPropertiesMessage(DBusMessage* aMsg, DBusError* aErr,
+                        BluetoothValue& aValue, nsAString& aErrorStr,
+                        Properties* aPropertyTypes,
+                        const int aPropertyTypeLen)
 {
-  if (!IsDBusMessageError(aMsg, aErrorStr) &&
+  if (!IsDBusMessageError(aMsg, aErr, aErrorStr) &&
       dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN) {
     DBusMessageIter iter;
     if (!dbus_message_iter_init(aMsg, &iter)) {
       aErrorStr.AssignLiteral("Cannot create dbus message iter!");
     } else {
       ParseProperties(&iter, aValue, aErrorStr, aPropertyTypes,
                       aPropertyTypeLen);
     }
   }
 }
 
-void UnpackAdapterPropertiesMessage(DBusMessage* aMsg, BluetoothValue& aValue,
-                                    nsAString& aErrorStr)
+void
+UnpackAdapterPropertiesMessage(DBusMessage* aMsg, DBusError* aErr,
+                               BluetoothValue& aValue,
+                               nsAString& aErrorStr)
 {
-  UnpackPropertiesMessage(aMsg, aValue, aErrorStr,
+  UnpackPropertiesMessage(aMsg, aErr, aValue, aErrorStr,
                           sAdapterProperties,
                           ArrayLength(sAdapterProperties));
 }
 
-void UnpackDevicePropertiesMessage(DBusMessage* aMsg, BluetoothValue& aValue,
-                                    nsAString& aErrorStr)
+void
+UnpackDevicePropertiesMessage(DBusMessage* aMsg, DBusError* aErr,
+                              BluetoothValue& aValue,
+                              nsAString& aErrorStr)
 {
-  UnpackPropertiesMessage(aMsg, aValue, aErrorStr,
+  UnpackPropertiesMessage(aMsg, aErr, aValue, aErrorStr,
                           sDeviceProperties,
                           ArrayLength(sDeviceProperties));
 }
 
-void UnpackManagerPropertiesMessage(DBusMessage* aMsg, BluetoothValue& aValue,
-                                    nsAString& aErrorStr)
+void
+UnpackManagerPropertiesMessage(DBusMessage* aMsg, DBusError* aErr,
+                               BluetoothValue& aValue,
+                               nsAString& aErrorStr)
 {
-  UnpackPropertiesMessage(aMsg, aValue, aErrorStr,
+  UnpackPropertiesMessage(aMsg, aErr, aValue, aErrorStr,
                           sManagerProperties,
                           ArrayLength(sManagerProperties));
 }
 
 void
 GetManagerPropertiesCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable)
 {
-  RunDBusCallback(aMsg, aBluetoothReplyRunnable, UnpackManagerPropertiesMessage);
+  RunDBusCallback(aMsg, aBluetoothReplyRunnable,
+                  UnpackManagerPropertiesMessage);
 }
 
 void
 GetAdapterPropertiesCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable)
 {
-  RunDBusCallback(aMsg, aBluetoothReplyRunnable, UnpackAdapterPropertiesMessage);
+  RunDBusCallback(aMsg, aBluetoothReplyRunnable,
+                  UnpackAdapterPropertiesMessage);
 }
 
 void
 GetDevicePropertiesCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable)
 {
-  RunDBusCallback(aMsg, aBluetoothReplyRunnable, UnpackDevicePropertiesMessage);
+  RunDBusCallback(aMsg, aBluetoothReplyRunnable,
+                  UnpackDevicePropertiesMessage);
 }
 
 static DBusCallback sBluetoothDBusPropCallbacks[] =
 {
   GetManagerPropertiesCallback,
   GetAdapterPropertiesCallback,
   GetDevicePropertiesCallback
 };
@@ -606,17 +627,16 @@ BluetoothDBusService::StartInternal()
     return NS_ERROR_FAILURE;
   }
   
   if (mConnection) {
     return NS_OK;
   }
 
   if (NS_FAILED(EstablishDBusConnection())) {
-    NS_WARNING("Cannot start DBus connection!");
     StopDBus();
     return NS_ERROR_FAILURE;
   }
 
   DBusError err;
   dbus_error_init(&err);
 
   // Set which messages will be processed by this dbus connection.
--- a/ipc/dbus/DBusUtils.cpp
+++ b/ipc/dbus/DBusUtils.cpp
@@ -194,38 +194,39 @@ DBusMessage * dbus_func_args_timeout_val
   if (!dbus_message_append_args_valist(msg, first_arg_type, args)) {
     LOG("Could not append argument to method call!");
     goto done;
   }
 
   /* Make the call. */
   reply = dbus_connection_send_with_reply_and_block(conn, msg, timeout_ms, err);
   if (!return_error && dbus_error_is_set(err)) {
-    //LOG_AND_FREE_DBUS_ERROR_WITH_MSG(err, msg);
+    LOG_AND_FREE_DBUS_ERROR_WITH_MSG(err, msg);
   }
 
 done:
   if (!return_error) {
     free(err);
   }
   if (msg) dbus_message_unref(msg);
   return reply;
 }
 
 DBusMessage * dbus_func_args_timeout(DBusConnection *conn,
                                      int timeout_ms,
+                                     DBusError* err,
                                      const char *path,
                                      const char *ifc,
                                      const char *func,
                                      int first_arg_type,
                                      ...) {
   DBusMessage *ret;
   va_list lst;
   va_start(lst, first_arg_type);
-  ret = dbus_func_args_timeout_valist(conn, timeout_ms, NULL,
+  ret = dbus_func_args_timeout_valist(conn, timeout_ms, err,
                                       path, ifc, func,
                                       first_arg_type, lst);
   va_end(lst);
   return ret;
 }
 
 DBusMessage * dbus_func_args(DBusConnection *conn,
                              const char *path,
--- a/ipc/dbus/DBusUtils.h
+++ b/ipc/dbus/DBusUtils.h
@@ -85,16 +85,17 @@ DBusMessage*  dbus_func_args_error(DBusC
                                    const char* path,
                                    const char* ifc,
                                    const char* func,
                                    int first_arg_type,
                                    ...);
 
 DBusMessage*  dbus_func_args_timeout(DBusConnection* conn,
                                      int timeout_ms,
+                                     DBusError* err,
                                      const char* path,
                                      const char* ifc,
                                      const char* func,
                                      int first_arg_type,
                                      ...);
 
 DBusMessage*  dbus_func_args_timeout_valist(DBusConnection* conn,
                                             int timeout_ms,
--- a/ipc/dbus/RawDBusConnection.h
+++ b/ipc/dbus/RawDBusConnection.h
@@ -26,15 +26,19 @@ class RawDBusConnection
   {
     static void release(DBusConnection* ptr);
   };
 
 public:
   RawDBusConnection();
   ~RawDBusConnection();
   nsresult EstablishDBusConnection();
+  DBusConnection* GetConnection() {
+    return mConnection;
+  }
+protected:
   Scoped<ScopedDBusConnectionPtrTraits> mConnection;
 };
 
 }
 }
 
 #endif