Closed Bug 1015826 Opened 7 years ago Closed 7 years ago

Avoid accessing static variables on different thread

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: echou, Assigned: echou)

References

Details

Attachments

(10 files, 6 obsolete files)

3.09 KB, patch
Details | Diff | Splinter Review
8.65 KB, patch
Details | Diff | Splinter Review
3.79 KB, patch
Details | Diff | Splinter Review
5.22 KB, patch
Details | Diff | Splinter Review
5.76 KB, patch
Details | Diff | Splinter Review
14.00 KB, patch
Details | Diff | Splinter Review
2.87 KB, patch
Details | Diff | Splinter Review
4.31 KB, patch
Details | Diff | Splinter Review
4.99 KB, patch
Details | Diff | Splinter Review
6.53 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → echou
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.
Attachment #8429165 - Flags: review?(tzimmermann) → review+
Attachment #8429166 - Flags: review?(tzimmermann) → review+
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.
Attachment #8429167 - Flags: review?(tzimmermann) → review+
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.
Attachment #8429168 - Flags: review?(tzimmermann) → review+
Attachment #8429169 - Flags: review?(tzimmermann) → review+
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.
Attachment #8429165 - Attachment is obsolete: true
Attachment #8429827 - Flags: review?(tzimmermann)
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.
Attachment #8429827 - Flags: review?(tzimmermann) → review+
* Categorized static variables into 3 groups: TODO, main thread only and callback thread only.
Attachment #8429166 - Attachment is obsolete: true
> 
> ::: 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.
Attachment #8429167 - Attachment is obsolete: true
> 
> 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()).
Follow-up to clean up: bug 1016873
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).
blocking-b2g: --- → 1.4?
blocking-b2g: 1.4? → 1.4+
Needs branch patches for uplift.
Flags: needinfo?(echou)
Target Milestone: --- → 2.0 S3 (6june)
Attached patch (1.4) patch 1Splinter Review
* patch 1 for 1.4
Attached patch (1.4) patch 2Splinter Review
* patch 2 for 1.4
Attached patch (1.4) patch 3Splinter Review
* patch 3 for 1.4
Attached patch (1.4) patch 4Splinter Review
* patch 4 for 1.4
Attached patch (1.4) patch 5Splinter Review
* patch 5 for 1.4
Patches for 1.4 are ready. Checkin-needed.
Flags: needinfo?(echou)
You need to log in before you can comment on or make changes to this bug.