Currently in dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp, certain static nsTArray objects may be used on different thread and might cause racing issues. Let's fix it.

Hi Thomas, Please review my patches. :) There will be still 3 variables non-thread-safe(sAdapterBdAddress, sAdapterBdName, sAdapterBondedAddressArray), however I couldn't find an effective or a good way to make them thread-safe. The good news is there is only one place using them in main thread, which is function GetDefaultAdapterPathInternal(). Keeping them as they are might not be severe now since they are all strings... I'll see if I can have a solution to make these variables thread-safe as well.

Comment on attachment 8429165 [details] [diff] [review] patch 1: v1: Dispatch AdapterStateChangeCallback to main thread Review of attachment 8429165 [details] [diff] [review]: ----------------------------------------------------------------- Hi Eric, The patch itself is OK, but there are two general things. First of all, you should probably group the variables by their owner thread, and add a comment to each group that says which thread that is. BlueZ does it, and it helps a lot. The second thing is that I was wondering if |sIsBtEnabled| is actually required. |sBluetoothService| already keeps track of the BT state. ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +265,5 @@ > + * Bluedroid HAL callback functions > + * > + * Several callbacks are dispatched to main thread to avoid racing issues. > + */ > +class AdapterStateChangeCallbackTask : public nsRunnable I think there's a compiler warning about having virtual functions in a class without virtual destructor. You can fix that by added MOZ_FINAL after the class name. Some nitpicking about class names: There are runnables and tasks, but classes inherited from runnables are often named *Task. We might want to clean this up at some later point.

Comment on attachment 8429167 [details] [diff] [review] patch 3: v1: Use Atomic on certain primitive variables to ensure thread-safe Review of attachment 8429167 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +52,5 @@ > +static bool sIsBtEnabled = false; > +static Atomic<bool> sAdapterDiscoverable(false); > +static Atomic<uint32_t> sAdapterDiscoverableTimeout(0); > +static nsTArray<nsRefPtr<BluetoothProfileController> > sControllerArray; > +static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sSetPropertyRunnableArray; Are you sure these arrays are thread-safe? @@ +705,5 @@ > BT_LOGR("Error: %s", strerror(err)); > return false; > } > module->methods->open(module, BT_HARDWARE_MODULE_ID, &device); > + bluetooth_device_t* sBtDevice = (bluetooth_device_t *)device; |btDevice| if declared locally.

Comment on attachment 8429168 [details] [diff] [review] patch 4: v1: Dispatch part of RemoteDevicePropertiesCallback to main thread Review of attachment 8429168 [details] [diff] [review]: ----------------------------------------------------------------- I'd oppose the general design of |sRemoteDevicesPack|, but that's not what the patch is about. ::: dom/bluetooth/BluetoothDevice.cpp @@ +59,4 @@ > > const InfallibleTArray<BluetoothNamedValue>& values = > aValue.get_ArrayOfBluetoothNamedValue(); > + ? ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +448,5 @@ > + BT_WARNING("Failed to dispatch to main thread!"); > + return NS_OK; > + } > + > + static InfallibleTArray<BluetoothNamedValue> sRemoteDevicesPack; This line and everything below looks like trouble. ;) @@ +455,5 @@ > + sRemoteDevicesPack.AppendElement( > + BluetoothNamedValue(mRemoteDeviceBdAddress, mProps)); > + > + if (--sRequestedDeviceCountArray[0] == 0) { > + if (sGetDeviceRunnableArray.IsEmpty()) { Should this branch clean up the state? @@ +510,5 @@ > } > } > > + // Redirect to main thread to avoid racing problem > + NS_DispatchToMainThread( Sometimes you check the return value of |NS_DispatchToMainThread| and sometimes you don't.

Hi Thomas, Thanks for prompt review! Reply inline: > > Hi Eric, > > The patch itself is OK, but there are two general things. First of all, you > should probably group the variables by their owner thread, and add a comment > to each group that says which thread that is. BlueZ does it, and it helps a > lot. > Yeah, that would help. I'll do it in another patch because patch 1 has been revised and would not be the best patch to do it. > The second thing is that I was wondering if |sIsBtEnabled| is actually > required. |sBluetoothService| already keeps track of the BT state. That's a really good point. The new patch is coming. > ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp > @@ +265,5 @@ > > + * Bluedroid HAL callback functions > > + * > > + * Several callbacks are dispatched to main thread to avoid racing issues. > > + */ > > +class AdapterStateChangeCallbackTask : public nsRunnable > > I think there's a compiler warning about having virtual functions in a class > without virtual destructor. You can fix that by added MOZ_FINAL after the > class name. > Will be done in the new patch. > Some nitpicking about class names: There are runnables and tasks, but > classes inherited from runnables are often named *Task. We might want to > clean this up at some later point. Sure. I'll do it.

* Revised by following Thomas' comments.

Comment on attachment 8429827 [details] [diff] [review] patch 1: v2: Remove unneccessary variable sIsBtEnabled to avoid racing issue Review of attachment 8429827 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me :) ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +34,5 @@ > #include "mozilla/unused.h" > > +#define ENSURE_BLUETOOTH_IS_READY(runnable, result) \ > + do { \ > + if (!sBtInterface || !mEnabled) { \ Just a nit: calling |IsEnabled| might be preferable to using |mEnabled| directly.

* Categorized static variables into 3 groups: TODO, main thread only and callback thread only.

* Nit picked.

> > ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp > @@ +52,5 @@ > > +static bool sIsBtEnabled = false; > > +static Atomic<bool> sAdapterDiscoverable(false); > > +static Atomic<uint32_t> sAdapterDiscoverableTimeout(0); > > +static nsTArray<nsRefPtr<BluetoothProfileController> > sControllerArray; > > +static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sSetPropertyRunnableArray; > > Are you sure these arrays are thread-safe? > Ah, I didn't mean that operations of nsTArray are thread-safe. I just wanted to say that each of them is only used on one thread so we're safe. ;)

* Don't use 's' as the initial of local variables.

> > I'd oppose the general design of |sRemoteDevicesPack|, but that's not what > the patch is about. Yeah, that sucks. We should come up with a better solution. > > @@ +455,5 @@ > > + sRemoteDevicesPack.AppendElement( > > + BluetoothNamedValue(mRemoteDeviceBdAddress, mProps)); > > + > > + if (--sRequestedDeviceCountArray[0] == 0) { > > + if (sGetDeviceRunnableArray.IsEmpty()) { > > Should this branch clean up the state? Good point. > > @@ +510,5 @@ > > } > > } > > > > + // Redirect to main thread to avoid racing problem > > + NS_DispatchToMainThread( > > Sometimes you check the return value of |NS_DispatchToMainThread| and > sometimes you don't. Just checked the implementation of NS_DispatchToMainThread(). It will print out warnings if it fails to dispatch, so I'll make a cleanup patch to remove all NS_FAILED(NS_DispatchToMainThread()) except those do other things than BT_WARNING().

> > @@ +510,5 @@ > > > } > > > } > > > > > > + // Redirect to main thread to avoid racing problem > > > + NS_DispatchToMainThread( > > > > Sometimes you check the return value of |NS_DispatchToMainThread| and > > sometimes you don't. > > Just checked the implementation of NS_DispatchToMainThread(). It will print > out warnings if it fails to dispatch, so I'll make a cleanup patch to remove > all NS_FAILED(NS_DispatchToMainThread()) except those do other things than > BT_WARNING(). Oh wait, it's still possible that NS_DispatchToMainThread() would return !NS_OK without warnings. We should keep checking NS_FAILED(NS_DispatchToMainThread()).

* nits picked.

try: https://tbpl.mozilla.org/?tree=Try&rev=8782e778c5c7

Follow-up to clean up: bug 1016873

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/743664d1dec7 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/b1261810cda5 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/b862a4112bc9 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/39661118f0a5 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/bb0cd4898af1

Nominate as 1.4+ since it will fix potential racing issues and even random crashes, which are described in bug 997962 and bug 1011110. During the discussion of bug 997962, I gave a patch which is just like what I have done to this issue. Tapas applied my patch and mentioned that the crash have not been seen until now (bug 997962 comment 60).

https://hg.mozilla.org/mozilla-central/rev/743664d1dec7 https://hg.mozilla.org/mozilla-central/rev/b1261810cda5 https://hg.mozilla.org/mozilla-central/rev/b862a4112bc9 https://hg.mozilla.org/mozilla-central/rev/39661118f0a5 https://hg.mozilla.org/mozilla-central/rev/bb0cd4898af1

Needs branch patches for uplift.

* patch 1 for 1.4

* patch 2 for 1.4

* patch 3 for 1.4

* patch 4 for 1.4

* patch 5 for 1.4

Patches for 1.4 are ready. Checkin-needed.

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c72852f8cce8 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ba87a98c2d36 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/256fe8a580d2 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/f16d304ac0af https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4e66e8583e92