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 101840 8045e57e00dd2e6875006499f63026532ae6246f
parent 101839 2195e7d3d8a33990bf2efa9ce26f118ee10d0729
child 101841 0e2a5e7e2423ccd66e8ffff68d468ec548a0cf9b
push id994
push userttaubert@mozilla.com
push dateThu, 09 Aug 2012 18:49:12 +0000
treeherderfx-team@4770bca01046 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs768306
milestone17.0a1
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