942712 - [bluetooth] first time searching devices won't work

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Walter Chen[:ypwalter][:wachen]]

device: nexus 4 master build

steps:
1. flash the phone
2. launch settings app
3. go to bluetooth subsection
4. it started to search for devices

expected result: 
there are some devices found

actual result:
no device found, you need to re-enable bluetooth to find devices.

Comment 1

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

I can reproduce. Patch is coming.

Comment 2

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

Let me explain the root cause briefly. When event adapterAdded is fired to Gaia, mozBluetooth.enabled is supposed to be true. If you trace related code in BluetoothService.cpp, you'll find out that mozBluetooth.enabled won't be set until event 'enabled'/'disabled' is fired. It seems to be reasonable so far, unfortunately another event 'adapteradded' may be fired before 'enabled' because the timing it comes up depends on our protocol stack. Apps will try to get mozBluetooth.enabled after it received adapteradded, however mozBluetooth.enabled may not be updated in time.

Comment 3

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

Attachment: patch 1: v1: move gToggleInProgress to a more reasonable position

* Set gToggleInProgress to false when bt toggling is really done.

Comment 4

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

Attachment: patch 2: v1: Call SetEnabled() before and after toggling Bluetooth

Comment 5

[Walter Chen[:ypwalter][:wachen]]

Thanks for the quick fix!! :D

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8338315 [details] [diff] [review]
patch 1: v1: move gToggleInProgress to a more reasonable position

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

Seems reasonable.

Comment 7

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

Attachment: patch 2: v2: Ensure event 'AdapterAdded' will be fired after 'Enabled' being fired

* Patch modified after offline discussion with Gina. In this patch, I can guarantee that event AdapterAdded would be fired after event Enabled. Because SetEnabled() would be called before 'Enabled' is fired, the sequence of these 3 operations would become

  SetEnabled() -> 'Enabled' is fired -> 'AdapterAdded' is fired

, and it is what we want.

Comment 8

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

Attachment: patch 2: v3: Ensure event 'AdapterAdded' will be fired after 'Enabled' being fired

* Patch updated since we need to ensure that low-level "AdapterAdded" event has been fired before notifying Gaia.

Comment 9

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8339156 [details] [diff] [review]
patch 2: v3: Ensure event 'AdapterAdded' will be fired after 'Enabled' being fired

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

Overall looks good, but still have some questions. Please correct me if I am wrong. Thanks.

::: dom/bluetooth/BluetoothService.cpp
@@ +796,5 @@
> +BluetoothService::TryFiringAdapterAdded()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (IsToggling() || sIsAdapterAddedFired || !mAdapterIsReady) {

Question: I was wondering if variable |sIsAdapterAddedFired| is needed? Am I missing something here?

On the other hands, we might get multiple events of 'AdapterAdded' after enabling Bluetooth on BlueZ. If we check the value of |sIsAdpaterAddedFired| here, we will fire the event for the first time. Is it what we expected?

@@ +805,5 @@
> +                         NS_LITERAL_STRING(KEY_MANAGER), true);
> +  DistributeSignal(signal);
> +
> +  sIsAdapterAddedFired = true;
> +  mAdapterIsReady = false;

Question: The name is a bit confusing to me. We reset |mAdapterIsReady| to its default value (false) here, however, the adapter is still ready to use. How about moving it to StopInternal? Does it make sense to you?

::: dom/bluetooth/BluetoothService.h
@@ +321,5 @@
> +   * Below 2 function/variable are used for ensuring event 'AdapterAdded' will
> +   * be fired after event 'Enabled'.
> +   */
> +  void TryFiringAdapterAdded();
> +  bool mAdapterIsReady;

I'd like to make |mAdapterIsReady| as a private/protected member and create a setter function for accessing its value.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +106,5 @@
> +    }
> +
> +    // Try to fire event 'AdapterAdded' to fit the original behaviour when
> +    // we used BlueZ as backend.
> +    BluetoothService* bs = BluetoothService::Get();

nit: Please make sure that |bs| is not a nullptr before using it.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +727,3 @@
>      // Notify BluetoothManager whenever adapter name is ready.
>      if (!propertyValue.get_nsString().IsEmpty()) {
>        sAdapterNameIsReady = true;

Question: Can we re-use/combine |sAdapterNameIsReady| and |mAdapterIsReady|?

Comment 10

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

> 
> ::: dom/bluetooth/BluetoothService.cpp
> @@ +796,5 @@
> > +BluetoothService::TryFiringAdapterAdded()
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  if (IsToggling() || sIsAdapterAddedFired || !mAdapterIsReady) {
> 
> Question: I was wondering if variable |sIsAdapterAddedFired| is needed? Am I
> missing something here?
> 

sIsAdapterAddedFired is not necessary. My bad.

> On the other hands, we might get multiple events of 'AdapterAdded' after
> enabling Bluetooth on BlueZ. If we check the value of |sIsAdpaterAddedFired|
> here, we will fire the event for the first time. Is it what we expected?
> 

Good point. This problem won't exist since sIsAdapterAddedFired is gone.

> @@ +805,5 @@
> > +                         NS_LITERAL_STRING(KEY_MANAGER), true);
> > +  DistributeSignal(signal);
> > +
> > +  sIsAdapterAddedFired = true;
> > +  mAdapterIsReady = false;
> 
> Question: The name is a bit confusing to me. We reset |mAdapterIsReady| to
> its default value (false) here, however, the adapter is still ready to use.
> How about moving it to StopInternal? Does it make sense to you?
> 

I would like to keep the "setting mAdapterIsReady to false" happening in BluetoothService.cpp instead of in each derived class (BluetoothDBusService.cpp and BluetoothServiceBluedroid.cpp). I'll put it in function BluetoothService::StartStopBluetooth().

> ::: dom/bluetooth/BluetoothService.h
> @@ +321,5 @@
> > +   * Below 2 function/variable are used for ensuring event 'AdapterAdded' will
> > +   * be fired after event 'Enabled'.
> > +   */
> > +  void TryFiringAdapterAdded();
> > +  bool mAdapterIsReady;
> 
> I'd like to make |mAdapterIsReady| as a private/protected member and create
> a setter function for accessing its value.

You caught me.

> 
> ::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
> @@ +106,5 @@
> > +    }
> > +
> > +    // Try to fire event 'AdapterAdded' to fit the original behaviour when
> > +    // we used BlueZ as backend.
> > +    BluetoothService* bs = BluetoothService::Get();
> 
> nit: Please make sure that |bs| is not a nullptr before using it.
> 
> ::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
> @@ +727,3 @@
> >      // Notify BluetoothManager whenever adapter name is ready.
> >      if (!propertyValue.get_nsString().IsEmpty()) {
> >        sAdapterNameIsReady = true;
> 
> Question: Can we re-use/combine |sAdapterNameIsReady| and |mAdapterIsReady|?

If we want to use mAdapterIsReady here, that means we have to write a getter of mAdapterIsReady, then we'll have both setter / getter of mAdapterIsReady (and then it would be no difference with using a public member variable). I'd prefer letting it stay as-is.

Thanks for looking into the patch. This patch was awful and I shouldn't even send to review in a rush.

Comment 11

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

Attachment: patch 2: v4: Ensure event 'AdapterAdded' will be fired after 'Enabled' being fired

* Updated based on Gina's review comments.

Comment 12

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8339789 [details] [diff] [review]
patch 2: v4: Ensure event 'AdapterAdded' will be fired after 'Enabled' being fired

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

Thanks for your quick response. I think we're ready to land the patch. :)

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +106,5 @@
> +    }
> +
> +    // Try to fire event 'AdapterAdded' to fit the original behaviour when
> +    // we used BlueZ as backend.
> +    BluetoothService* bs = BluetoothService::Get();

NS_ENSURE_TRUE(bs, NS_ERROR_FAILURE);

Comment 13

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

Attachment: patch 2: final: Ensure event 'AdapterAdded' will be fired after 'Enabled' being fired, r=gyeh

* nits picked.

Comment 14

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

https://hg.mozilla.org/integration/b2g-inbound/rev/097a3702af4f
https://hg.mozilla.org/integration/b2g-inbound/rev/b064b9271e0e

Comment 15

[Carsten Book [:Tomcat]]

hi Eric, i had to backout this 2 changesets in https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=acb441feff28 since it was causing build bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=31213566&tree=B2g-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=31213625&tree=B2g-Inbound

Comment 16

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

Sorry, my bad. Thank you, Carsten.

Comment 17

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

try: https://tbpl.mozilla.org/?tree=Try&rev=6e05db456305

Comment 18

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

try: https://tbpl.mozilla.org/?tree=Try&rev=8c4597e31af8

Comment 19

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

oops, I have to update the try server link again.

https://tbpl.mozilla.org/?tree=Try&rev=0eeae75bf6ed

Comment 20

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

https://tbpl.mozilla.org/?tree=Try&rev=22f786e963b5

Comment 21

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

https://hg.mozilla.org/integration/b2g-inbound/rev/31306f1b7891
https://hg.mozilla.org/integration/b2g-inbound/rev/d27d0489b689

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31306f1b7891
https://hg.mozilla.org/mozilla-central/rev/d27d0489b689