The Bluetooth 'Device Discovering' routine would be started by user's action such as pressing 'Discover' button on Settings UI, and then the StartDiscovery() function of BluetoothAdapter will be called. StartDiscovery() is an asynchronous function, during the period of discovering, a 'DeviceFound' event need to be fired to registered listeners whenever a remote Bluetooth device is discovered.

Taking this one since it's a good end-point for the work I've been doing currently.

Comment on attachment 630082 [details] [diff] [review] v3: Fire DeviceFound event whenever a remote Bluetooth device is discovered Going to backport service changes needed for e10s to this before review. Probably refiling for review this evening.

Note to reviewers: There's some already-reviewed code rearranging happening here that's not showing up as moves. I moved what was in BluetoothUtils from Bug 744349 to BluetoothDBusService, and surrounded it in a class so we can inherit from BluetoothService and create a NULL pointer to the service if we're not supported (Needed for implementing e10s). Also: Not sure the way I'm doing the factory is right, since it's basically a singleton being maintained outside its own class.

Comment on attachment 631505 [details] [diff] [review] v5: Fire DeviceFound event whenever a remote Bluetooth device is discovered Review of attachment 631505 [details] [diff] [review]: ----------------------------------------------------------------- Hm, I know you said that a bunch of code was moved, but I found some blocking and maybe leaks in it anyway. ::: dom/Makefile.in @@ +76,4 @@ > endif > > ifdef MOZ_B2G_BT > +DIRS += bluetooth I would revert changes to this file. ::: dom/base/nsDOMClassInfo.cpp @@ +1655,5 @@ > EVENTTARGET_SCRIPTABLE_FLAGS) > NS_DEFINE_CLASSINFO_DATA(BluetoothAdapter, nsEventTargetSH, > + EVENTTARGET_SCRIPTABLE_FLAGS) > + NS_DEFINE_CLASSINFO_DATA(BluetoothDevice, nsDOMGenericSH, > + DOM_DEFAULT_SCRIPTABLE_FLAGS) This thing is probably an event target, right? Maybe just not yet? @@ +4511,5 @@ > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMBluetoothDevice) > + DOM_CLASSINFO_MAP_END > + > + DOM_CLASSINFO_MAP_BEGIN(BluetoothDeviceEvent, nsIDOMBluetoothDeviceEvent) > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMBluetoothDeviceEvent) You also want nsIDOMEvent ::: dom/bluetooth/BluetoothAdapter.cpp @@ +7,5 @@ > #include "BluetoothAdapter.h" > +#include "BluetoothServicesFactory.h" > +#include "BluetoothService.h" > +#include "BluetoothDevice.h" > +#include "BluetoothDeviceEvent.h" Nit: I usually try to alphabetize so you can avoid double-including. @@ +45,5 @@ > NS_IMPL_ADDREF_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper) > NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper) > > +BluetoothAdapter::BluetoothAdapter(const nsAString& aPath) : > +mPath(aPath) You're not initializing any of your additional POD members. Also please indent a little here. @@ +52,5 @@ > > BluetoothAdapter::~BluetoothAdapter() > { > + BluetoothService* bs = GetBluetoothService(); > + if(!bs) { Nit: Space between if and (. You seem to have copy/pasted this in lots of places below. @@ +54,5 @@ > { > + BluetoothService* bs = GetBluetoothService(); > + if(!bs) { > + NS_WARNING("Failed to get bluetooth service manager!"); > + } else if(NS_FAILED(bs->UnregisterBluetoothEventHandler(mPath, this))) { Seems odd that you have to pass mPath here. Couldn't you take care of this with just the 'this' pointer? @@ +61,5 @@ > } > > // static > already_AddRefed<BluetoothAdapter> > +BluetoothAdapter::Create(const nsAString& aPath) { Can you assert anything about the path here? At least that it's non-empty? @@ +77,5 @@ > } > > +void > +BluetoothAdapter::Notify(const BluetoothEvent& aData) { > + if (aData.mEventName.Equals(NS_LITERAL_STRING("DeviceFound"))) { EqualsLiteral is what you want here. @@ +86,3 @@ > } > + > +NS_IMETHODIMP BluetoothAdapter::StartDiscovery(bool* b) { Nit: NS_IMETHODIMP on its own line, and please name your variable something descriptive. @@ +90,5 @@ > + if(!bs) { > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NS_ERROR_FAILURE; > + } > + bs->StartDiscoveryInternal(mPath); This isn't ok, it's a synchronous call. Also, it could fail, and you're not checking here. We need to redesign this to return a DOMRequest. Also, shouldn't you set mDiscovering to true here? @@ +101,5 @@ > + if(!bs) { > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NS_ERROR_FAILURE; > + } > + bs->StopDiscoveryInternal(mPath); Another sync call, and you're not checking for errors, nor are you resetting mDiscovering. @@ +160,5 @@ > +{ > + PRUint32 length = mDevices.Length(); > + > + if (length == 0) { > + *aDevices = JSVAL_NULL; Let's return an empty array here instead. That will make for nicer JS. @@ +163,5 @@ > + if (length == 0) { > + *aDevices = JSVAL_NULL; > + return NS_OK; > + } > + BluetoothService* bs = GetBluetoothService(); You don't seem to use this. @@ +169,5 @@ > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NS_ERROR_FAILURE; > + } > + > + JSObject* array = JS_NewArrayObject(aCx, length, nsnull); This can fail. @@ +170,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + JSObject* array = JS_NewArrayObject(aCx, length, nsnull); > + nsresult rv = NS_OK; Nit: No need to initialize. @@ +174,5 @@ > + nsresult rv = NS_OK; > + > + for (PRUint32 i = 0; i < length; ++i) { > + jsval val; > + JSObject* scope = JS_GetGlobalForScopeChain(aCx); Lift this outside the loop, it won't change. @@ +176,5 @@ > + for (PRUint32 i = 0; i < length; ++i) { > + jsval val; > + JSObject* scope = JS_GetGlobalForScopeChain(aCx); > + > + nsCOMPtr<BluetoothDevice> device = BluetoothDevice::Create(mDevices[i]); Use nsRefPtr for concrete pointers (nsCOMPtr is for interface pointers only). @@ +177,5 @@ > + jsval val; > + JSObject* scope = JS_GetGlobalForScopeChain(aCx); > + > + nsCOMPtr<BluetoothDevice> device = BluetoothDevice::Create(mDevices[i]); > + //UpdateDeviceProperties(device); Nit: Commented-out code shouldn't get checked in, either it's not needed or it should have a more descriptive comment (e.g. "we would do this, but...") @@ +179,5 @@ > + > + nsCOMPtr<BluetoothDevice> device = BluetoothDevice::Create(mDevices[i]); > + //UpdateDeviceProperties(device); > + > + rv = nsContentUtils::WrapNative(aCx, scope, device, &val, nsnull, true); Leave off those last two optional args. @@ +182,5 @@ > + > + rv = nsContentUtils::WrapNative(aCx, scope, device, &val, nsnull, true); > + if (!JS_SetElement(aCx, array, i, &val)) { > + //LOG("Set element error."); > + return NS_OK; This isn't right. You need to return an error if that fails. Remember, returning NS_OK without setting all outvals will cause XPConnect to return garbage to JS. Also either use NS_WARNING or remove that LOG call. @@ +195,5 @@ > +NS_IMETHODIMP > +BluetoothAdapter::GetRemoteDevice(const nsAString& aAddress, > + nsIDOMBluetoothDevice** aDevice) > +{ > + nsRefPtr<BluetoothDevice> device = BluetoothDevice::Create(aAddress); You're not really sanitizing aAddress at all here... Should you? Can you at least NS_ENSURE that it's non-empty? Or that the address belongs to this adapter somehow? ::: dom/bluetooth/BluetoothAdapter.h @@ +54,1 @@ > private: Nit: Newline between } and private @@ +54,2 @@ > private: > + void SetPropertyByVariant(const BluetoothNamedVariant& aValue); Don't you need to forward declare this? And I'd add an extra line after this method before the members. @@ +63,5 @@ > + bool mPowered; > + PRUint32 mPairableTimeout; > + PRUint32 mDiscoverableTimeout; > + PRUint32 mClass; > + nsTArray<nsString> mDevices; Hm, this isn't an array of "devices", it's an array of device addresses. Can we rename this to be more descriptive? @@ +70,1 @@ > BluetoothAdapter() {} Can you keep all your methods above these members? Otherwise it gets hard to find everything. @@ +70,2 @@ > BluetoothAdapter() {} > + BluetoothAdapter(const nsAString& aName); Why the two constructors? Do you need the no-arg one? ::: dom/bluetooth/BluetoothCommon.h @@ +55,2 @@ > */ > struct BluetoothEvent I think this name is misleading... It's not an event, like a DOM event. It's more like "BluetoothEventData" or something. Let's put our heads together and figure out a better name. ::: dom/bluetooth/BluetoothDevice.cpp @@ +10,5 @@ > +#if defined(MOZ_WIDGET_GONK) > +#include <android/log.h> > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "Bluetooth", args) > +#else > +#define LOG(args...) printf(args); printf("\n"); You don't seem to use this, but that extra ; at the end is different from the GONK case. @@ +17,5 @@ > +USING_BLUETOOTH_NAMESPACE > + > +DOMCI_DATA(BluetoothDevice, BluetoothDevice) > + > + NS_INTERFACE_MAP_BEGIN(BluetoothDevice) Nit: Un-indent first and last line here. @@ +24,5 @@ > + NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(BluetoothDevice) > + NS_INTERFACE_MAP_END > + > +NS_IMPL_ADDREF(BluetoothDevice) > + NS_IMPL_RELEASE(BluetoothDevice) Nit: Un-indent. @@ +27,5 @@ > +NS_IMPL_ADDREF(BluetoothDevice) > + NS_IMPL_RELEASE(BluetoothDevice) > + > +BluetoothDevice::BluetoothDevice(const nsAString& aAddress) : > + mAddress(aAddress) Nit: Looks like this could be moved to the header? @@ +31,5 @@ > + mAddress(aAddress) > +{ > +} > + > +BluetoothDevice::BluetoothDevice(const BluetoothEvent& aEvent) So what about mAddress for this constructor? In general having more than one constructor is a little confusing ("Which one am I supposed to call? What's the difference between the two?"). Can we fix this so that there's only one? @@ +41,5 @@ > + > +void > +BluetoothDevice::SetPropertyByVariant(const BluetoothNamedVariant& aValue) > +{ > + if(aValue.mName.Equals(NS_LITERAL_STRING("Name"))) { EqualsLiteral for all these. @@ +51,5 @@ > + } else if (aValue.mName.Equals(NS_LITERAL_STRING("Connected"))) { > + mConnected = aValue.mValue.mUint32Value ? true : false; > + } else if (aValue.mName.Equals(NS_LITERAL_STRING("Paired"))) { > + mPaired = aValue.mValue.mUint32Value ? true : false; > + } else if (aValue.mName.Equals(NS_LITERAL_STRING("UUIDs"))) { Warn if you don't see a match here? @@ +66,5 @@ > + nsRefPtr<BluetoothDevice> device = new BluetoothDevice(aAddress); > + return device.forget(); > +} > + > +// We can just create a device object out of a DeviceFound event return This comment doesn't make sense ::: dom/bluetooth/BluetoothDevice.h @@ +19,5 @@ > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIDOMBLUETOOTHDEVICE > + > + nsString mAddress; All these members should be private, right? @@ +29,5 @@ > + > + static already_AddRefed<BluetoothDevice> > + Create(const nsAString& aAddress); > + > + // We can just create a device object out of a DeviceFound event return Nit: This comment doesn't make sense. @@ +38,5 @@ > + BluetoothDevice(const BluetoothEvent& aValue); > + BluetoothDevice(const nsAString& aAddress); > + void SetPropertyByVariant(const BluetoothNamedVariant& aValue); > + ~BluetoothDevice() {} > + Nit: Nuke extra line. ::: dom/bluetooth/BluetoothDeviceEvent.cpp @@ +26,5 @@ > +} > + > +NS_IMPL_CYCLE_COLLECTION_CLASS(BluetoothDeviceEvent) > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(BluetoothDeviceEvent, You need to traverse mDevice. ::: dom/bluetooth/BluetoothDeviceEvent.h @@ +18,5 @@ > + > +class BluetoothDevice; > + > +class BluetoothDeviceEvent : public nsDOMEvent > + , public nsIDOMBluetoothDeviceEvent Nit: , on prev line @@ +20,5 @@ > + > +class BluetoothDeviceEvent : public nsDOMEvent > + , public nsIDOMBluetoothDeviceEvent > +{ > + nsRefPtr<BluetoothDevice> mDevice; Nit: The rest of your bluetooth classes have members at the bottom. Telephony was different, but I think consistency should win. ::: dom/bluetooth/BluetoothManager.cpp @@ +173,4 @@ > > BluetoothManager::BluetoothManager(nsPIDOMWindow *aWindow) : > mEnabled(false), > + mName(NS_LITERAL_STRING("/")) I'd do: mName.AssignLiteral("/") in the body of the constructor to avoid the temp object. @@ +179,5 @@ > } > > BluetoothManager::~BluetoothManager() > { > + BluetoothService* es = GetBluetoothService(); "es"? @@ +180,5 @@ > > BluetoothManager::~BluetoothManager() > { > + BluetoothService* es = GetBluetoothService(); > + if(!es) { Nit: space between if and (, below in a few places too. @@ +232,5 @@ > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NS_ERROR_FAILURE; > + } > + > + nsresult rv = bs->GetDefaultAdapterPathInternal(path); This blocks currently, and that's not ok. In the future when this doesn't block then our API will have to be async, returning a DOMRequest, etc. We need to fix this now before we go much further. @@ +246,4 @@ > // static > already_AddRefed<BluetoothManager> > BluetoothManager::Create(nsPIDOMWindow* aWindow) { > + BluetoothService* bs = GetBluetoothService(); You don't seem to use this? @@ +248,5 @@ > BluetoothManager::Create(nsPIDOMWindow* aWindow) { > + BluetoothService* bs = GetBluetoothService(); > + if(!bs) { > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NULL; Nit: nsnull ::: dom/bluetooth/BluetoothService.cpp @@ +5,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "BluetoothService.h" > +#include "BluetoothServicesFactory.h" > +#include "BluetoothCommon.h" This was already included in your header. @@ +9,5 @@ > +#include "BluetoothCommon.h" > + > +USING_BLUETOOTH_NAMESPACE > + > +BluetoothService::BluetoothService() { Nit: { on next line. @@ +10,5 @@ > + > +USING_BLUETOOTH_NAMESPACE > + > +BluetoothService::BluetoothService() { > + mBluetoothEventObserverTable.Init(100); 100 seems... very large. Just leave it with the defaults? @@ +15,5 @@ > +} > + > +nsresult > +BluetoothService::RegisterBluetoothEventHandler(const nsAString& aNodeName, > + BluetoothEventObserver* aHandler) Nit: indent is off here. @@ +18,5 @@ > +BluetoothService::RegisterBluetoothEventHandler(const nsAString& aNodeName, > + BluetoothEventObserver* aHandler) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + BluetoothEventObserverList *ol; Nit: * on the left. @@ +21,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + BluetoothEventObserverList *ol; > + if (!mBluetoothEventObserverTable.Get(aNodeName, &ol)) { > + mBluetoothEventObserverTable.Put(aNodeName, > + new BluetoothEventObserverList()); This isn't ok, you're not calling Init on the new hash table. Also, your indent is off. @@ +23,5 @@ > + if (!mBluetoothEventObserverTable.Get(aNodeName, &ol)) { > + mBluetoothEventObserverTable.Put(aNodeName, > + new BluetoothEventObserverList()); > + } > + mBluetoothEventObserverTable.Get(aNodeName, &ol); This is doing two lookups, just set ol to the newly created hash table if it doesn't exist. E.g.: BluetoothEventObserverList* ol; if (!mBluetoothEventObserverTable.Get(aNodeName, &ol)) { ol = new BluetoothEventObserverList(); ol.Init(); mBluetoothEventObserverTable.Put(aNodeName, ol); } ol->AddObserver(aHandler); @@ +30,5 @@ > +} > + > +nsresult > +BluetoothService::UnregisterBluetoothEventHandler(const nsAString& aNodeName, > + BluetoothEventObserver* aHandler) Nit: indent is off. @@ +36,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + BluetoothEventObserverList *ol; > + if (!mBluetoothEventObserverTable.Get(aNodeName, &ol)) { > + NS_WARNING("Node does not exist to remove BluetoothEventListener from!"); > + return NS_ERROR_FAILURE; We generally don't return errors for this kind of thing (it's not in the list any more...). Warning is good though. @@ +38,5 @@ > + if (!mBluetoothEventObserverTable.Get(aNodeName, &ol)) { > + NS_WARNING("Node does not exist to remove BluetoothEventListener from!"); > + return NS_ERROR_FAILURE; > + } > + mBluetoothEventObserverTable.Get(aNodeName, &ol); Don't do this second lookup. @@ +40,5 @@ > + return NS_ERROR_FAILURE; > + } > + mBluetoothEventObserverTable.Get(aNodeName, &ol); > + ol->RemoveObserver(aHandler); > + if (ol->Length() == 0) { I generally prefer IsEmpty() if that's what you actually care about. @@ +47,5 @@ > + return NS_OK; > +} > + > +nsresult > +BluetoothService::DistributeEvent(const BluetoothEvent& event) { Nit: { on next line @@ +49,5 @@ > + > +nsresult > +BluetoothService::DistributeEvent(const BluetoothEvent& event) { > +// Notify observers that a message has been sent > + BluetoothEventObserverList *ol; Nit: * on the left. @@ +51,5 @@ > +BluetoothService::DistributeEvent(const BluetoothEvent& event) { > +// Notify observers that a message has been sent > + BluetoothEventObserverList *ol; > + if (!mBluetoothEventObserverTable.Get(event.mEventPath, &ol)) { > + NS_WARNING("No objects registered for msg, returning\n"); No \n for NS_WARNING. Also, why warn in this case? Seems likely to happen in practice. @@ +62,5 @@ > +nsresult > +BluetoothService::DistributeBluetoothEventTask::Run() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + BluetoothService* es = GetBluetoothService(); "es"? @@ +63,5 @@ > +BluetoothService::DistributeBluetoothEventTask::Run() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + BluetoothService* es = GetBluetoothService(); > + if(!es) { Nit: space here. ::: dom/bluetooth/BluetoothService.h @@ +13,5 @@ > +#include "nsClassHashtable.h" > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +class BluetoothService { Nit: { on next line. @@ +17,5 @@ > +class BluetoothService { > +public: > + BluetoothService(); > + virtual ~BluetoothService() {} > + /** Nit: newline between the destructor and this. @@ +28,5 @@ > + * @return NS_OK on successful addition to observer, NS_ERROR_FAILED > + * otherwise > + */ > + nsresult RegisterBluetoothEventHandler(const nsAString& aNodeName, > + BluetoothEventObserver *aMsgHandler); Nit: * on the left, here and below. @@ +43,5 @@ > + */ > + nsresult UnregisterBluetoothEventHandler(const nsAString& aNodeName, > + BluetoothEventObserver *aMsgHandler); > + > + nsresult DistributeEvent(const BluetoothEvent& ev); Nit: params all get 'a' prefix, and a nicer name ;) @@ +47,5 @@ > + nsresult DistributeEvent(const BluetoothEvent& ev); > + > + virtual nsresult StartBluetoothService() = 0; > + virtual nsresult StopBluetoothService() = 0; > + /** Nit: newline there, and indent is off. @@ +73,5 @@ > + * @return NS_OK if discovery stopped correctly, false otherwise > + */ > + virtual nsresult StartDiscoveryInternal(const nsAString& aAdapterPath) = 0; > + > + struct DistributeBluetoothEventTask : public nsRunnable { Nit: class, you're inheriting a class. @@ +78,5 @@ > + BluetoothEvent mEvent; > + > + DistributeBluetoothEventTask(BluetoothEvent aEvent) : mEvent(aEvent) > + { > + } Nit: You use {} above, let's be consistent either way. ::: dom/bluetooth/BluetoothServicesFactory.cpp @@ +12,5 @@ > +#endif > + > +USING_BLUETOOTH_NAMESPACE > + > +BluetoothService* sBluetoothService = NULL; This isn't static, use a 'g' for global? @@ +13,5 @@ > + > +USING_BLUETOOTH_NAMESPACE > + > +BluetoothService* sBluetoothService = NULL; > +static bool sTriedService = false; This needn't be static. @@ +18,5 @@ > + > +BluetoothService* > +mozilla::dom::bluetooth::GetBluetoothService() > +{ > + if(!sTriedService) { Nit: spaces between if and (, here and below. @@ +22,5 @@ > + if(!sTriedService) { > + sTriedService = true; > +#ifdef MOZ_B2G_BT > + if(!sBluetoothService) { > + sBluetoothService = new BluetoothDBusService(); Wait... who is responsible for deleting this on shutdown? ::: dom/bluetooth/Makefile.in @@ +7,5 @@ > srcdir = @srcdir@ > +VPATH = \ > + $(srcdir) \ > + $(srcdir)/ipc \ > + $(NULL) Revert this @@ +62,5 @@ > endif > + > +# Add VPATH to LOCAL_INCLUDES so we are going to include the correct backend > +# subdirectory (and the ipc one). > +LOCAL_INCLUDES += $(VPATH:%=-I%) I don't see what this does for you. Revert? ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +104,5 @@ > +}; > + > +int > +get_property(DBusMessageIter iter, Properties* properties, > + int max_num_properties, int *prop_index, Nit: * on the left. There are lots of places where this is inconsistent below. @@ +166,5 @@ > + return 0; > +} > + > +nsTArray<BluetoothNamedVariant> > +parse_properties(DBusMessageIter *iter, This copies your array every time... Would be much better to have a nsTArray& out param so you could modify in-place. @@ +203,5 @@ > + return values; > +} > + > +BluetoothNamedVariant > +parse_property_change(DBusMessage *msg, Properties* properties, This copies too. @@ +230,5 @@ > +// Called by dbus during WaitForAndDispatchEventNative() > +// This function is called on the IOThread > +static > +DBusHandlerResult > +EventFilter(DBusConnection *aConn, DBusMessage *aMsg, void *aData) Can you add an assertion here that this is not the main thread? @@ +247,5 @@ > + > + BluetoothEvent event; > + DBusError err; > + dbus_error_init(&err); > + event.mEventPath = TO_UTF16(dbus_message_get_path(aMsg)); I think these macros are sort of a bad idea. Here, for instance, we're converting utf8->utf16, then we're a) counting the length of the converted string, and b) copying that converted string. That's twice as much work as we need to do, and we're doing it for every message. @@ +252,5 @@ > + LOG("%s: Received signal %s:%s from %s\n", __FUNCTION__, > + dbus_message_get_interface(aMsg), dbus_message_get_member(aMsg), > + dbus_message_get_path(aMsg)); > + > + event.mEventName = TO_UTF16(dbus_message_get_member(aMsg)); Here too... @@ +262,5 @@ > + DBusMessageIter iter; > + > + if (dbus_message_iter_init(aMsg, &iter)) { > + BluetoothNamedVariant v; > + v.mName = NS_LITERAL_STRING("Address"); AssignLiteral @@ +273,5 @@ > + event.mValues.AppendElement(v); > + } > + } else if (dbus_message_is_signal(aMsg, "org.bluez.Adapter", "DeviceDisappeared")) { > + BluetoothNamedVariant v; > + v.mName = NS_LITERAL_STRING("Address"); AssignLiteral @@ +303,5 @@ > + return DBUS_HANDLER_RESULT_HANDLED; > +} > + > +nsresult > +BluetoothDBusService::StartBluetoothService() Assert that this is the main thread. @@ +305,5 @@ > + > +nsresult > +BluetoothDBusService::StartBluetoothService() > +{ > + if(mConnection) { Nit: space here @@ +310,5 @@ > + NS_WARNING("DBusConnection already established, skipping"); > + return NS_OK; > + } > + > + EstablishDBusConnection(); This calls dbus_bus_get. According to the docs I saw (http://dbus.freedesktop.org/doc/api/html/group__DBusBus.html), "If returning a newly-created connection, this function will block until authentication and bus registration are complete." The StartDBus() call in SystemWorkerManager kicks off an async process to create this connection so I think this means that we will block here. We can't call this on the main thread. @@ +313,5 @@ > + > + EstablishDBusConnection(); > + > + // Add a filter for all incoming messages_base > + if (!dbus_connection_add_filter(mConnection, EventFilter, It's unclear whether this blocks or not. The docs don't say, and I would assume ("ass-u-me", etc.) that it doesn't, but we should find out. In general I think we should make a rule that all dbus calls happen on a non-main thread unless they're guaranteed to be threadsafe and non-blocking. @@ +323,5 @@ > + return NS_OK; > +} > + > +nsresult > +BluetoothDBusService::StopBluetoothService() Assert that this is the main thread. @@ +325,5 @@ > + > +nsresult > +BluetoothDBusService::StopBluetoothService() > +{ > + if(!mConnection) { Nit: space @@ +326,5 @@ > +nsresult > +BluetoothDBusService::StopBluetoothService() > +{ > + if(!mConnection) { > + NS_WARNING("DBusConnection does not exist, nothing to stop, skipping."); Nit: Not sure this is worth warning about. Up to you. @@ +336,5 @@ > + return NS_OK; > +} > + > +nsresult > +BluetoothDBusService::GetDefaultAdapterPathInternal(nsAString& aAdapterPath) Assert that this is the main thread. @@ +345,5 @@ > + int attempt = 0; > + > + for (attempt = 0; attempt < 1000 && reply == NULL; attempt ++) { > + msg = dbus_message_new_method_call("org.bluez", "/", > + "org.bluez.Manager", "DefaultAdapter"); You're making a new msg in every loop here, but i only see you derefing one below. I think this will leak? @@ +355,5 @@ > + dbus_message_append_args(msg, DBUS_TYPE_INVALID); > + dbus_error_init(&err); > + > + // TODO: This should not block > + reply = dbus_connection_send_with_reply_and_block( Really, it shouldn't, and I'm not very comfortable doing this now. Also, we're in a loop of 1000 tries! Need to chat with cjones to see if we're ok with short-term blocking. I would say no, but I'm happy to let him make the final call. @@ +365,5 @@ > + "org.freedesktop.DBus.Error.ServiceUnknown")) { > + // bluetoothd is still down, retry > + LOG("Service unknown\n"); > + dbus_error_free(&err); > + //usleep(10000); // 10 ms Nit: Please remove commented-out code. @@ +398,5 @@ > + dbus_message_unref(msg); > + aAdapterPath = TO_UTF16(device_path); > + return NS_OK; > +failed: > + dbus_message_unref(msg); What about 'reply'? Is that leaking? @@ +403,5 @@ > + return NS_ERROR_FAILURE; > +} > + > +nsresult > +BluetoothDBusService::StopDiscoveryInternal(const nsAString& aAdapterPath) This and StartDiscoveryInternal need to be reworked to be async. Also need to assert main thread on both. @@ +413,5 @@ > + // TODO(Eric) > + // "dbus_func_args()": supposed to be implemented in another .cpp > + // "GetCurrentConnection()": supposed to be implemented in another .cpp > + reply = dbus_func_args(mConnection, > + TO_UTF8(aAdapterPath), This is only safe as long as this is a blocking call, right? Otherwise you're not holding the string's buffer alive in any way. Will need to change this a bit. Same problem in StartDiscoveryInternal below. @@ +427,5 @@ > + LOG("DBus reply is NULL in function %s\n", __FUNCTION__); > + return false; > + } > + > + return true; Do we have to deref reply? In StartDiscoveryInternal too. @@ +431,5 @@ > + return true; > +} > + > +nsresult > +BluetoothDBusService::StartDiscoveryInternal(const nsAString& aAdapterPath) This is exactly the same as StopDiscoveryInternal, just a different string passed to dbus_func_args. Let's combine the two? ::: dom/bluetooth/linux/BluetoothDBusService.h @@ +22,5 @@ > + * here. > + */ > + > +class BluetoothDBusService : public BluetoothService > + , private mozilla::ipc::RawDBusConnection Nit: , on prev line. @@ +43,5 @@ > + * otherwise > + */ > + virtual nsresult StopBluetoothService(); > + > + /** Nit: Indent is off. @@ +70,5 @@ > + */ > + virtual nsresult StartDiscoveryInternal(const nsAString& aAdapterPath); > + > +}; > +END_BLUETOOTH_NAMESPACE Nit: newline between } and END_ ::: dom/bluetooth/nsIDOMBluetoothAdapter.idl @@ +19,5 @@ > + [implicit_jscontext] > + readonly attribute jsval devices; > + > + readonly attribute DOMString name; // Set custom device name > + readonly attribute bool discoverable; // Set to false if this device would not like to be discovered There's nothing to "set" here, they're readonly. Let's fix these comments. @@ +22,5 @@ > + readonly attribute DOMString name; // Set custom device name > + readonly attribute bool discoverable; // Set to false if this device would not like to be discovered > + readonly attribute unsigned long discoverableTimeout; // Unit: sec > + > + bool startDiscovery(); // ret: false if bluetooth is not enabled These all need to change to return DOMRequests. ::: dom/bluetooth/nsIDOMBluetoothDevice.idl @@ +13,5 @@ > + readonly attribute DOMString name; > + readonly attribute unsigned long class; > + readonly attribute bool connected; > + readonly attribute bool paired; > + Nit: Nuke extra line. ::: dom/bluetooth/nsIDOMBluetoothDeviceEvent.idl @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsISupports.idl" > +#include "nsIDOMEvent.idl" Nit: Only need the nsIDOMEvent include. ::: dom/system/gonk/SystemWorkerManager.cpp @@ +53,1 @@ > Nit: Only one newline here, not three. @@ +272,4 @@ > > StopRil(); > #ifdef MOZ_B2G_BT > + BluetoothService* bs = GetBluetoothService(); You've null-checked everywhere else and you should here too. @@ +400,5 @@ > if(EnsureBluetoothInit()) { > #endif > StartDBus(); > + BluetoothService* bs = GetBluetoothService(); > + if(bs) { Nit: space ::: dom/system/gonk/SystemWorkerManager.h @@ +20,5 @@ > > namespace mozilla { > namespace dom { > +namespace bluetooth { > +class BluetoothService; You don't seem to need this here.

Comment on attachment 631505 [details] [diff] [review] v5: Fire DeviceFound event whenever a remote Bluetooth device is discovered Retracting reviews until the last review notes are taken care of, since this won't pass any sort of review until we get everything async'd.

> Hm, I know you said that a bunch of code was moved, but I found some > blocking and maybe leaks in it anyway. Yeah, pretty much everything you found was in new stuff. I think I overstated how much moved, was mainly shuffling things so we could null out BluetoothService when we're on a platform we don't support, for future e10s work. There's also a lot of this that got cut/pasted from a couple of other branches without me rethinking it, that's my fault. > @@ +45,5 @@ > > NS_IMPL_ADDREF_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper) > > NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper) > > > > +BluetoothAdapter::BluetoothAdapter(const nsAString& aPath) : > > +mPath(aPath) > > You're not initializing any of your additional POD members. Also please > indent a little here. So this one is kinda weird. All we have at this point is the adapter address. I'm thinking I can do the same thing we do with devices, kick off a properties query before the object is created and pass in a whole property array. Since this will become a DOMRequest anyways, we'll have time to do 2 calls in the async portion. > @@ +54,5 @@ > > { > > + BluetoothService* bs = GetBluetoothService(); > > + if(!bs) { > > + NS_WARNING("Failed to get bluetooth service manager!"); > > + } else if(NS_FAILED(bs->UnregisterBluetoothEventHandler(mPath, this))) { > > Seems odd that you have to pass mPath here. Couldn't you take care of this > with just the 'this' pointer? If we don't pass the path, we don't know which bucket of the hash table to remove from. This function only takes an observer object so it can't query for the path itself. > @@ +90,5 @@ > > + if(!bs) { > > + NS_WARNING("Failed to get bluetooth service manager!"); > > + return NS_ERROR_FAILURE; > > + } > > + bs->StartDiscoveryInternal(mPath); > > This isn't ok, it's a synchronous call. Also, it could fail, and you're not > checking here. > > We need to redesign this to return a DOMRequest. Ok, I started thinking about this on the e10s stuff I've been doing, was wondering if it was going to need to happen easier, and apparently it is. I'll start backporting what I've got there into this patch. > @@ +195,5 @@ > > +NS_IMETHODIMP > > +BluetoothAdapter::GetRemoteDevice(const nsAString& aAddress, > > + nsIDOMBluetoothDevice** aDevice) > > +{ > > + nsRefPtr<BluetoothDevice> device = BluetoothDevice::Create(aAddress); > > You're not really sanitizing aAddress at all here... Should you? Can you at > least NS_ENSURE that it's non-empty? Or that the address belongs to this > adapter somehow? We can test against our device array that we carry, but there's a good question of what a device with no properties filled in really gets us. This might turn into a DOMRequest much like GetDefaultAdapter, where we get the address and properties. > @@ +54,2 @@ > > private: > > + void SetPropertyByVariant(const BluetoothNamedVariant& aValue); > > Don't you need to forward declare this? And I'd add an extra line after this > method before the members. We pull the BluetoothNamedVariant type from BluetoothCommon. Probably not a good idea, but that's getting overhauled in the e10s stuff when we switch to unions anyways. > ::: dom/bluetooth/BluetoothCommon.h > @@ +55,2 @@ > > */ > > struct BluetoothEvent > > I think this name is misleading... It's not an event, like a DOM event. It's > more like "BluetoothEventData" or something. Let's put our heads together > and figure out a better name. I used event because it is generated on the IOThread, but yeah, event is way overloaded as is. I'll see what I can figure out. > > +BluetoothDevice::BluetoothDevice(const BluetoothEvent& aEvent) > > So what about mAddress for this constructor? > In general having more than one constructor is a little confusing ("Which > one am I supposed to call? What's the difference between the two?"). Can we > fix this so that there's only one? We assume that the address comes along in the aEvent object, but yeah, I can probably standardize that down. In fact, I should probably have the ::Create()'s take a BluetoothEvent and do the settings there so we can do verification that we've gotten an Address property, and just leave in private null param'd constructors > EqualsLiteral for all these. Damn that's handy. :| > @@ +179,5 @@ > > } > > > > BluetoothManager::~BluetoothManager() > > { > > + BluetoothService* es = GetBluetoothService(); > > "es"? Holdout from when I had a BluetoothEventService and BluetoothPlatformService. Will change. > @@ +22,5 @@ > > + if(!sTriedService) { > > + sTriedService = true; > > +#ifdef MOZ_B2G_BT > > + if(!sBluetoothService) { > > + sBluetoothService = new BluetoothDBusService(); > > Wait... who is responsible for deleting this on shutdown? So I wasn't really sure how to shut down the service and forgot to resolve that before submitting. Should I put a xpcom-shutdown observer on it, or just expose a function for shutdown so SystemWorkerManager can do it? > This calls dbus_bus_get. According to the docs I saw > (http://dbus.freedesktop.org/doc/api/html/group__DBusBus.html), "If > returning a newly-created connection, this function will block until > authentication and bus registration are complete." The StartDBus() call in > SystemWorkerManager kicks off an async process to create this connection so > I think this means that we will block here. We can't call this on the main > thread. > @@ +313,5 @@ > > + > > + EstablishDBusConnection(); > > + > > + // Add a filter for all incoming messages_base > > + if (!dbus_connection_add_filter(mConnection, EventFilter, > > It's unclear whether this blocks or not. The docs don't say, and I would > assume ("ass-u-me", etc.) that it doesn't, but we should find out. In > general I think we should make a rule that all dbus calls happen on a > non-main thread unless they're guaranteed to be threadsafe and non-blocking. Well, all of the dbus calls are clamped down to the BluetoothService file, so we should be able to case things up in tasks to throw at the IOThread I guess. Since everything will be async anyways, we'll just throw tasks back and forht between the IOThread and the main thread? Worried it's gonna more complicated than it already is. > @@ +355,5 @@ > > + dbus_message_append_args(msg, DBUS_TYPE_INVALID); > > + dbus_error_init(&err); > > + > > + // TODO: This should not block > > + reply = dbus_connection_send_with_reply_and_block( > > Really, it shouldn't, and I'm not very comfortable doing this now. Also, > we're in a loop of 1000 tries! Agh, yeah, this needs to be changed badly. I brought it in from the JNI which is (I believe) running this out on a thread, didn't realize the loop didn't get removed, and this needs to be called async anyways. > Need to chat with cjones to see if we're ok with short-term blocking. I > would say no, but I'm happy to let him make the final call. I'm pretty sure he would definitely not like this. :)

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #11) > So I wasn't really sure how to shut down the service and forgot to resolve > that before submitting. Should I put a xpcom-shutdown observer on it, or > just expose a function for shutdown so SystemWorkerManager can do it? Well, as far as I can tell bluetooth really has nothing to do with the SystemWorkerManager... That service exists to glue JS-y state machine workers like RIL and netman to their C++ pieces. Why is bluetooth piggy-backing there anyway? If we decide to leave it tethered to SystemWorkerManager then I guess exposing a shutdown function for it to call is better than adding a new xpcom observer. But in the future we're probably going to want to have system-wide bluetooth notifications so that various components can clean up, right? The code as written now would just call 'delete' on the one and only BluetoothDBusService, and then every other thing still trying to use bluetooth would just start failing when it can't access it any more. I think we need to come up with a nice shutdown sequence.

(In reply to ben turner [:bent] from comment #12) > Well, as far as I can tell bluetooth really has nothing to do with the > SystemWorkerManager... That service exists to glue JS-y state machine > workers like RIL and netman to their C++ pieces. Why is bluetooth > piggy-backing there anyway? Because that's where the other 2 radios come up on boot, mostly, so it seemed like a good place to put it at the time, especially when it was going to just be hiding behind settings. > If we decide to leave it tethered to SystemWorkerManager then I guess > exposing a shutdown function for it to call is better than adding a new > xpcom observer. Well, assuming we untie it, where should it go? > But in the future we're probably going to want to have system-wide bluetooth > notifications so that various components can clean up, right? The code as > written now would just call 'delete' on the one and only > BluetoothDBusService, and then every other thing still trying to use > bluetooth would just start failing when it can't access it any more. I think > we need to come up with a nice shutdown sequence. This is probably more of a sooner than later thing actually, since we'll have a system-wide shutdown any time we put the phone in airplane mode. I'll go ahead and file this as a different bug, but we can talk about it tomorrow and start planning for it as part of this.

Ok, first round of nit-fixes done, a few things to point out. > > endif > > + > > +# Add VPATH to LOCAL_INCLUDES so we are going to include the correct backend > > +# subdirectory (and the ipc one). > > +LOCAL_INCLUDES += $(VPATH:%=-I%) > > I don't see what this does for you. Revert? That brings in any platform specific directory (like linux) as an include path. Just saves repeating more lines > > + > > +class BluetoothDevice; > > + > > +class BluetoothDeviceEvent : public nsDOMEvent > > + , public nsIDOMBluetoothDeviceEvent > > Nit: , on prev line I do this a lot elsewhere, and style guide says otherwise? https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2B.2B_classes > > + MOZ_ASSERT(NS_IsMainThread()); > > + BluetoothEventObserverList *ol; > > + if (!mBluetoothEventObserverTable.Get(aNodeName, &ol)) { > > + mBluetoothEventObserverTable.Put(aNodeName, > > + new BluetoothEventObserverList()); > > This isn't ok, you're not calling Init on the new hash table. Also, your indent > is off. It's not a hash table, it's an ObserverList. There's only one hash table, going from nsString of the node path to the various ObserverLists. That's init'd earlier (which you commented on since I went overboard on the pre-init) and I don't think ObserverLists require Initing? > > + return NS_ERROR_FAILURE; > > + } > > + mBluetoothEventObserverTable.Get(aNodeName, &ol); > > + ol->RemoveObserver(aHandler); > > + if (ol->Length() == 0) { > > I generally prefer IsEmpty() if that's what you actually care about. Same thing, it's an ObserverList, not a regular List, so it only has Length. Should I add an IsEmpty()? Seems like it could be useful.

(In reply to ben turner [:bent] from comment #12) > If we decide to leave it tethered to SystemWorkerManager then I guess > exposing a shutdown function for it to call is better than adding a new > xpcom observer. Ok, answering my own question from a couple of replies ago: I can make the thread start/stop on SetEnabled requests to the BluetoothManager object. Even when these objects die, they'll still leave the service and DBusThread up in the background until either another BluetoothManager object is brought up and SetEnabled is called with false as the argument, or until they get a xpcom-shutdown observation notice. That means we can keep all of the bluetooth calls inside bluetooth, the thread only comes up when it's requested, and that's already a DOMRequest so it's async anyways. This also means I can create another service that will deal with Platform init/shutdown, to get the Android specific code out of BluetoothManager.cpp (Even though we wrap the checks in a dlload to look for a certain dynamic libs existence already).

Moved firmware loading out to service, took initial dbus connection off main thread, removed bluetooth calls from SystemWorkerManager

Fixes to comments from last review, async's all dbus calls.

v7 wasn't building for B2G phones due to BluetoothGonkFirmware compilation issues. Also updated for new LazyIdleThread constructor.

Comment on attachment 632860 [details] [diff] [review] v8: Fire DeviceFound event whenever a remote Bluetooth device is discovered One more review cancellation, as new version with IPDL'ized types will be up tomorrow morning, and will hopefully be last version.

IPDL'ized types for easier transition to e10s, not to mention IPDL unions match well to the data we're getting from DBus. Adding cjones for feedback just in case bent can't get to this soon, mainly to make sure this doesn't go like the last review round. Doing lots of new things here I'm not quite sure about.

Last version of patch had some files that weren't saved correctly.

Comment on attachment 633233 [details] [diff] [review] v10: Fire DeviceFound event whenever a remote Bluetooth device is discovered This looks ok on the whole, but I'm too far away from the details here to provide useful feedback. I think we'd need to sit down together to really cover the details. Ben/Blake have the obvious advantage over me there. As a general note, these kinds of large patches tend to be easier to review when split approximately as - refactorings first - then interfaces - then implementions - finally, build goop or whatever's needed to turn everything on >diff --git a/dom/bluetooth/BluetoothAdapter.cpp b/dom/bluetooth/BluetoothAdapter.cpp >+NS_IMETHODIMP >+BluetoothAdapter::GetDevices(JSContext* aCx, jsval* aDevices) >+{ >+ aDevices = nsnull; For 5¢, spot the error in this statement. >diff --git a/dom/bluetooth/BluetoothDevice.cpp b/dom/bluetooth/BluetoothDevice.cpp >+BluetoothDevice::BluetoothDevice(const BluetoothSignal& aSignal) >+{ >+ for (uint32_t i = 0; i < aSignal.value().get_ArrayOfBluetoothNamedValue().Length(); ++i) { >+ SetPropertyByValue(aSignal.value().get_ArrayOfBluetoothNamedValue()[i]); >+ } Yuck. const InfallibleTArray<BluetoothNamedValue>& values = aSignal.value().get_ArrayOfBluetoothNamedValue() for (uint32_t i = 0; i < values.Length(); ++i) { SetPropertyByValue(values[i]); } >+void >+BluetoothDevice::SetPropertyByValue(const BluetoothNamedValue& aValue) >+{ const nsString& name = aValue.name(); const BluetoothValue& value = aValue.value(); >diff --git a/dom/bluetooth/BluetoothService.cpp b/dom/bluetooth/BluetoothService.cpp >+nsresult >+BluetoothService::Start(nsCOMPtr<nsIRunnable> aResultRunnable) >+{ >+#if defined(MOZ_WIDGET_GONK) >+ sBluetoothService = new BluetoothGonkService(); >+#elif defined(MOZ_B2G_BT) >+ sBluetoothService = new BluetoothDBusService(); A better trick than this is to make a (protected) static BluetoothService "factory" Create() function that returns already_Addrefed<BluetoothService>, but leave it undefined. Then in each platform-specific implementation, define Create(). That avoids the ifdef'ery here. >diff --git a/dom/bluetooth/BluetoothService.h b/dom/bluetooth/BluetoothService.h >+ static nsresult Start(nsCOMPtr<nsIRunnable> aResultRunnable); The COM convention we've borrowed for XPCOM is Foo(nsIRunnable* aBar); where |aBar| is guaranteed to have ref when passed Foo(). That is, Foo() is allowed to temporarily borrow the ref to aBar. This cuts down on what would otherwise be a massive amount of ref/unref churn. It's not all that big of a deal here, but a good habit to be in. >+ static nsresult Get(BluetoothService** aService); This is the COM syntactic convention for outparams, which is a little confusing because you're not following COM semantics, which is to ref the return value. It looks to a COM-y like you're leaking at all callsites like BluetoothService* s; Get(&s); Handing out raw pointers from here to DOM-y stuff makes me pretty nervous. What might be nicer is already_AddRefed<BluetoothService> Get(); which lets you write code like nsRefPtr<BluetoothService> s = Get(); And for here and all the other BluetoothService functions, you're using the XPCOM style for something that's pure C++. You don't need to return nsresult if you don't want to. >diff --git a/dom/bluetooth/gonk/BluetoothGonkService.cpp b/dom/bluetooth/gonk/BluetoothGonkService.cpp >+nsresult >+StartStopGonkBluetooth(bool aShouldEnable) Make translation-unit-static functions |static| plz. >diff --git a/ipc/ipdl/Makefile.in b/ipc/ipdl/Makefile.in > IPDLDIRS = \ > uriloader/exthandler \ > dom/indexedDB/ipc \ >+ dom/bluetooth/ipc \ A heathen tab character! My feedback+ is pretty useless here, but you're welcome to it.

Taking some of the things from cjones's feedback into account, as well as the FAR more important issue that BluetoothReplyRunnables were not always getting run in error cases, meaning we'd have DOMRequests that just sort of fall off into nowhere. Extracted BluetoothReplyRunnables to its own class, now it explicitly deals with runnable returns, which leaves us with less code in the DOM classes again. Patch still grows another 7k because I created new factory classes to get the ifdefs out of BluetoothService. But hopefully this is it. Really seriously hopefully.

Comment on attachment 633842 [details] [diff] [review] v11: Fire DeviceFound event whenever a remote Bluetooth device is discovered Review of attachment 633842 [details] [diff] [review]: ----------------------------------------------------------------- A bunch of little things below may mask the bigger issues, so those are: 1. The startup sequence and service lifetime are a little unconventional, plus a bit racy. We can chat about how to fix that. 2. In a lot of places you have args that are nsRefPtr<T> and nsCOMPtr<T>. This is almost never what you want as it will construct temporary objects, add wasted AddRef/Release calls, and confuse leak log tools. The Gecko (really, COM) convention is that all inparams must have a reference before the call is made and then the arg type is a raw interface pointer (T*). 3. Some kind of blocking problem on shutdown where we might or might not be calling dbus on the main thread. 4. Confusion over error *names* vs. *messages*. The style is mostly right, though I've found a few exceptions. No one is going to die if they aren't all fixed but let's try to make it as pretty as possible given our deadline. ::: dom/bluetooth/BluetoothAdapter.cpp @@ +72,5 @@ > + return true; > + } > + > +private: > + nsRefPtr<BluetoothAdapter> mAdapterPtr; So if the task fails you'll never get a ParseSuccessfulReply callback. And if you don't you'll leave mAdapterPtr alive until the destructor, which means you'll be racing with the dbus thread. If you lose the race the CC will abort the process :) Need to have a guaranteed place to release these types of things to avoid this problem. @@ +117,5 @@ > + //mDeviceAddresses = value.get_ArrayOfnsString(); > + } else if (name.EqualsLiteral("UUIDs")) { > + mUuids = value.get_ArrayOfnsString(); > + } else { > + //NS_WARNING("Adapter property sent but not set"); Probably want NS_NOTREACHED? @@ +122,5 @@ > + } > +} > + > +nsresult > +BluetoothAdapter::CreateNewDOMRequest(nsIDOMDOMRequest** aRequest) This could just be: already_AddRefed<nsIDOMDOMRequest> BluetoothAdapter::CreateNewDOMRequest() @@ +146,5 @@ > + NS_ASSERTION(!aPath.IsEmpty(), "Adapter created with empty path!"); > + > + BluetoothService* bs; > + if (NS_FAILED(BluetoothService::Get(&bs))) { > + NS_WARNING("Failed to get bluetooth service manager!"); Race here too. @@ +147,5 @@ > + > + BluetoothService* bs; > + if (NS_FAILED(BluetoothService::Get(&bs))) { > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NULL; Nit: nsnull in DOM-land @@ +161,5 @@ > > +void > +BluetoothAdapter::Notify(const BluetoothSignal& aData) > +{ > + if (aData.name().EqualsLiteral("DeviceFound")) { And if it isn't this one? Warn? Error? @@ +177,5 @@ > + NS_WARNING("Failed to get bluetooth service manager!"); > + return NS_ERROR_FAILURE; > + } > + > + if(NS_FAILED(CreateNewDOMRequest(aRequest))) { In general I don't like setting outparams until im going to return a success code. If the caller is being cute and does this, for instance: nsCOMPtr<nsIDOMDOMRequest> request; adapter->StartStopDiscovery(true, getter_AddRefs(request)); if (request) { // I won! Except no. } Best to wait until the end. @@ +265,5 @@ > + > +NS_IMETHODIMP > +BluetoothAdapter::GetDevices(JSContext* aCx, jsval* aDevices) > +{ > + // TODO: Implement device fetching NS_WARNING in the meantime ::: dom/bluetooth/BluetoothAdapter.h @@ +78,5 @@ > + PRUint32 mDiscoverableTimeout; > + PRUint32 mClass; > + nsTArray<nsString> mDeviceAddresses; > + nsTArray<nsString> mUuids; > + Nit: extra line ::: dom/bluetooth/BluetoothCommon.h @@ +9,4 @@ > > #include "nsString.h" > #include "nsTArray.h" > +#include "nsThreadUtils.h" Is this needed in *every* file? Seems much better to move this into the one or two places you use it. ::: dom/bluetooth/BluetoothDevice.cpp @@ +36,5 @@ > + > +BluetoothDevice::BluetoothDevice(const BluetoothSignal& aSignal) > +{ > + const InfallibleTArray<BluetoothNamedValue>& values = > + aSignal.value().get_ArrayOfBluetoothNamedValue(); Are you always assured that this will be an array? Couldn't be just a single BluetoothNamedValue? @@ +60,5 @@ > + mPaired = value.get_bool(); > + } else if (name.EqualsLiteral("UUIDs")) { > + mUuids = value.get_ArrayOfnsString(); > + } else { > + //NS_WARNING("Device property sent but not set"); If we really don't know all the types that we could see then this warning should exist. If we do know and we have one that isn't handled we should assert. @@ +64,5 @@ > + //NS_WARNING("Device property sent but not set"); > + } > +} > + > +//static Nit: space after // @@ +97,5 @@ > + > +NS_IMETHODIMP > +BluetoothDevice::GetPaired(bool* aPaired) > +{ > + // TODO: Implement device pairing Make this an NS_WARNING for now? @@ +105,5 @@ > + > +NS_IMETHODIMP > +BluetoothDevice::GetConnected(bool* aConnected) > +{ > + // TODO: Implement device connection Here too. ::: dom/bluetooth/BluetoothDevice.h @@ +30,5 @@ > + nsDOMEventTargetHelper) > + NS_DECL_EVENT_HANDLER(propertychanged) > + > + static already_AddRefed<BluetoothDevice> > + Create(nsPIDOMWindow* aOwner, const BluetoothSignal& aSignal); Nit: indent unnecessary here ::: dom/bluetooth/BluetoothManager.cpp @@ +54,4 @@ > { > +public: > + GetAdapterTask(BluetoothManager* aManager, > + nsCOMPtr<nsIDOMDOMRequest> aReq) : BluetoothReplyRunnable(aReq), Nit: the initializer list should be on the next line and back over on the left @@ +65,5 @@ > + nsCOMPtr<nsIDOMBluetoothAdapter> adapter; > + nsString& path = mReply->get_BluetoothReplySuccess().value().get_nsString(); > + adapter = BluetoothAdapter::Create(mManagerPtr->GetOwner(), > + path); > + NS_ADDREF(adapter); Why are you adding a ref here? @@ +79,5 @@ > + sc->GetNativeGlobal(), > + adapter, > + &aValue); > + bool result = NS_SUCCEEDED(rv) ? true : false; > + if(!result) { Nit: space here. @@ +85,5 @@ > } > > + //mManagerPtr must be null before returning to prevent the background > + //thread from racing to release it during the destruction of this runnable. > + mManagerPtr = nsnull; This won't work if you failed above... @@ +108,5 @@ > + NS_IMETHOD Run() > + { > + MOZ_ASSERT(NS_IsMainThread()); > + bool result = true; > + BluetoothService* bs = nsnull; You have a C background, don't you ;) @@ +240,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsRefPtr<BluetoothReplyRunnable> results = new GetAdapterTask(this, request); > + > + bs->GetDefaultAdapterPathInternal(results); This can fail, and it's better to throw here than to get an error event later. @@ +253,5 @@ > nsRefPtr<BluetoothManager> manager = new BluetoothManager(aWindow); > + BluetoothService* bs; > + > + // Can fail if we've not yet enabled bluetooth > + if (NS_SUCCEEDED(BluetoothService::Get(&bs))) { This is basically a race, right? We can fix this, but right now you'd be handing out a useless object. @@ +284,5 @@ > return NS_OK; > } > > +void > +BluetoothManager::Notify(const BluetoothSignal& aData) Hm, does this just need to die? ::: dom/bluetooth/BluetoothManager.h @@ +13,2 @@ > #include "mozilla/Observer.h" > +#include "mozilla/RefPtr.h" Do you need this? ::: dom/bluetooth/BluetoothReplyRunnable.cpp @@ +13,5 @@ > +USING_BLUETOOTH_NAMESPACE > + > +BluetoothReplyRunnable::BluetoothReplyRunnable(nsCOMPtr<nsIDOMDOMRequest> aReq) > +{ > + mDOMRequest.swap(aReq); You're swapping after you already copied... @@ +31,5 @@ > + DebugOnly<nsresult> rv = > + mReply->type() == BluetoothReply::TBluetoothReplySuccess ? > + rs->FireSuccess(mDOMRequest, aVal) : > + rs->FireError(mDOMRequest, mReply->get_BluetoothReplyError().error()); > + mDOMRequest = nsnull; Let's stick this in Run so you don't have to duplicate in FireErrorString. @@ +47,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + DebugOnly<nsresult> rv = > + rs->FireError(mDOMRequest, mErrorString); This doesn't do what you think. The string you pass here is supposed to be the *name* of the error. Things like "InvalidStateError" and "TypeError", etc. It's not for an error message (e.g. "Can't create script context!"). @@ +48,5 @@ > + } > + > + DebugOnly<nsresult> rv = > + rs->FireError(mDOMRequest, mErrorString); > + mDOMRequest = nsnull; Remove. @@ +53,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +BluetoothReplyRunnable::Run() This and all other methods here could use some "correct thread" assertions. @@ +57,5 @@ > +BluetoothReplyRunnable::Run() > +{ > + // We have to explicitly AddRef before passing this into the void* for > + // DBus's callback, so we also have to explicity deref > + Release(); I think it would be much nicer to do this release in the same code that added the reference. @@ +59,5 @@ > + // We have to explicitly AddRef before passing this into the void* for > + // DBus's callback, so we also have to explicity deref > + Release(); > + // We could very well not even have a reply yet > + if(!mErrorString.IsEmpty()) { Nit: space before ( @@ +63,5 @@ > + if(!mErrorString.IsEmpty()) { > + return FireErrorString(); > + } > + if(mReply->type() != BluetoothReply::TBluetoothReplySuccess) { > + return FireReply(JSVAL_VOID); Hm. What's the difference between having an error string and not having a success reply? I'm not understanding why there are three options here. Maybe after we stop trying to fake an error for failed async calls this will go away? @@ +65,5 @@ > + } > + if(mReply->type() != BluetoothReply::TBluetoothReplySuccess) { > + return FireReply(JSVAL_VOID); > + } > + jsval v = JSVAL_VOID; Nit: initializing is unnecessary @@ +66,5 @@ > + if(mReply->type() != BluetoothReply::TBluetoothReplySuccess) { > + return FireReply(JSVAL_VOID); > + } > + jsval v = JSVAL_VOID; > + if(!ParseSuccessfulReply(v)) { Nit: space before ( @@ +75,5 @@ > + > +bool > +BluetoothReplyRunnable::ParseSuccessfulReply(jsval& aValue) > +{ > + return true; This is not what you really want... I think this method should be pure virtual. ::: dom/bluetooth/BluetoothService.h @@ +26,5 @@ > + NS_DECL_NSIOBSERVER > + > + BluetoothService(); > + > + virtual ~BluetoothService(); Nit: This should be protected @@ +102,5 @@ > + * > + * @return NS_OK on proper assignment, NS_ERROR_FAILURE otherwise (if service > + * has not yet been started, for instance) > + */ > + static nsresult Get(BluetoothService** aService); We should talk about this startup sequence a little. I see what you've done here, and it's ok, but it's quite different from the way most services work in Gecko. I think it would be simpler if we stick with the normal model. Basically you'd always return a service pointer here, but it wouldn't always have turned the bluetooth stuff on yet. Then you call Start on it, passing your callback runnable. Make sense? ::: dom/bluetooth/BluetoothUtils.h @@ +30,5 @@ > * @return NS_OK on successful addition to observer, NS_ERROR_FAILED > * otherwise > */ > +nsresult RegisterBluetoothSignalHandler(const nsCString& aNodeName, > + BluetoothSignalObserver *aMsgHandler); Nit: You goofed your indent, here and below. @@ +72,4 @@ > */ > nsresult StopBluetoothConnection(); > > + Nit: Revert. ::: dom/bluetooth/Makefile.in @@ +22,5 @@ > BluetoothManager.cpp \ > BluetoothAdapter.cpp \ > + BluetoothDevice.cpp \ > + BluetoothDeviceEvent.cpp \ > + BluetoothReplyRunnable.cpp \ Tab! @@ +35,3 @@ > $(NULL) > > +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT)) We should get khuey or ted to look over this, see what they want here. ::: dom/bluetooth/gonk/BluetoothGonkFirmware.cpp @@ +78,5 @@ > +{ > + return sBluedroidFunctions.bt_disable(); > +} > + > + Nit: nuke extra line ::: dom/bluetooth/gonk/BluetoothGonkFirmware.h @@ +13,5 @@ > + > +bool EnsureBluetoothInit(); > +int IsBluetoothEnabled(); > +int EnableBluetooth(); > +int DisableBluetooth(); Any reason these live in a separate file? Why not just put them into BluetoothGonkService? ::: dom/bluetooth/gonk/BluetoothGonkService.cpp @@ +61,5 @@ > + > +nsresult > +BluetoothGonkService::StopInternal() > +{ > + // This may be called from the main thread on shutdown, so no check That sounds bad... We should make this kind of thing consistent. And we should not be calling dbus stuff on the main thread :) ::: dom/bluetooth/ipc/BluetoothTypes.ipdlh @@ +7,5 @@ > +namespace mozilla { > +namespace dom { > +namespace bluetooth { > + > +//struct BluetoothNamedValue; Nit: Nuke. @@ +12,5 @@ > + > +/** > + * Value structure for returns from bluetooth. Currently modeled after dbus > + * returns, which can be a 32-bit int, an UTF8 string, a bool, or an array of > + * UTF8 strings. Can also hold key-value pairs for dictionary-ish access. Hm. This union uses UTF16 strings, which makes this comment confusing. @@ +21,5 @@ > + uint32_t; > + nsString; > + bool; > + nsString[]; > + BluetoothNamedValue[]; This confuses me a little... I understand that it works, but it muddies the water a little (a value that could be a name+value, which could be a name+value, which could be a name+value...). Can't we just get rid of this? Then you could always have a BluetoothNamedValue[] as the member of BluetoothReplySuccess, or, if you didn't like that, you could turn BluetoothReplySuccess into a union that had both BluetoothNamedValue and BluetoothNamedValue[]. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +115,5 @@ > +}; > + > +class DistributeBluetoothSignalTask : public nsRunnable { > +public: > + BluetoothSignal mSignal; Why is this public? @@ +117,5 @@ > +class DistributeBluetoothSignalTask : public nsRunnable { > +public: > + BluetoothSignal mSignal; > + > + DistributeBluetoothSignalTask(const BluetoothSignal& aSignal) : mSignal(aSignal) Nit: initializer on its own line. @@ +135,5 @@ > + } > +}; > + > + > +bool CheckDBusMessageForError(DBusMessage* aMsg, nsString& aError) { Would it make more sense if we called this "IsErrorMessage" or "IsDBusMessageError"? Nit: bool and { on their own lines @@ +142,5 @@ > + 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) { Nit: operators always go on previous line @@ +156,5 @@ > + } > + return false; > +} > + > +void GetObjectPathCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable) Nit: void on its own line @@ +164,5 @@ > + BluetoothReplyRunnable* replyRunnable = > + static_cast< BluetoothReplyRunnable* >(aBluetoothReplyRunnable); > + if (!replyRunnable) { > + NS_WARNING("BluetoothReplyRunnable is null!"); > + return; This should just be asserted @@ +173,5 @@ > + > + nsTArray<BluetoothNamedValue> replyValues; > + > + if (!CheckDBusMessageForError(aMsg, replyError)) { > + if (dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN) { What if it isn't? Is this assertable? @@ +177,5 @@ > + if (dbus_message_get_type(aMsg) == DBUS_MESSAGE_TYPE_METHOD_RETURN) { > + const char* object_path; > + if (!dbus_message_get_args(aMsg, &err, DBUS_TYPE_OBJECT_PATH, > + &object_path, DBUS_TYPE_INVALID) > + || !object_path) { Nit: operator on prev line @@ +178,5 @@ > + 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)) { Can we reuse CheckDBusMessageForError here? @@ +188,5 @@ > + } > + } > + } > + > + // Reply will be deleted by the runnable after running on main thread This block of code seems repeated in multiple places, can we consolidate somehow? @@ +204,5 @@ > + NS_WARNING("Failed to dispatch to main thread!"); > + } > +} > + > +void GetVoidCallback(DBusMessage* aMsg, void* aBluetoothReplyRunnable) Nit: void on its own line @@ +212,5 @@ > + BluetoothReplyRunnable* replyRunnable = > + static_cast< BluetoothReplyRunnable* >(aBluetoothReplyRunnable); > + if (!replyRunnable) { > + NS_WARNING("BluetoothReplyRunnable is null!"); > + return; Assert only @@ +220,5 @@ > + BluetoothNamedValue v; > + if (!CheckDBusMessageForError(aMsg, replyError) && > + 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)) { Reuse CheckDBusMessageForError? @@ +244,5 @@ > +} > + > +bool > +GetProperty(DBusMessageIter aIter, Properties* aPropertyTypes, > + int aMaxProperties, int* aPropIndex, Can you rename aMaxProperties to aPropertyTypesLen or something? @@ +245,5 @@ > + > +bool > +GetProperty(DBusMessageIter aIter, Properties* aPropertyTypes, > + int aMaxProperties, int* aPropIndex, > + InfallibleTArray<BluetoothNamedValue>& aProperties) { Nit: { on its own line @@ +252,5 @@ > + uint32_t array_type; > + int i, type; > + > + if (dbus_message_iter_get_arg_type(&aIter) != DBUS_TYPE_STRING) > + return false; Nit: please add braces to single-line if statements @@ +257,5 @@ > + dbus_message_iter_get_basic(&aIter, &property); > + if (!dbus_message_iter_next(&aIter) || > + dbus_message_iter_get_arg_type(&aIter) != DBUS_TYPE_VARIANT) > + return false; > + for (i = 0; i < aMaxProperties; i++) { Nit: extra space here @@ +265,5 @@ > + nsString propertyName; > + propertyName.AssignASCII(aPropertyTypes[i].name); > + *aPropIndex = i; > + if (i == aMaxProperties) > + return false; You should move this up above the propertyName assignment, otherwise you're reading garbage memory. @@ +272,5 @@ > + type = aPropertyTypes[*aPropIndex].type; > + if (dbus_message_iter_get_arg_type(&prop_val) != type) { > + LOG("Property type mismatch in GetProperty: %d, expected:%d, index:%d", > + dbus_message_iter_get_arg_type(&prop_val), type, *aPropIndex); > + return false; This should probably be an assertion @@ +276,5 @@ > + return false; > + } > + > + BluetoothValue propertyValue; > + switch(type) { Nit: space before ( @@ +277,5 @@ > + } > + > + BluetoothValue propertyValue; > + switch(type) { > + case DBUS_TYPE_STRING: Nit: please indent cases (https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_structures) @@ +298,5 @@ > + case DBUS_TYPE_ARRAY: > + dbus_message_iter_recurse(&prop_val, &array_val_iter); > + array_type = dbus_message_iter_get_arg_type(&array_val_iter); > + if (array_type == DBUS_TYPE_OBJECT_PATH || > + array_type == DBUS_TYPE_STRING){ What if it's not? Assert? Warn? Can we be more robust here? @@ +302,5 @@ > + array_type == DBUS_TYPE_STRING){ > + InfallibleTArray<nsString> arr; > + do { > + const char* tmp; > + dbus_message_iter_get_basic(&array_val_iter, &tmp); Hm. How do you know this is going to always be a string? @@ +311,5 @@ > + propertyValue = arr; > + } > + break; > + default: > + return false; NS_NOTREACHED here @@ +321,5 @@ > +void > +ParseProperties(DBusMessageIter* aIter, > + Properties* aPropertyTypes, > + const int aMaxProperties, > + InfallibleTArray<BluetoothNamedValue>& aProperties) { Nit: { on its own line @@ +327,5 @@ > + int prop_index = -1; > + > + if (dbus_message_iter_get_arg_type(aIter) != DBUS_TYPE_ARRAY) { > + NS_WARNING("Trying to parse a property from something that's not an array!"); > + return; Can we assert this instead? @@ +334,5 @@ > + > + do { > + if (dbus_message_iter_get_arg_type(&dict) != DBUS_TYPE_DICT_ENTRY) { > + NS_WARNING("Trying to parse a property from something that's not an dict!"); > + return; Here too? @@ +398,5 @@ > + > + signalName = NS_ConvertUTF8toUTF16(dbus_message_get_member(aMsg)); > + BluetoothValue v; > + > + if (dbus_message_is_signal(aMsg, "org.bluez.Adapter", "DeviceFound")) { Seems like you don't make much use of the #define above to get this string value. I like #define better than string constants usually because you get a compile error instead of a runtime error when you have a typo. @@ +402,5 @@ > + if (dbus_message_is_signal(aMsg, "org.bluez.Adapter", "DeviceFound")) { > + > + DBusMessageIter iter; > + > + if (dbus_message_iter_init(aMsg, &iter)) { Let's make sure we issue warnings if any of these checks fail. @@ +405,5 @@ > + > + if (dbus_message_iter_init(aMsg, &iter)) { > + InfallibleTArray<BluetoothNamedValue> value; > + const char* addr; > + dbus_message_iter_get_basic(&iter, &addr); How do you know this is a string? Can we at least assert this? @@ +407,5 @@ > + InfallibleTArray<BluetoothNamedValue> value; > + const char* addr; > + dbus_message_iter_get_basic(&iter, &addr); > + value.AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Address"), > + NS_ConvertUTF8toUTF16(addr))); Nit: indent is off @@ +411,5 @@ > + NS_ConvertUTF8toUTF16(addr))); > + > + if (dbus_message_iter_next(&iter)) { > + ParseProperties(&iter, > + (Properties*)&remote_device_properties, I think you can just pass |remote_device_properties| there and it will decompose into Properties* automatically @@ +420,5 @@ > + } > + } else if (dbus_message_is_signal(aMsg, "org.bluez.Adapter", "DeviceDisappeared")) { > + nsString value; > + if (!dbus_message_get_args(aMsg, &err, > + DBUS_TYPE_STRING, &value, Wait, dbus doesn't know about nsString... Is this supposed to be const char*? @@ +428,5 @@ > + v = value; > + } else if (dbus_message_is_signal(aMsg, "org.bluez.Adapter", "DeviceCreated")) { > + nsString value; > + if (!dbus_message_get_args(aMsg, &err, > + DBUS_TYPE_OBJECT_PATH, &value, Here too. Not sure what DBUS_TYPE_OBJECT_PATH wants, probably char* again @@ +439,5 @@ > + ParsePropertyChange(aMsg, > + (Properties*)&adapter_properties, > + ArrayLength(adapter_properties), > + value); > + v = value; Would be weird if value was empty here, right? Let's assert. @@ +445,5 @@ > + > + BluetoothSignal signal(signalName, signalPath, v); > + > + nsRefPtr<DistributeBluetoothSignalTask> > + t(new DistributeBluetoothSignalTask(signal)); Please do nsRefPtr<T> t = new T() @@ +459,5 @@ > +{ > + // This could block. It should never be run on the main thread. > + MOZ_ASSERT(!NS_IsMainThread()); > + > + StartDBus(); This can fail, right? @@ +462,5 @@ > + > + StartDBus(); > + > + if (mConnection) { > + NS_WARNING("DBusConnection already established, skipping"); Doesn't seem worth warning about? @@ +466,5 @@ > + NS_WARNING("DBusConnection already established, skipping"); > + return NS_OK; > + } > + > + EstablishDBusConnection(); This can fail too. @@ +482,5 @@ > +nsresult > +BluetoothDBusService::StopInternal() > +{ > + // While this most likely shouldn't be run on the main thread, it's best to do > + // so during shutdown, so we don't check here. This asymmetry is worrisome. @@ +490,5 @@ > + } > + dbus_connection_remove_filter(mConnection, EventFilter, NULL); > + mConnection = NULL; > + mBluetoothSignalObserverTable.Clear(); > + StopDBus(); Wait, this definitely blocks. We don't want this running on the main thread. @@ +500,5 @@ > +{ > + NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!"); > + NS_ADDREF(aRunnable); > + if (!dbus_func_args_async(mConnection, > + 1000, I don't really have any idea, but isn't 1 second a little quick? Seems possible that we'd want a longer timeout. @@ +507,5 @@ > + "/", > + "org.bluez.Manager", > + "DefaultAdapter", > + DBUS_TYPE_INVALID)) { > + aRunnable->SetError(NS_LITERAL_STRING("Could not start async function!")); If this fails then you are going to return an error code immediately (which becomes a JS exception), so the page will never be able to get a request. That makes this just needless churn. @@ +544,5 @@ > +nsresult > +BluetoothDBusService::StopDiscoveryInternal(const nsAString& aAdapterPath, > + nsRefPtr<BluetoothReplyRunnable> aRunnable) > +{ > + return SendDiscoveryMessage(aAdapterPath, "StopDiscovery", aRunnable); These could be moved to the header, right? ::: dom/bluetooth/linux/BluetoothDBusService.h @@ +16,5 @@ > +BEGIN_BLUETOOTH_NAMESPACE > + > +/** > + * BluetoothService functions are used to dispatch messages to Bluetooth DOM > + * objects on the main thread, as well as provide platform indenpendent access Nit: "indenpendent" is misspelled. @@ +29,5 @@ > +{ > +public: > + /** > + * Set up variables and start the platform specific connection. Must > + * be called from main thread. The code actually asserts that it is *not* on the main thread. ::: dom/bluetooth/nsIDOMBluetoothAdapter.idl @@ +21,5 @@ > + readonly attribute jsval devices; > + > + readonly attribute DOMString name; > + readonly attribute bool discoverable; > + readonly attribute unsigned long discoverableTimeout; // Unit: sec Nit: Here and below, it's customary to put comments above the method/attribute, not off to the side. @@ +23,5 @@ > + readonly attribute DOMString name; > + readonly attribute bool discoverable; > + readonly attribute unsigned long discoverableTimeout; // Unit: sec > + > + nsIDOMDOMRequest startDiscovery(); // ret: false if bluetooth is not enabled Nit: I'd nuke the comment. It doesn't make sense given that this returns a request... ::: dom/bluetooth/nsIDOMBluetoothDevice.idl @@ +10,5 @@ > +interface nsIDOMBluetoothDevice : nsIDOMEventTarget > +{ > + readonly attribute DOMString address; > + readonly attribute DOMString name; > + readonly attribute unsigned long class; I can't remember if this will bite us... It may be that you should use the [binaryname()] annotation here to prevent conflicts... ::: ipc/dbus/DBusUtils.cpp @@ +86,5 @@ > + dbus_bool_t reply = FALSE; > + > + /* Make the call. */ > + pending = (dbus_async_call_t *)malloc(sizeof(dbus_async_call_t)); > + if (pending) { malloc is infallible. @@ +91,5 @@ > + DBusPendingCall *call; > + > + pending->user_cb = user_cb; > + pending->user = user; > + //pending->method = msg; Nit: Nuke. @@ +105,5 @@ > + } > + } > + > +done: > + if (msg) dbus_message_unref(msg); Does dbus take care of 'pending' here? @@ +140,3 @@ > done: > if (msg) dbus_message_unref(msg); > + return false; You used FALSE above. Which is it? ::: ipc/dbus/DBusUtils.h @@ +53,5 @@ > void log_and_free_dbus_error(DBusError* err, > const char* function, > DBusMessage* msg = NULL); > + > +dbus_bool_t dbus_func_send_async(DBusConnection *conn, I was gonna Nit here about * on the left, because that's what the previous one does... But hey, look at the next one! Not sure what to do here, pick one I guess. @@ +57,5 @@ > +dbus_bool_t dbus_func_send_async(DBusConnection *conn, > + DBusMessage *msg, > + int timeout_ms, > + void (*user_cb)(DBusMessage*, > + void*), Wouldn't it be nicer to have this as its own typedef?

Replying to things some of the issues, everything else can be assumed fixed. >> @@ +36,5 @@ >> + >> +BluetoothDevice::BluetoothDevice(const BluetoothSignal& aSignal) >> +{ >> + const InfallibleTArray<BluetoothNamedValue>& values = >> + aSignal.value().get_ArrayOfBluetoothNamedValue(); > >Are you always assured that this will be an array? Couldn't be just a single BluetoothNamedValue? Nope, only function that handed it a single value was removed. We assume that we'll always hand it an array now. The fact that we're handing it a signal lets us know that since values can't have single BluetoothNamedValues anyways. >> @@ +57,5 @@ >> +BluetoothReplyRunnable::Run() >> +{ >> + // We have to explicitly AddRef before passing this into the void* for >> + // DBus's callback, so we also have to explicity deref >> + Release(); > >I think it would be much nicer to do this release in the same code that added the reference. Done, but it feels weird. I have to call release in each of the BluetoothReplyRunnable subclasses now. There has to be a better way to do this. :/ >> @@ +21,5 @@ >> + uint32_t; >> + nsString; >> + bool; >> + nsString[]; >> + BluetoothNamedValue[]; > >This confuses me a little... I understand that it works, but it muddies the water a little (a value that could be a name+value, which could be a name+value, which could be a name+value...). > >Can't we just get rid of this? Then you could always have a BluetoothNamedValue[] as the member of BluetoothReplySuccess, or, if you didn't like that, you could turn BluetoothReplySuccess into a union that had both BluetoothNamedValue and BluetoothNamedValue[]. Nope. We need BluetoothValues in both BluetoothSignal (which is an unsolicited message) and BluetoothReplySuccess (which is a successful return to a function we ran async). We're usually going to get back multiple key/value pairs, so this seemed the easiest way to do it. >> @@ +178,5 @@ >> + 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)) { > >Can we reuse CheckDBusMessageForError here? No. CheckDBusMessageForError checks the message TYPE to see if the message itself is an error. This is checking whether the call to check for the argument type had an error in it. >> @@ +302,5 @@ >> + array_type == DBUS_TYPE_STRING){ >> + InfallibleTArray<nsString> arr; >> + do { >> + const char* tmp; >> + dbus_message_iter_get_basic(&array_val_iter, &tmp); > >Hm. How do you know this is going to always be a string? The lines above check for that: if (array_type == DBUS_TYPE_OBJECT_PATH || array_type == DBUS_TYPE_STRING){ >> @@ +405,5 @@ >> + >> + if (dbus_message_iter_init(aMsg, &iter)) { >> + InfallibleTArray<BluetoothNamedValue> value; >> + const char* addr; >> + dbus_message_iter_get_basic(&iter, &addr); > >How do you know this is a string? Can we at least assert this? Once again, we've already done the type check before this point. >> +{ >> + NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!"); >> + NS_ADDREF(aRunnable); >> + if (!dbus_func_args_async(mConnection, >> + 1000, > >I don't really have any idea, but isn't 1 second a little quick? Seems possible that we'd want a longer timeout. Nah, most of these will return instantiously. None of these block that long, if anything is taking > 1s, we have other, more serious issues with the bus being flooded. >> +nsresult >> +BluetoothDBusService::StopDiscoveryInternal(const nsAString& aAdapterPath, >> + nsRefPtr<BluetoothReplyRunnable> aRunnable) >> +{ >> + return SendDiscoveryMessage(aAdapterPath, "StopDiscovery", aRunnable); > >These could be moved to the header, right? It's require me to bring in a ton more headers up there due to BluetoothReplyRunnable.

Fixes issues mentioned in review. Also switches from pthread to nsThread for DBusThread, which was talked about in person after review.

Changed Release() positions for BluetoothReplyRunnable back to where they were in last review, otherwise we leak.

>>> @@ +57,5 @@ >>> +BluetoothReplyRunnable::Run() >>> +{ >>> + // We have to explicitly AddRef before passing this into the void* for >>> + // DBus's callback, so we also have to explicity deref >>> + Release(); >> >> I think it would be much nicer to do this release in the same code that added the >> reference. > > Done, but it feels weird. I have to call release in each of the > BluetoothReplyRunnable subclasses now. There has to be a better > way to do this. :/ Ok, was tired and not thinking about this when I reverted that, so it's rereverted. The reason release is there is to guarentee we actually do release the pointer. There's no way to do this where we addref, as the only time this object is available to that thread again is on off the main thread. This ensures we're doing it on the right thread at the right time, even if it is disjointed in the code.

Comment on attachment 635635 [details] [diff] [review] v13: Fire DeviceFound event whenever a remote Bluetooth device is discovered Review of attachment 635635 [details] [diff] [review]: ----------------------------------------------------------------- nsDOMClassInfoClasses.h.orig should not be checked in :) ::: dom/bluetooth/BluetoothAdapter.cpp @@ +68,5 @@ > + for (uint32_t i = 0; i < values.Length(); ++i) { > + mAdapterPtr->SetPropertyByValue(values[i]); > + } > + mAdapterPtr = nsnull; > + return true; You're not setting aValue. @@ +74,5 @@ > + > + void > + ReleaseMembers() > + { > + mAdapterPtr = nsnull; Should call base class ReleaseMembers, see below. @@ +80,5 @@ > +private: > + nsRefPtr<BluetoothAdapter> mAdapterPtr; > +}; > + > +BluetoothAdapter::BluetoothAdapter(const nsAString& aPath) : mPath(aPath) Nit: This could be inlined @@ +124,1 @@ > } What about other properties? @@ +130,5 @@ > +{ > + // Make sure we at least have a path > + NS_ASSERTION(!aPath.IsEmpty(), "Adapter created with empty path!"); > + > + BluetoothService* bs = BluetoothService::Get(); Assert bs @@ +149,5 @@ > + nsRefPtr<BluetoothDevice> d = BluetoothDevice::Create(GetOwner(), aData); > + nsRefPtr<BluetoothDeviceEvent> e = BluetoothDeviceEvent::Create(d); > + e->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("devicefound")); > + } else { > + nsCString warningMsg; #ifdef DEBUG @@ +159,5 @@ > + > +nsresult > +BluetoothAdapter::StartStopDiscovery(bool aStart, nsIDOMDOMRequest** aRequest) > +{ > + BluetoothService* bs = BluetoothService::Get(); Assert bs @@ +164,5 @@ > + > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + > + if (!rs) { > + NS_ERROR("No DOMRequest Service!"); NS_WARNING @@ +171,5 @@ > + > + nsCOMPtr<nsIDOMDOMRequest> req; > + nsresult rv = rs->CreateRequest(GetOwner(), getter_AddRefs(req)); > + if (NS_FAILED(rv)) { > + NS_ERROR("Can't create DOMRequest!"); NS_WARNING ::: dom/bluetooth/BluetoothAdapter.h @@ +33,5 @@ > NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothAdapter, > nsDOMEventTargetHelper) > + NS_DECL_EVENT_HANDLER(propertychanged) > + NS_DECL_EVENT_HANDLER(devicefound) > + NS_DECL_EVENT_HANDLER(devicedisappeared) These should be private ::: dom/bluetooth/BluetoothDevice.cpp @@ +59,5 @@ > + } else if (name.EqualsLiteral("Paired")) { > + mPaired = value.get_bool(); > + } else if (name.EqualsLiteral("UUIDs")) { > + mUuids = value.get_ArrayOfnsString(); > + } So wait, what did you decide here? That we may have other values that we silently ignore? ::: dom/bluetooth/BluetoothDevice.h @@ +27,5 @@ > + NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::) > + > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothDevice, > + nsDOMEventTargetHelper) > + NS_DECL_EVENT_HANDLER(propertychanged) This should be private. ::: dom/bluetooth/BluetoothManager.cpp @@ +63,5 @@ > + bool > + ParseSuccessfulReply(jsval& aValue) > + { > + nsCOMPtr<nsIDOMBluetoothAdapter> adapter; > + nsString& path = mReply->get_BluetoothReplySuccess().value().get_nsString(); Nit: const nsString& @@ +70,5 @@ > + > + nsresult rv; > + nsIScriptContext* sc = mManagerPtr->GetContextForEventHandlers(&rv); > + if (!sc) { > + NS_ERROR("Cannot create script context!"); This should be NS_WARNING... This can happen legitimately for a number of reasons. @@ +91,5 @@ > + > + void > + ReleaseMembers() > + { > + mManagerPtr = nsnull; See below for more about ReleaseMembers, you should call base class method here once you make those changes. @@ +121,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + bool result = true; > + > + mManagerPtr->SetEnabledInternal(mEnabled); > + return result; You need to set aValue here, and why not just 'return true'? @@ +128,5 @@ > + void > + ReleaseMembers() > + { > + //mManagerPtr must be null before returning to prevent the background > + //thread from racing to release it during the destruction of this runnable. Nit: space after // @@ +134,5 @@ > + } > + > +private: > + nsRefPtr<BluetoothManager> mManagerPtr; > + nsCOMPtr<nsIDOMDOMRequest> mDOMRequest; Oops, this is not needed. @@ +159,5 @@ > > NS_IMETHODIMP > BluetoothManager::SetEnabled(bool aEnabled, nsIDOMDOMRequest** aDomRequest) > { > + BluetoothService* bs = BluetoothService::Get(); Assert bs here? (haha) @@ +171,5 @@ > > nsCOMPtr<nsIDOMDOMRequest> request; > nsresult rv = rs->CreateRequest(GetOwner(), getter_AddRefs(request)); > + if (NS_FAILED(rv)) { > + NS_ERROR("Can't create DOM request!"); NS_WARNING @@ +204,2 @@ > { > + BluetoothService* bs = BluetoothService::Get(); Assert bs @@ +206,5 @@ > + > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + > + if (!rs) { > + NS_ERROR("No DOMRequest Service!"); NS_WARNING @@ +213,5 @@ > + > + nsCOMPtr<nsIDOMDOMRequest> request; > + nsresult rv = rs->CreateRequest(GetOwner(), getter_AddRefs(request)); > + if (NS_FAILED(rv)) { > + NS_ERROR("Can't create DOMRequest!"); NS_WARNING @@ +234,2 @@ > nsRefPtr<BluetoothManager> manager = new BluetoothManager(aWindow); > + BluetoothService* bs = BluetoothService::Get(); Assert bs @@ +258,5 @@ > } > > nsRefPtr<BluetoothManager> bluetoothManager = BluetoothManager::Create(aWindow); > + if (!bluetoothManager) { > + return NS_ERROR_FAILURE; Warning here? Or NS_ERROR would be better i think. @@ +267,5 @@ > > +void > +BluetoothManager::Notify(const BluetoothSignal& aData) > +{ > + nsCString warningMsg; Can this all be #ifdef DEBUG for now? ::: dom/bluetooth/BluetoothReplyRunnable.cpp @@ +11,5 @@ > +#include "jsapi.h" > + > +USING_BLUETOOTH_NAMESPACE > + > +BluetoothReplyRunnable::BluetoothReplyRunnable(nsIDOMDOMRequest* aReq) : Nit: This could be inlined in the header @@ +30,5 @@ > + > + DebugOnly<nsresult> rv = > + mReply->type() == BluetoothReply::TBluetoothReplySuccess ? > + rs->FireSuccess(mDOMRequest, aVal) : > + rs->FireError(mDOMRequest, mReply->get_BluetoothReplyError().error()); Oh, right, you should warn if NS_FAILED(rv). Otherwise the rv is pretty pointless. @@ +62,5 @@ > + > + nsresult rv; > + if (!mDOMRequest) { > + NS_WARNING("DOMRequest is null!"); > + rv = NS_ERROR_FAILURE; This should never happen, right? You should just assert that you always have mDOMRequest. ::: dom/bluetooth/BluetoothReplyRunnable.h @@ +23,5 @@ > + NS_DECL_NSIRUNNABLE > + > + BluetoothReplyRunnable(nsIDOMDOMRequest* aReq); > + > + virtual ~BluetoothReplyRunnable() {} Nit: protected @@ +35,5 @@ > + { > + mErrorString = aError; > + } > + > + virtual void ReleaseMembers() = 0; One thing I've found helpful with this pattern is to require that derived classes always call their base class' ReleaseMembers method if they override it (otherwise you can get into situations where derived classes don't have all their members released like they expect which usually leads to crashes). You can sort-of enforce this here by making your base class ReleaseMembers function null out mDOMRequest, and then assert that it is null after you call ReleaseMembers from Run. Make sense? @@ +38,5 @@ > + > + virtual void ReleaseMembers() = 0; > + > +protected: > + virtual bool ParseSuccessfulReply(jsval& aValue) = 0; Nit: We almost always use pointers to indicate out or in/out params so that callers can tell that the arg will be modified. Refs could be const or non-const and you have to go digging to find out. @@ +67,5 @@ > + } > +protected: > + virtual bool ParseSuccessfulReply(jsval& aValue) > + { > + return true; You should be setting aValue here to JSVAL_VOID or whatever. ::: dom/bluetooth/BluetoothService.cpp @@ +23,5 @@ > +USING_BLUETOOTH_NAMESPACE > + > +// Class for cleaning up static service > +static nsRefPtr<BluetoothService> sBluetoothService; > +static nsCOMPtr<nsIThread> sToggleBtThread; Nit: These needn't be static, they can just be globals in this file. In any case they're not static members, so 'g' is the more appropriate prefix here. @@ +72,5 @@ > + > + nsString replyError; > + if (mEnabled) { > + if (NS_FAILED(sBluetoothService->StartInternal())) { > + replyError = NS_LITERAL_STRING("Bluetooth service not available - We should never reach this point!"); replyError.AssignLiteral, below too. @@ +82,5 @@ > + } > + } > + > + if (replyError.IsEmpty()) { > + NS_ASSERTION(!mEnabled || (mEnabled && !mDeleteService), !mEnabled || !mDeleteService will do. Though, really, seems like you should be doing this in the constructor. @@ +129,5 @@ > +} > + > +nsresult > +BluetoothService::RegisterBluetoothSignalHandler(const nsAString& aNodeName, > + BluetoothSignalObserver* aHandler) Nit: indent is off here, and in the next one. @@ +176,5 @@ > +BluetoothService::Start(BluetoothReplyRunnable* aResultRunnable) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mStarted) { This is racy. mStarted is set from CleanUpBluetoothService so this isn't really guarding correctly. @@ +177,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mStarted) { > + return NS_OK; Hm, and what if a result runnable was passed in? Shouldn't you make sure that gets called? Maybe it's better for now to ensure that this is only ever called once. Same problem in Stop fwiw. @@ +181,5 @@ > + return NS_OK; > + } > + if (sToggleBtThread) { > + NS_WARNING("Already running Start/Stop function!"); > + return NS_ERROR_FAILURE; So either we should make this an assertion or we should handle this case. I'm fine with just asserting for now since there's only one place where this should be called, right? @@ +183,5 @@ > + if (sToggleBtThread) { > + NS_WARNING("Already running Start/Stop function!"); > + return NS_ERROR_FAILURE; > + } > + NS_ADDREF(aResultRunnable); This should not be necessary once you fix BluetoothReplyRunnable to not release itself (see below). @@ +198,5 @@ > +BluetoothService::Get() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + if (mInShutdown) { > + return nsnull; Warn here? Ideally we shouldn't have anything calling here once shutdown starts. @@ +205,5 @@ > + sBluetoothService = BluetoothService::Create(); > + > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + MOZ_ASSERT(obs); > + obs->AddObserver(sBluetoothService, "xpcom-shutdown", false); This can fail, but really shouldn't. Assert that it succeeds I guess. If it does we'll leak about a bajillion things. @@ +220,5 @@ > + return NS_OK; > + } > + > + if (sToggleBtThread) { > + NS_WARNING("Already running Start/Stop function!"); This seems wrong. What if we call Start, then for some reason we call Stop before the Start operation has finished? I think you have to assume that sToggleBtThread might exist here, and you should handle that case. @@ +253,5 @@ > + if (NS_FAILED(obs->RemoveObserver(this, "xpcom-shutdown"))) { > + NS_WARNING("Can't unregister bluetooth service with xpcom shutdown!"); > + sBluetoothService = nsnull; > + sToggleBtThread = nsnull; > + return NS_ERROR_FAILURE; This isn't actually a big deal. Warning should be fine, but I wouldn't early-exit here or above if you fail to get the observer service. @@ +256,5 @@ > + sToggleBtThread = nsnull; > + return NS_ERROR_FAILURE; > + } > + > + if (!mStarted) { Seems better to let Stop deal with all this. I'm not sure why you're nulling things out here. ::: dom/bluetooth/BluetoothService.h @@ +24,5 @@ > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIOBSERVER > + > + BluetoothService(); This should be protected. @@ +153,5 @@ > + mStarted = aStarted; > + } > +protected: > + virtual ~BluetoothService(); > + static BluetoothService* Create(); Nit: comment that this is implemented in a bunch of different files depending on platform @@ +156,5 @@ > + virtual ~BluetoothService(); > + static BluetoothService* Create(); > + > + bool mStarted; > + static bool mInShutdown; Nit: static members should be prefixed with 's' ::: dom/bluetooth/gonk/BluetoothGonkService.cpp @@ +8,5 @@ > +#include "BluetoothDBusService.h" > + > +USING_BLUETOOTH_NAMESPACE > + > +static struct BluedroidFunctions { Nit: This struct and several functions have a { that should go on the next line. @@ +23,5 @@ > + int (* bt_disable)(); > + int (* bt_is_enabled)(); > +} sBluedroidFunctions; > + > +bool EnsureBluetoothInit() { Nit: below you have return type on its own line (my preference) but consistency either way would be good. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +181,5 @@ > +{ > + DBusError err; > + dbus_error_init(&err); > + BluetoothReplyRunnable* replyRunnable = > + static_cast< BluetoothReplyRunnable* >(aBluetoothReplyRunnable); Sorry, I should have been more clear before ("I think it would be much nicer to do this release in the same code that added the reference" is not very helpful). What I meant was that you should do this here: // Absorb the extra ref added in GetDefaultAdapterPathInternal. nsRefPtr<BluetoothReplyRunnable> replyRunnable = dont_AddRef(static_cast<BluetoothReplyRunnable*>(aBluetoothReplyRunnable)); This assigns to the refptr without adding an additional ref so that when this function exits your extra ref will be released. The nice thing about this is that you don't have to make the runnable worry about its own refcount, and you keep all the refcount gymnastics in this file. (In general NS_ADDREF_THIS/NS_RELEASE_THIS/kungFuDeathGrip usually indicate callers that could be improved, though it's sometimes hard to figure out who those callers are.) Make sense? You can also do this for GetVoidCallback and any others you add. @@ +492,5 @@ > + // This could block. It should never be run on the main thread. > + MOZ_ASSERT(!NS_IsMainThread()); > + > + if (!mConnection) { > + return NS_OK; Seems like if EstablishDBusConnection() fails somehow you'll still be left with a dbus thread but no connection here. I'd make sure you always call StopDBus here. @@ +509,5 @@ > + NS_ERROR("Bluetooth service not started yet!"); > + return NS_ERROR_FAILURE; > + } > + NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!"); > + NS_ADDREF(aRunnable); We can clean this up a little (and SendDiscoveryMessage as well) like this: nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable; if (!dbus_func_args_async(...)) { NS_ERROR(...); return NS_ERROR_FAILURE; } // This extra ref will be released in GetObjectPathCallback. runnable.forget(); return NS_OK; That way you don't have to remember to manually release at the early-exit, and it looks a little more modern. ::: ipc/dbus/DBusThread.cpp @@ +473,4 @@ > TearDownData(); > return false; > } > + if (NS_FAILED(NS_NewNamedThread("DBus Thread", How about "DBus Event Thread" or "DBus Poll Thread" maybe? To distinguish from the "DBus Control Thread". @@ +473,5 @@ > TearDownData(); > return false; > } > + if (NS_FAILED(NS_NewNamedThread("DBus Thread", > + getter_AddRefs(mThread), Nit: Indent is off here @@ +477,5 @@ > + getter_AddRefs(mThread), > + NS_NewRunnableMethod(this, > + &DBusThread::EventLoop)))) { > + NS_WARNING("Cannot create DBus Thread!"); > + Probably want to return false here right? @@ +497,2 @@ > LOG("DBus Thread Joining\n"); > + mThread->Shutdown(); In theory this can fail (in practice only if we screw up and call it twice). A warning would be good if it does. @@ +521,4 @@ > } > > sDBusThread = new DBusThread(); > + if (!sDBusThread->StartEventLoop()) Nit: Just |return sDBusThread->StartEventLoop();| @@ +537,4 @@ > } > + > + sDBusThread->StopEventLoop(); > + printf("REMOVING THREAD!\n"); Please remove this before checkin. ::: ipc/dbus/DBusUtils.cpp @@ +94,5 @@ > + > + reply = dbus_connection_send_with_reply(conn, msg, > + &call, > + timeout_ms); > + if (reply == TRUE) { Nit: |if (reply)| @@ +116,5 @@ > const char *func, > int first_arg_type, > va_list args) { > DBusMessage *msg = NULL; > + Nit: extra whitespace

Fixed issues on gonk

Comment on attachment 635985 [details] [diff] [review] v15: Fire DeviceFound event whenever a remote Bluetooth device is discovered Review of attachment 635985 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothService.cpp @@ +21,5 @@ > +using namespace mozilla; > + > +USING_BLUETOOTH_NAMESPACE > + > +// Class for cleaning up static service Nit: This comment is in the wrong spot. @@ +24,5 @@ > + > +// Class for cleaning up static service > +nsRefPtr<BluetoothService> gBluetoothService; > +nsCOMPtr<nsIThread> gToggleBtThread; > +static int sPendingInitCount = 0; No need for static, s/s/g/ @@ +29,5 @@ > +bool BluetoothService::sInShutdown = false; > + > +NS_IMPL_ISUPPORTS1(BluetoothService, nsIObserver) > + > +class CleanUpBluetoothService : public nsRunnable Up to you, but it seems that we should call this 'ToggleBtAck' or something. @@ +45,5 @@ > + > + gBluetoothService->SetStarted(mStarted); > + > + if (mDeleteService) { > + gBluetoothService = nsnull; I don't think this is right. You only want to delete the service if a) there are no pending runnables, and b) sInShutdown is true. There should be no need to have the mDeleteService member on this or the other runnable. @@ +54,5 @@ > + } > + nsCOMPtr<nsIThread> t; > + gToggleBtThread.swap(t); > + t->Shutdown(); > + t = nsnull; Nit: This is not needed, you're about to return. @@ +57,5 @@ > + t->Shutdown(); > + t = nsnull; > + return NS_OK; > + } > +private: Nit: newline above here. @@ +93,5 @@ > + } > + } > + > + if (replyError.IsEmpty()) { > + nsCOMPtr<nsIRunnable> cleanUpTask = new CleanUpBluetoothService(mEnabled, mDeleteService); So now that we're counting runnables I think you need to always post some sort of reply here, even if there was an error (you'll need to adjust the enabled arg you pass to the runnable if it fails, of course). Otherwise we'll never shut down. @@ +126,5 @@ > + bool mDeleteService; > + nsRefPtr<BluetoothReplyRunnable> mRunnable; > +}; > + > +BluetoothService::BluetoothService() : Can you inline the constructor/destructor in the header? @@ +189,5 @@ > + > + // If we're shutting down, bail early. > + if (sInShutdown && aStart) { > + NS_WARNING("Start called while in shutdown!"); > + return NS_OK; You should return an error code here. And I'd make this NS_ERROR so we can go fix callers to do something smarter. @@ +192,5 @@ > + NS_WARNING("Start called while in shutdown!"); > + return NS_OK; > + } > + if (!gToggleBtThread) { > + NS_NewNamedThread("BT Init", Nit: "Bluetooth", not "BT". Actually, how about "BluetoothControl"? @@ +195,5 @@ > + if (!gToggleBtThread) { > + NS_NewNamedThread("BT Init", > + getter_AddRefs(gToggleBtThread)); > + } > + nsRefPtr<BluetoothReplyRunnable> runnable = aResultRunnable; As far as I can tell you're leaking this by calling 'forget' below. In this case there's no reason to do extra refcount gymnastics, just pass aResultRunnable into the ToggleBtTask and its refcount will be modified by its nsRefPtr member. And lose this refptr entirely. @@ +197,5 @@ > + getter_AddRefs(gToggleBtThread)); > + } > + nsRefPtr<BluetoothReplyRunnable> runnable = aResultRunnable; > + nsCOMPtr<nsIRunnable> r = new ToggleBtTask(aStart, sInShutdown, aResultRunnable); > + gToggleBtThread->Dispatch(r, NS_DISPATCH_NORMAL); This can fail, in theory, so we should handle it. @@ +215,5 @@ > +{ > + return StartStopBluetooth(aResultRunnable, false); > +} > + > +//static Nit: space after // @@ +220,5 @@ > +BluetoothService* > +BluetoothService::Get() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + if (sInShutdown) { I'd move this check inside the !gBluetoothService block. No harm handing it out during shutdown, you just don't want to *create* it after shutdown. @@ +225,5 @@ > + NS_WARNING("BluetoothService returns null during shutdown"); > + return nsnull; > + } > + if (!gBluetoothService) { > + gBluetoothService = BluetoothService::Create(); Some of these implementations return null, right? You don't want to call AddObserver with a null pointer here. @@ +231,5 @@ > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + MOZ_ASSERT(obs); > + nsresult rv; > + rv = obs->AddObserver(gBluetoothService, "xpcom-shutdown", false); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); This will warn in opt builds about unused variables. This pattern is helpful here: if (NS_FAILED(obs->AddObserver(...))) { NS_ERROR("AddObserver failed!"); } @@ +237,5 @@ > + return gBluetoothService; > +} > + > +nsresult > +BluetoothService::Observe(nsISupports* aSubject, const char* aTopic, You should assert that aTopic is the one you expect. @@ +243,5 @@ > +{ > + sInShutdown = true; > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + if (obs) { > + if (NS_FAILED(obs->RemoveObserver(this, "xpcom-shutdown"))) { Nit: if (obs && NS_FAILED(...)) ::: dom/bluetooth/BluetoothService.h @@ +21,5 @@ > + > +class BluetoothService : public nsIObserver > +{ > +public: > + BluetoothService(); This should be protected so that you have to go through the Get method. @@ +138,5 @@ > + > + /** > + * Platform specific startup functions go here. Usually deals with member > + * variables, so not static. Expected to be called outside of main thread, but > + * may be called on main thread during shutdown. Let's fix this comment to match the new reality. In both, it's not 'expected' but guaranteed to be called off the main thread. @@ +144,5 @@ > + * @return NS_OK on correct startup, NS_ERROR_FAILURE otherwise > + */ > + virtual nsresult StopInternal() = 0; > + > + void SetStarted(bool aStarted) So now that we've reworked stuff this seems unnecessary now. Can we just remove all this? @@ +148,5 @@ > + void SetStarted(bool aStarted) > + { > + mStarted = aStarted; > + } > +protected: Nit: newline above here. @@ +150,5 @@ > + mStarted = aStarted; > + } > +protected: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIOBSERVER Hm, these should be public.

> Nit: "Bluetooth", not "BT". Actually, how about "BluetoothControl"? Went with BluetoothCtrl. BluetoothControl ends up being 16 characters without a null, and there's a static assert under NS_NewThreadName to make sure the string length is <= 16

Comment on attachment 636389 [details] [diff] [review] v16: Asynchronous dbus events for bluetooth, Start/StopDiscovery and device event firing Review of attachment 636389 [details] [diff] [review]: ----------------------------------------------------------------- Eek, I thought we were done, but I found more problems in DBusThread :( BluetoothService looks great though! In DBusThread::StartEventLoop you've got a lock that wraps the whole function. In there you're calling socketpair and a series of dbus calls followed by NS_NewThread. Our general rule is to never call system functions with locks held, and in Gecko we try to never call into other modules (maybe loosely defined as any code that is in a different directory these days) with locks held. Basically we want mutexes to be held as briefly as possible to ensure that they can't block for more than a tiny bit. In the particular case of StartEventLoop there's no real need for the mutex to be held at all... You haven't created the thread that could be racing with you yet, so your lock isn't really doing much for you. StopEventLoop also locks and then calls write and then spins an event loop while waiting for the thread to shut down which must be fixed. But I wonder if you really need the mutex at all. You do some up-front work to make the fds that you need, but after that couldn't everything else be handled on the dbus poll thread? If all your mutable state is handled there then you shouldn't need the mutex. You're calling seemingly matched dbus functions on two different threads (e.g. dbus_bus_add_match on parent thread, dbus_bus_remove_match on dbus thread) which may technically be ok but seems mighty confusing. Moving all the mutable stuff to the dbus poll thread would clear that up too. So for now we have to fix the system-calls-under-mutex, and the event-loop-under-mutex, and the race I found below (mIsRunning). If you want to wait to do the mutex removal/data separation then we should file a followup to give DBusThread a thorough scrubbing. ::: dom/bluetooth/BluetoothService.cpp @@ +31,5 @@ > + > +class ToggleBtAck : public nsRunnable > +{ > +public: > + ToggleBtAck() Nit: This isn't really needed. @@ +55,5 @@ > + return NS_OK; > + } > + > +private: > + bool mStarted; This is no longer needed. @@ +176,5 @@ > + return NS_ERROR_FAILURE; > + } > + if (!gToggleBtThread) { > + NS_NewNamedThread("BluetoothCtrl", > + getter_AddRefs(gToggleBtThread)); Ah, should have seen this earlier, we should NS_ENSURE_TRUE gToggleBtThread here since it can fail. @@ +206,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + // If we already exist, exit early > + if(gBluetoothService) { Nit: space here, and in a few places below. ::: dom/bluetooth/BluetoothService.h @@ +6,5 @@ > + > +#ifndef mozilla_dom_bluetooth_bluetootheventservice_h__ > +#define mozilla_dom_bluetooth_bluetootheventservice_h__ > + > +#include "mozilla/RefPtr.h" This is no longer used I think. @@ +79,5 @@ > + * Start bluetooth services. Starts up any threads and connections that > + * bluetooth needs to operate on the current platform. Assumed to be run on > + * the main thread with delayed return for blocking startup functions via > + * runnable. If runnable pointer is NULL, assumes we're in shutdown mode and > + * will run blocking shutdown functions inline. Nit: This last part of comment is now untrue. ::: ipc/dbus/DBusThread.cpp @@ +125,4 @@ > > // DBus Thread Class prototype > > +struct DBusThread : public RefCounted<DBusThread>, It's unclear to me why this is refcounted. You don't do anything but assign once to sDBusThread and then later set it to null, right? @@ +390,2 @@ > > + mIsRunning = true; Ok, just now realizing that this is a race. You set and check mIsRunning on the main thread while holding the lock, but then here you set without holding it. This means that a call to StartEventLoop followed by a call to StopEventLoop could leave the dbus poll thread stuck waiting on the socket. I think the simplest solution is probably to lose the mIsRunning member entirely (and the unused IsEventLoopRunning function). Then StartEventLoop can just set mThread and StopEventLoop should always run if mThread is non-null. @@ +404,2 @@ > != -1) { > switch (data) { Nit: Can you reformat this to match the style guide again? In particular you should indent the cases and you don't need the braces. @@ +497,2 @@ > LOG("DBus Thread Joining\n"); > + if(NS_FAILED(mThread->Shutdown())) { This will spin the event loop and so you'll need to swap this to a stack comptr before calling it.

>> >> // DBus Thread Class prototype >> >> +struct DBusThread : public RefCounted<DBusThread>, > > It's unclear to me why this is refcounted. You don't do anything but > assign once to sDBusThread and then later set it to null, right? I was using NS_NewRunnableMethod which requires RefCounted traits. If I switch to NS_NewNonOwningRunnableMethod, I can drop the RefCounted/nsRefPtr and use an AutoPtr. Seems since we'll have dropped out of the thread before destruction of the object anyways?

I'm going to file the DBusThread issues as a different bug, will attach bug number to this bug once that's done. Happy to make it block on this one too, just not liking the idea of more changes to this already gigantic patch.

Ok, followup filed at bug 768685

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #40) > I'm going to file the DBusThread issues as a different bug, will attach bug > number to this bug once that's done. Happy to make it block on this one too, > just not liking the idea of more changes to this already gigantic patch. Well, I outlined the things that have to be fixed before we can land this one ("the system-calls-under-mutex, and the event-loop-under-mutex, and the race I found"). I can't really r+ this with those still broken. The cleanup and such can wait, of course.

Ah, ok, will take care of those ASAP and try to get this out of the way.

Actually... "If you want to wait to do the mutex removal/data separation then we should file a followup to give DBusThread a thorough scrubbing." So, I cleaned up the race by removing mIsRunning is v17. But we need to scrub the mutexes in this one too? That wasn't what I got from what you were saying there. I hadn't done that yet because I thought we were moving that to the other bug? Does this mean I should fix the system-calls-under-mutex/event-loop(...) by moving the mutexes, or should I just go ahead and kill them here?

Ok, nevermind, rereviewed what you said and can just kill them here easily. New patch incoming.

Comment on attachment 640750 [details] [diff] [review] v18: Asynchronous dbus events for bluetooth, Start/StopDiscovery and device event firing Review of attachment 640750 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following stuff fixed! Thanks for sticking with this! ::: ipc/dbus/DBusThread.cpp @@ +378,4 @@ > } > > void* > +DBusThread::EventLoop() Weird, why does this return void* ? I think you can just make this a real void function. @@ +477,5 @@ > } > + char data = DBUS_EVENT_LOOP_EXIT; > + ssize_t wret = write(mControlFdW.get(), &data, sizeof(char)); > + if (wret < 0) { > + LOG("Cannot write exit bit to DBus Thread!\n"); I don't know about this... This is a pretty serious problem because you're about to hang below (when you call tmpThread->Shutdown). I think in addition to the LOG you should do a fatal assertion and an early return. @@ +502,4 @@ > } > + > + sDBusThread = new DBusThread(); > + return sDBusThread->StartEventLoop(); This can fail, and if it does you'll leave sDBusThread set. Best to use this pattern: nsAutoPtr<DBusThread> thread = new DBusThread(); if (!thread->StartEventLoop()) { NS_WARNING(...); return false; } sDBusThread = thread; return true; @@ +514,3 @@ > } > + > + sDBusThread->StopEventLoop(); This will spin an event loop and during that loop other code could try to call StartDBus or something. You need to swap this into a stack pointer before calling StopEventLoop here.

Nits picked