Closed Bug 976943 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: echou, Assigned: echou)

Details

Attachments

(2 files, 7 obsolete files)

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.
Assignee: nobody → echou
Attachment #8381939 - Flags: review?(gyeh)
Attachment #8381939 - Flags: feedback?(btian)
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.
(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.
Summary: [bluedroid] Error handing for BluetoothHfpManager::Connect() → [bluedroid] Error handing for Bluetooth*Manager::Connect() and Bluetooth*Manager::Disconnect()
* Updated according to Ben's feedback and offline discussion
Attachment #8381939 - Attachment is obsolete: true
Attachment #8381939 - Flags: review?(gyeh)
Attachment #8381939 - Flags: feedback?(btian)
Attachment #8382096 - Flags: review?(gyeh)
Attachment #8382096 - Flags: feedback?(btian)
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.
(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.
* nits picked and BluetoothDBusService.cpp is modified.
Attachment #8382096 - Attachment is obsolete: true
Attachment #8382096 - Flags: review?(gyeh)
Attachment #8382096 - Flags: feedback?(btian)
Attachment #8384410 - Flags: feedback?(btian)
* Updated per offline discussion.
Attachment #8384410 - Attachment is obsolete: true
Attachment #8384410 - Flags: feedback?(btian)
Attachment #8384423 - Flags: feedback?(btian)
Comment on attachment 8384423 [details] [diff] [review]
patch 1: v4: Add error handling for connect/disconnect

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

LGTM.
Attachment #8384423 - Flags: feedback?(btian) → feedback+
Comment on attachment 8384423 [details] [diff] [review]
patch 1: v4: Add error handling for connect/disconnect

Hi Gina,

Please review my patch. Thanks.
Attachment #8384423 - Flags: review?(gyeh)
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.
Attachment #8384466 - Flags: feedback?(btian)
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();
  }

  ...
}
* Keep OnConnect/OnDisconnect of Bluetooth*Manager as is since they may be implemented differently at profile level.
Attachment #8384466 - Attachment is obsolete: true
Attachment #8384466 - Flags: feedback?(btian)
Attachment #8384541 - Flags: feedback?(btian)
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.
Attachment #8384541 - Flags: feedback?(btian) → feedback+
* nits picked. Gina, Would you mind taking a look?
Attachment #8384541 - Attachment is obsolete: true
Attachment #8385099 - Flags: review?(gyeh)
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 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.
Attachment #8385099 - Flags: review?(gyeh) → review+
* updated per Gina's comment.
Attachment #8384423 - Attachment is obsolete: true
Attachment #8384423 - Flags: review?(gyeh)
Attachment #8385216 - Flags: review?(gyeh)
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().
Attachment #8385216 - Flags: review?(gyeh) → review+
(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.
Attachment #8385216 - Attachment description: patch 1: v5: Add error handling for connect/disconnect, f=btian → patch 1: final: Add error handling for connect/disconnect, f=btian, r=gyeh
Attachment #8385216 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/98e130e325df
https://hg.mozilla.org/mozilla-central/rev/9f916baff01a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
You need to log in before you can comment on or make changes to this bug.