Closed
Bug 1015826
Opened 11 years ago
Closed 11 years ago
Avoid accessing static variables on different thread
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(blocking-b2g:1.4+, 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 | ||
Updated•11 years ago
|
Assignee: nobody → echou
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8429165 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8429166 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8429167 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8429168 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8429169 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8429166 -
Flags: review?(tzimmermann) → review+
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8429169 -
Flags: review?(tzimmermann) → review+
Assignee | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
* Revised by following Thomas' comments.
Attachment #8429165 -
Attachment is obsolete: true
Attachment #8429827 -
Flags: review?(tzimmermann)
Comment 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
* Categorized static variables into 3 groups: TODO, main thread only and callback thread only.
Attachment #8429166 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
>
> ::: 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. ;)
Assignee | ||
Comment 16•11 years ago
|
||
* Don't use 's' as the initial of local variables.
Attachment #8429167 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
>
> 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().
Assignee | ||
Comment 18•11 years ago
|
||
> > @@ +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()).
Assignee | ||
Comment 19•11 years ago
|
||
* nits picked.
Attachment #8429168 -
Attachment is obsolete: true
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8429169 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Comment 22•11 years ago
|
||
Follow-up to clean up: bug 1016873
Assignee | ||
Comment 23•11 years ago
|
||
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
Assignee | ||
Comment 24•11 years ago
|
||
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?
Comment 25•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 26•11 years ago
|
||
Needs branch patches for uplift.
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
status-firefox30:
--- → wontfix
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
Flags: needinfo?(echou)
Keywords: branch-patch-needed
Target Milestone: --- → 2.0 S3 (6june)
Assignee | ||
Comment 27•11 years ago
|
||
* patch 1 for 1.4
Assignee | ||
Comment 28•11 years ago
|
||
* patch 2 for 1.4
Assignee | ||
Comment 29•11 years ago
|
||
* patch 3 for 1.4
Assignee | ||
Comment 30•11 years ago
|
||
* patch 4 for 1.4
Assignee | ||
Comment 31•11 years ago
|
||
* patch 5 for 1.4
Assignee | ||
Comment 32•11 years ago
|
||
Patches for 1.4 are ready. Checkin-needed.
Flags: needinfo?(echou)
Keywords: branch-patch-needed → checkin-needed
Updated•11 years ago
|
Keywords: checkin-needed
Comment 33•11 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•