891257 - Disconnect gracefully when the user turns off Bluetooth

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Eric Chou [:ericchou] [:echou]]

During the process of figuring out the root cause of bug 872976, we realized that Bluetooth low-level connection (ACL) would not be correctly dropped if the user disables Bluetooth without choosing 'Disconnect' first. The remote connected device may feel like the connection was gone abnormally, so it may try to restore connection aggressively. Meanwhile, if the user enables Bluetooth, Gaia would start to reconnect with the remote device as well, and collision is very likely to happen.

Comment 1

[Eric Chou [:ericchou] [:echou]]

Attachment: WIP: Brute force

* WIP
* Can build pass

Comment 2

[Eric Chou [:ericchou] [:echou]]

Attachment: Bug-891257-Disconnect-gracefully-when-the-user-turns.patch

* Use sConnectedDeviceCount to decide if it's ready for turning off Bluetooth
* Get rid of function non-portable function DisconnectAllAcls()

Comment 3

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: disconnect gracefully

Hi Blake,

Please assist with reviewing. There are 3 major changes in this patch:

1. Turn off Bluetooth after all connections are dropped.
2. Error handling if the counter doesn't become 0.
3. Remove 'DisconnectAllAcls' DBus call since it's not from original BlueZ but the BlueZ modified by CodeAurora

Moreover, after applying this patch, the problem described in bug 872976 could be also fixed.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 778392 [details] [diff] [review]
patch 1: v1: disconnect gracefully

I'm pretty sure this is correct, but I'm not sure about the busy loop. Asking bent for his input.

Comment 5

[Eric Chou [:ericchou] [:echou]]

(In reply to Blake Kaplan (:mrbkap) from comment #4)
> Comment on attachment 778392 [details] [diff] [review]
> patch 1: v1: disconnect gracefully
> 
> I'm pretty sure this is correct, but I'm not sure about the busy loop.
> Asking bent for his input.

Thanks, Blake!

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 778392 [details] [diff] [review]
patch 1: v1: disconnect gracefully

Review of attachment 778392 [details] [diff] [review]:
-----------------------------------------------------------------

Let's please not add a busy loop that could go for five seconds.

This needs a lock and condition variable that triggers on changes to the connection count.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +162,2 @@
>  static nsString sAdapterPath;
> +static Atomic<int32_t> sIsPairing(0);

Could this be a bool now?

Comment 7

[Eric Chou [:ericchou] [:echou]]

Hi Ben,

> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +162,2 @@
> >  static nsString sAdapterPath;
> > +static Atomic<int32_t> sIsPairing(0);
> 
> Could this be a bool now?

I tried but got an error in build time: 

  "mozilla/Atomics.h only supports 32-bit and pointer-sized types"

That's why I used int32_t.

Comment 8

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v2: disconnect gracefully

Hi Ben,

I replaced the busy loop with a Mutex. Please review my patch.

Thanks!

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 783059 [details] [diff] [review]
patch 1: v2: disconnect gracefully

Hi Eric, my review queue is super long at the moment, perhaps Blake can help here :)

Comment 10

[Eric Chou [:ericchou] [:echou]]

(In reply to ben turner [:bent] from comment #9)
> Comment on attachment 783059 [details] [diff] [review]
> patch 1: v2: disconnect gracefully
> 
> Hi Eric, my review queue is super long at the moment, perhaps Blake can help
> here :)

Sure. Thanks anyway!

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 783059 [details] [diff] [review]
patch 1: v2: disconnect gracefully

Review of attachment 783059 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +162,4 @@
>  static nsString sAdapterPath;
> +static Atomic<int32_t> sIsPairing(0);
> +static Atomic<int32_t> sConnectedDeviceCount(0);
> +static Mutex sStopBluetoothLock("lock.bluetooth.sStopBluetoothLock");

This needs to be a StaticMutex and a condvar (see below).

@@ +1436,5 @@
> +
> +      if (sConnectedDeviceCount > 0) {
> +        sStopBluetoothLock.Lock();
> +      } else {
> +        sStopBluetoothLock.Unlock();

This is not the way to do this. Holding locks for large portions of time like this is very much an anti-pattern as it invites deadlocks, amongst other problems. I would have expected this to look more like:

hold lock
increment/decrement sConnectedDeviceCount
if (sConnectedDeviceCount == 0)
  notify on a condition variable;

@@ +1674,5 @@
>  
> +  nsRefPtr<UnlockTask> r = new UnlockTask();
> +  NS_DispatchToMainThread(r);
> +
> +  sStopBluetoothLock.Lock();

And then this could wait on a condition variable, that also has the advantage that you can pass a timeout to Wait(), which allows you to get rid of the UnlockTask.

Comment 12

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v3: disconnect gracefully (using Monitor)

In this patch, I changed to use Monitor instead of Mutex, so that I can notify the 'StopInternal' thread about the connectedBluetoothDevices becomes 0. Moreover, calling function monitor.Wait([Timeout]) can let us get rid of UnlockTask, just like Blake said in the previous comment.

One potential problem is that I couldn't find something like 'StaticMonitor'. Not sure if I need to use another way to declare the Monitor variable.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 785644 [details] [diff] [review]
patch 1: v3: disconnect gracefully (using Monitor)

Review of attachment 785644 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +163,4 @@
>  static nsString sAdapterPath;
> +static Atomic<int32_t> sIsPairing(0);
> +static Atomic<int32_t> sConnectedDeviceCount(0);
> +static Monitor sStopBluetoothMonitor("BluetoothService.sStopBluetoothMonitor");

This is going to lead to additional debug spew on startup. Can you please file a followup bug on implementing StaticMonitor (a la StaticMutex)?

Also, with the lock here, sConnectedDeviceCount no longer needs to be an Atomic<> since all reads and writes are protected by it.

@@ +1436,5 @@
> +      } else {
> +        --sConnectedDeviceCount;
> +      }
> +
> +      if (sConnectedDeviceCount == 0) {

This check can actually be moved into the disconnected case. Can you also add an assertion that sConnectedDeviceCount never falls below 0?

@@ +1619,5 @@
>    // This could block. It should never be run on the main thread.
>    MOZ_ASSERT(!NS_IsMainThread());
>  
> +  if (sConnectedDeviceCount > 0) {
> +    MonitorAutoLock lock(sStopBluetoothMonitor);

This isn't correct from a threading perspective. You have to make sure you hold the lock before you test the count variable, otherwise you risk racing with another thread changing sConnectedDeviceCount. So you need to move the AutoLock out of the if statement here.

@@ +1620,5 @@
>    MOZ_ASSERT(!NS_IsMainThread());
>  
> +  if (sConnectedDeviceCount > 0) {
> +    MonitorAutoLock lock(sStopBluetoothMonitor);
> +    lock.Wait(TIMEOUT_FORCE_TO_DISABLE_BT);

The parameter to Wait is a PRIntervalTime. This should really be: lock.Wait(PR_SecondsToInterval(TIMEOUT_FORCE_TO_DISABLE_BT));

And TIMEOUT_FORCE_TO_DISABLE_BT should become 5 instead of 5000.

@@ +1625,1 @@
>    }

You'll also probably want to unlock the lock after the waiting is done because calling StopDBus with a random lock held invites deadlocks.

@@ +1625,4 @@
>    }
>  
>    if (!mConnection) {
> +    goto result;

I don't see a reason for this change.

Comment 14

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v4: disconnect gracefully (using Monitor)

* Fixed problems mentioned by Blake in the previous review. 
* Haven't filed a followup for StaticMonitor. Will do later.

Comment 15

[Eric Chou [:ericchou] [:echou]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #14)
> Created attachment 786719 [details] [diff] [review]
> patch 1: v4: disconnect gracefully (using Monitor)
> 
> * Fixed problems mentioned by Blake in the previous review. 
> * Haven't filed a followup for StaticMonitor. Will do later.

Followup filed: bug 902365

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 786719 [details] [diff] [review]
patch 1: v4: disconnect gracefully (using Monitor)

Review of attachment 786719 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks.

Comment 17

[Eric Chou [:ericchou] [:echou]]

Thanks, Blake. :)

try: https://tbpl.mozilla.org/?tree=Try&rev=3cd5188a06be

Comment 18

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9fc269fbc710

Comment 19

[Eric Chou [:ericchou] [:echou]]

Nominate as leo+ since the patch could significantly reduce the failure rate of auto restore connection. See bug 872976 for more information.

Comment 20

[bhavana bajaj [:bajaj]]

A nice to have but not a blocker for 1.1

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fc269fbc710