976943 - [bluedroid] Error handing for Bluetooth*Manager::Connect() and Bluetooth*Manager::Disconnect()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Currently we don't handle the error which occurs during the whole process of BluetoothHfpManager::Connect() very well. The worst case may block the connect/disconnect request queue of BluetoothProfileController.

Comment 1

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

Attachment: patch 1: v1: add error handling and make sure onConnect() would be called anyway

Comment 2

[Ben Tian (inactive)]

Comment on attachment 8381939 [details] [diff] [review]
patch 1: v1: add error handling and make sure onConnect() would be called anyway

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

Please see my questions below. Also I think BluetoothHfpManager::Disconnect() should also be revised. Is there a patch 2 for it?

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +170,4 @@
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(!mDeviceAddress.IsEmpty());
>    MOZ_ASSERT(mProfilesIndex == -1);
> +  MOZ_ASSERT(mProfiles.Length() > 0);

Given the following if statement, is the assertion still necessary?

@@ +223,5 @@
>  BluetoothProfileController::OnConnect(const nsAString& aErrorStr)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (mProfilesIndex >= 0) {

Should the if wrap the following log printing and Next() call? If so, use NS_ENSURE_TRUE_VOID(mProfilesIndex >= 0) instead.

Comment 3

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

(In reply to Ben Tian [:btian] from comment #2)
> Comment on attachment 8381939 [details] [diff] [review]
> patch 1: v1: add error handling and make sure onConnect() would be called
> anyway
> 
> Review of attachment 8381939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my questions below. Also I think
> BluetoothHfpManager::Disconnect() should also be revised. Is there a patch 2
> for it?

Yes, actually we need more patches for Bluetooth*Manager::Connect()/Bluetooth*Manager::Disconnect().

> 
> ::: dom/bluetooth/BluetoothProfileController.cpp
> @@ +170,4 @@
> >    MOZ_ASSERT(NS_IsMainThread());
> >    MOZ_ASSERT(!mDeviceAddress.IsEmpty());
> >    MOZ_ASSERT(mProfilesIndex == -1);
> > +  MOZ_ASSERT(mProfiles.Length() > 0);
> 
> Given the following if statement, is the assertion still necessary?
> 

It can let me sense the problem more easily in debug build.

> @@ +223,5 @@
> >  BluetoothProfileController::OnConnect(const nsAString& aErrorStr)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  if (mProfilesIndex >= 0) {
> 
> Should the if wrap the following log printing and Next() call? If so, use
> NS_ENSURE_TRUE_VOID(mProfilesIndex >= 0) instead.

I don't want to change the logic in this function. The only reason I did this is mProfilesIndex may be less than 0 so I wrap the BT_LOGR_PROFILE() to prevent from crash. As you can see from the current code, next profile connect/disconnect request would still be processed by Next() even when aErrorStr is not empty.

Comment 4

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

Attachment: patch 1: v2: Add error handling for connect/disconnect

* Updated according to Ben's feedback and offline discussion

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8382096 [details] [diff] [review]
patch 1: v2: Add error handling for connect/disconnect

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

BluetoothDBusService.cpp should also apply the StartSession() name change as well.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +191,5 @@
> +BluetoothProfileController::EndSession()
> +{
> +  MOZ_ASSERT(mRunnable && mCallback);
> +
> +  // The action has been completed, so the dom request should be replied then

nit: capitalize DOM to conform with EndSession() comment in BluetoothProfileController.h

@@ +251,4 @@
>  void
>  BluetoothProfileController::OnDisconnect(const nsAString& aErrorStr)
>  {
> +  OnConnect(aErrorStr);

The redirection is a little confusing for the function name. I suggest to open a follow-up to merge OnConnect() and OnDisconnect() of profile controller into one function.

Comment 6

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

(In reply to Ben Tian [:btian] from comment #5)
> Comment on attachment 8382096 [details] [diff] [review]
> patch 1: v2: Add error handling for connect/disconnect
> 
> Review of attachment 8382096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BluetoothDBusService.cpp should also apply the StartSession() name change as
> well.
> 

oops, thanks for reminding.

> ::: dom/bluetooth/BluetoothProfileController.cpp
> @@ +191,5 @@
> > +BluetoothProfileController::EndSession()
> > +{
> > +  MOZ_ASSERT(mRunnable && mCallback);
> > +
> > +  // The action has been completed, so the dom request should be replied then
> 
> nit: capitalize DOM to conform with EndSession() comment in
> BluetoothProfileController.h
> 

ok

> @@ +251,4 @@
> >  void
> >  BluetoothProfileController::OnDisconnect(const nsAString& aErrorStr)
> >  {
> > +  OnConnect(aErrorStr);
> 
> The redirection is a little confusing for the function name. I suggest to
> open a follow-up to merge OnConnect() and OnDisconnect() of profile
> controller into one function.

I'll attach another patch for this.

Comment 7

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

Attachment: patch 1: v3: Add error handling for connect/disconnect

* nits picked and BluetoothDBusService.cpp is modified.

Comment 8

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

Attachment: patch 1: v4: Add error handling for connect/disconnect

* Updated per offline discussion.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8384423 [details] [diff] [review]
patch 1: v4: Add error handling for connect/disconnect

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

LGTM.

Comment 10

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

Comment on attachment 8384423 [details] [diff] [review]
patch 1: v4: Add error handling for connect/disconnect

Hi Gina,

Please review my patch. Thanks.

Comment 11

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

Attachment: patch 2: v1: Replace OnConnect/OnDisconnect with one function

In BluetoothProfileController, it's meaningless to separate OnConnect and OnDisconnect since the following steps would be the same for both cases. Therefore I introduced a function called NotifyCompletion() for each profile manager to report the completion of connecting/disconnecting operations.

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8384466 [details] [diff] [review]
patch 2: v1:  Replace OnConnect/OnDisconnect with one function

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

Can we further merge OnConnect() and OnDisconnect() functions in BluetoothProfileManagerBase into OnCompletion()? Only bluez/BluetoothHfpManager requires slightly different OnConnect and OnDisconnect functions and I think they can be merged as below since only OnSocketConnectSuccess passes empty string into OnConnect.

BluetoothHfpManager::OnCompletion(const nsAString& aErrorStr)
{
  MOZ_ASSERT(NS_IsMainThread());

  if (!IsConnected()) {
    mSocket = nullptr;
    Listen();
  }

  ...
}

Comment 13

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

Attachment: patch 2: v2: Replace OnConnect/OnDisconnect with one function

* Keep OnConnect/OnDisconnect of Bluetooth*Manager as is since they may be implemented differently at profile level.

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8384541 [details] [diff] [review]
patch 2: v2: Replace OnConnect/OnDisconnect with one function

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

f=me with comment addressed.

::: dom/bluetooth/BluetoothHidManager.cpp
@@ -186,5 @@
>  
> -  /**
> -   * On the one hand, notify the controller that we've done for outbound
> -   * connections. On the other hand, we do nothing for inbound connections.
> -   */

Should we keep this comment?

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ -661,5 @@
>  
> -  /**
> -   * On the one hand, notify the controller that we've done for outbound
> -   * connections. On the other hand, we do nothing for inbound connections.
> -   */

Ditto.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ -1298,5 @@
>  
> -  /**
> -   * On the one hand, notify the controller that we've done for outbound
> -   * connections. On the other hand, we do nothing for inbound connections.
> -   */

Ditto.

::: dom/bluetooth/bluez/BluetoothA2dpManager.cpp
@@ -224,5 @@
>  
> -  /**
> -   * On the one hand, notify the controller that we've done for outbound
> -   * connections. On the other hand, we do nothing for inbound connections.
> -   */

Ditto.

Comment 15

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

Attachment: patch 2: v3: Replace OnConnect/OnDisconnect with one function

* nits picked. Gina, Would you mind taking a look?

Comment 16

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

Comment on attachment 8384423 [details] [diff] [review]
patch 1: v4: Add error handling for connect/disconnect

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

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +232,5 @@
> +
> +  if (mProfilesIndex >= (int)mProfiles.Length()) {
> +    EndSession();
> +    return;
> +  }

This part is redundant, right? We have similar checks in Next(), and |mSuccess| might be incorrect since it hasn't updated yet and goes to EndSession().

@@ +242,2 @@
>      mSuccess = true;
>    }

mSuccess = aErrorStr.IsEmpty();

@@ +253,5 @@
> +  if (mProfilesIndex >= (int)mProfiles.Length()) {
> +    EndSession();
> +    return;
> +  }
> +

ditto.

@@ +262,2 @@
>      mSuccess = true;
>    }

ditto.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +592,5 @@
>    StringToBdAddressType(aDeviceAddress, &remoteAddress);
> +
> +  bt_status_t result = sBtA2dpInterface->connect(&remoteAddress);
> +  if (BT_STATUS_SUCCESS != result) {
> +    BT_LOGR("Failed to disconnect: %x", result);

nit: s/disconnect/connect/

Comment 17

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

Comment on attachment 8385099 [details] [diff] [review]
patch 2: v3: Replace OnConnect/OnDisconnect with one function

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

r+ with nit picked.

::: dom/bluetooth/BluetoothProfileController.h
@@ +96,4 @@
>    void EndSession();
>  
>    /**
> +   * It would be invoked after connect/disconnect operation completes.

nit: after ... is completed.

Comment 18

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

Attachment: patch 1: final: Add error handling for connect/disconnect, f=btian, r=gyeh

* updated per Gina's comment.

Comment 19

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

Comment on attachment 8385216 [details] [diff] [review]
patch 1: final: Add error handling for connect/disconnect, f=btian, r=gyeh

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

Bug 951634 is going to land. Please rebase the patch before pushing it.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +228,5 @@
>  void
>  BluetoothProfileController::OnConnect(const nsAString& aErrorStr)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +

Let's add an assertion for checking array length as what we did in OnDisconnect().

Comment 20

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

Attachment: patch 2: final: Replace OnConnect/OnDisconnect with one function, f=btian, r=gyeh

Thank you both!

Comment 21

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

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #19)
> Comment on attachment 8385216 [details] [diff] [review]
> patch 1: v5: Add error handling for connect/disconnect, f=btian
> 
> Review of attachment 8385216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bug 951634 is going to land. Please rebase the patch before pushing it.

ok.

> 
> ::: dom/bluetooth/BluetoothProfileController.cpp
> @@ +228,5 @@
> >  void
> >  BluetoothProfileController::OnConnect(const nsAString& aErrorStr)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +
> 
> Let's add an assertion for checking array length as what we did in
> OnDisconnect().

Yeah, that would be fixed in patch 2.

Comment 22

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

try:

https://tbpl.mozilla.org/?tree=Try&rev=82b3a825219d

Comment 23

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

https://hg.mozilla.org/integration/b2g-inbound/rev/98e130e325df
https://hg.mozilla.org/integration/b2g-inbound/rev/9f916baff01a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98e130e325df
https://hg.mozilla.org/mozilla-central/rev/9f916baff01a