Closed Bug 951634 Opened 6 years ago Closed 6 years ago

[Bluetooth][Bluedroid] Add TimerCallback in Profile Controller to guard callback never returned

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)

People

(Reporter: wachen, Assigned: shawnjohnjr)

References

Details

(Whiteboard: burirun1.3-1)

Attachments

(5 files, 22 obsolete files)

769.35 KB, text/plain
Details
24.13 KB, application/octet-stream
Details
16.05 KB, patch
Details | Diff | Splinter Review
2.92 KB, patch
Details | Diff | Splinter Review
22.38 KB, patch
Details | Diff | Splinter Review
It is found in last v1.3 mako build in pvt

STR:
1. Launch settings
2. Pair with an a2dp earphone
3. Turn on and off bluetooth for 3 times (or more)

Expected Result:
Device should connect with earphone very fast

Actual Result:
Device connect with earphone slowly, even if it is connected, no a2dp connection provided and you can't disconnect it. (you can unpair it though)

And other kind of bt earphone won't be able to connect again (but pairing is fine)
found in full run of buri v1.3
added burirun1.3-1
Whiteboard: burirun1.3-1
Summary: [Bluetooth][Settings] Unable to disconnect device after some actions → [Bluetooth][Settings][Bluedroid] Unable to disconnect device after some actions
Assignee: nobody → shuang
blocking-b2g: --- → 1.3?
Summary: [Bluetooth][Settings][Bluedroid] Unable to disconnect device after some actions → [Bluetooth][Bluedroid] Unable to disconnect device after some actions
Two problems here:
1. sControllerArray does not clean up after disable bluetooth. So if there is a new outgoing connection request, sControllerArray will be added one element, and if hfp/a2dp connection request callback never return. sControllerArray.RemoveElement will not never be called, and sControllerArray length will not be 1. This leads BluetooothProfileController never start().
https://github.com/mozilla/gecko-dev/blob/master/dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp#L1217

2. Even sControllerArray was cleaned up, we still need to handle cases that bluez never send back connection fail dbus signal or bluedroid stack never call ConnectionStatusChanged callback.
Even if I clean up sControllerArray when disabled bluetooth, during test
I will hit MOZ_ASSERT, |MOZ_ASSERT(aController && !mController);| in BluetoothA2dpManager::Connect().
STR:
1. Turn on/off Bluetooth a few times
2. When HFP connection is requesting (from phone to headset), turn off Bluetooth headset and turn on headset again
3. Bluetooth headset tries to reconnect again (connection request from headset to phone)
4. HFP connected
5. Execute BluetoothProfileController::Next() (A2DP)
6. Hit assertion
> Even if I clean up sControllerArray when disabled bluetooth, during test
> I will hit MOZ_ASSERT, |MOZ_ASSERT(aController && !mController);| in
> BluetoothA2dpManager::Connect().
This is what I saw:
a. HFP connected and A2DP is connecting
b. Bluetooth turned off during that time, A2DP is connecting
c. Bluetooth turned on, 
d. Bluetooth HFP is connecting but stuck
e. Turn off headset and turn on headset to reconnect to phone
f. HFP connected, and which trigger a2dp connection
g. Hit assertion due to previous A2DP outgoing connection was made and mController has been assigned.
> f. HFP connected, and which trigger a2dp connection
> g. Hit assertion due to previous A2DP outgoing connection was made and
> mController has been assigned.
We can reset |mController| after bluetooth disabled. Or we don't have to assert here since mController will be re-assigned again in |Connect()|. IMHO, HFP/A2DP both can hit the same assertion, but shall not be a problem.

However, even if we fix this part, we still have chances to hit nothing update from stack.

12-19 05:25:16.669 I/GeckoBluetooth(  178): ProcessUnknownAt: [+XAPL=047F-22-26,1]
12-19 05:25:16.679 I/GeckoBluetooth(  178): ProcessUnknownAt: [+XEVENT=USER-AGENT,COM.PLANTRONICS,PLT_Legend,113,34.38,f3dfc42d37810d4c825c1f63fdd566cb]
12-19 05:25:17.970 E/bt-btif (  178): bta_av_rc_create ACP handle exist for shdl:0
12-19 05:25:17.970 W/bt-btif (  178): btif_av_state_opening_handler : unhandled event:BTA_AV_PENDING_EVT
12-19 05:25:18.000 E/bt-btif (  178): use_rc:1
12-19 05:25:18.000 E/bt-btif (  178): bta_av_rc_opened rcb[0] shdl:1 lidx:3/0
12-19 05:25:18.050 W/bt-sdp  (  178): process_service_search_attr_rsp
12-19 05:25:18.070 E/bt-avp  (  178): bad play state
12-19 05:25:18.070 E/bt-btif (  178): send_metamsg_rsp: failed to build metamsg response. status: 0x01

I'm looking into a2dp stack part first.
triage: 1.3+
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Even I add extra timer to stop 'connecting' state and restore connection. I still have chance to hit problems (if user tries to disconnect), ProfileController Array will still add element. I will still work on this bug.
Since I recently focus on FxOS-1.3-CS-release blocking bugs, this bug is delay. I don't have confidence to fix before end of Friday.
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.3 C3/1.4 S3(31jan)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #11)
> Created attachment 8361537 [details]
> btsnoop_hci(1).cfa
Based on sniiffer trace, only AVRCP get connected, but not a2dp. This is why we can see log 
'12-19 05:25:18.050 W/bt-sdp  (  178): process_service_search_attr_rsp
 12-19 05:25:18.070 E/bt-avp  (  178): bad play state'
PM triaged this bug and it be a 1.3 blocker.
Attached patch part2.patch (WIP) (obsolete) — Splinter Review
I'm still working on this patch. Even though I already replied error, the problem still not been resolved.
It's possible that timer is still alive but we already turn off Bluetooth and turn on again.
01-24 19:25:26.017 I/GeckoBluetooth( 2844): ConnectionTimeout: !!!! after 10 sec ConnectionTimeout cancel CONNECT  !!!!
01-24 19:25:26.017 F/MOZ_Assert( 2844): Assertion failure: mReplyRunnable.IsPending(), at ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/dom/bluetooth/ipc/BluetoothParent.cpp:300
It looks like I need to inherit CancelableTask, whever get connected.
(In reply to Walter Chen[:ypwalter][:wachen] from comment #1)
> found in full run of buri v1.3
> added burirun1.3-1

Even if this is found in Buri full run, it is actually in a mako device.
The reason behind this is:
There are 2 different versions of bluetooth - BlueZ & BlueDroid. We need to make sure both version are fine. However, Buri is using BlueZ and Mako is using BlueDroid. In that case, we tested both mako and buri.
Renominate as non-blocking 1.3 according to comment 17. No devices which takes 1.3 will use Bluedroid as its Bluetooth backend.
blocking-b2g: 1.3+ → 1.3?
I still see segmentfault after sending Dbus reply from DelayedCancelConnectionTask

01-29 11:43:27.399 I/GeckoBluetooth(13596): Run: !!!! after 10 sec DelayedCancelConnectionTask::  Run, index: 1 !!!!
01-29 11:43:27.399 I/GeckoBluetooth(13596): Run: !!!! after 10 sec Run cancel CONNECT  !!!!


01-29 11:43:27.779 I/GeckoBluetooth(13596): OnConnect: [A2DP] <A2dpConnectionError>
01-29 11:43:27.779 I/Gecko   (13596): [Parent 13596] WARNING: A2dpConnectionError: file ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/dom/bluetooth/BluetoothProfileController.cpp, line 317
01-29 11:43:27.779 I/GeckoBluetooth(13596): OnConnect: ! OnConnect Cancel, index: 1 !
01-29 11:43:27.779 I/GeckoBluetooth(13596): OnConnect: ! OnConnect mDelayedCancelConnectionTask is empty !
01-29 11:43:27.779 I/Gecko   (13596): [Parent 13596] WARNING: NS_ENSURE_TRUE(!(sControllerArray.IsEmpty())) failed: file ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp, line 1196


#0  nsRefPtr<mozilla::IncrementalFinalizeRunnable>::assign_assuming_AddRef (this=0xacb2f990, newPtr=0x0) at ../../dist/include/nsAutoPtr.h:888
#1  0xb4be29e4 in mozilla::dom::bluetooth::BluetoothRequestParent::ReplyRunnable::Run (this=0xacb3deb0)
    at ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/dom/bluetooth/ipc/BluetoothParent.cpp:47
#2  0xb43f0e4a in ProcessNextEvent (result=0xbecd87f7, mayWait=<optimized out>, this=0xb6b34700)
    at ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/xpcom/threads/nsThread.cpp:637
#3  nsThread::ProcessNextEvent (this=0xb6b34700, mayWait=<optimized out>, result=0xbecd87f7) at ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/xpcom/threads/nsThread.cpp:568
#4  0xb43ab338 in NS_ProcessNextEvent (thread=0xb6b34700, mayWait=<optimized out>) at ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/xpcom/glue/nsThreadUtils.cpp:263
#5  0xb4588ef4 in mozilla::ipc::MessagePump::Run (this=0xb6b01e50, aDelegate=0xb6b4e1a0) at ../../../../../workspace1/bluedroid-b2g-inbound/b2g-inbound/ipc/glue/MessagePump.cpp:95
(In reply to Eric Chou [:ericchou] [:echou] (away from 1/30 ~ 2/4) from comment #18)
> Renominate as non-blocking 1.3 according to comment 17. No devices which
> takes 1.3 will use Bluedroid as its Bluetooth backend.

Clarifying for triage decision: all first wave devices and currently known chipsets will stay on BlueZ for 1.3, recommending that we not block on this.
Comment on attachment 8367210 [details] [diff] [review]
Bug951634-wip.patch 2nd version (CanableTask version)

I fixed problems that this patch introduced, but I need more tests since this change is more tricky.
Attachment #8367210 - Attachment description: Bug951634-wip.patch 2nd version → Bug951634-wip.patch 2nd version (CanableTask version)
moving to 1.4 per comment 21.
blocking-b2g: 1.3? → 1.4?
Extra things I observed:
During testing my WIP patch with bluedroid stack, I found that if the remote headset turns off, HFP profile cannot connect to the remote side, however OnDisconnect get called without any error string, which is not expected here. The original design that we shall expect OnConnect with error string returned. Somehow, we try to map bluedroid HFP state-change (from connecting to disconnected).
Since mSuccess = true, even if A2DP also fail to connect with the headset, mSuccess will not be updated.
https://github.com/mozilla/gecko-dev/blob/master/dom/bluetooth/BluetoothProfileController.cpp#L242

This leads gecko reply success to Gaia.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #27)
> Extra things I observed:
> During testing my WIP patch with bluedroid stack, I found that if the remote
> headset turns off, HFP profile cannot connect to the remote side, however
> OnDisconnect get called without any error string, which is not expected
> here. The original design that we shall expect OnConnect with error string
> returned. Somehow, we try to map bluedroid HFP state-change (from connecting
> to disconnected).
> Since mSuccess = true, even if A2DP also fail to connect with the headset,
> mSuccess will not be updated.
> https://github.com/mozilla/gecko-dev/blob/master/dom/bluetooth/
> BluetoothProfileController.cpp#L242
> 
> This leads gecko reply success to Gaia.
For this problem i shall make another patch to fix this. If the current state is connecting, transits to disconnected, OnConnect with error shall be returned.
https://github.com/mozilla/gecko-dev/blob/master/dom/bluetooth/bluedroid/BluetoothHfpManager.cpp#L756
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #29)
> Created attachment 8370676 [details] [diff] [review]
> Bug 951634 - [bluedroid] Outgoing connection callback shall trigger
> OnConnect instead of OnDisconnect
Previous state shall be stored in |ProcessConnectionState()|, however, bluedroid would not report state =  BTHF_CONNECTION_STATE_CONNECTING, so we cannot depend on Connection callback.
Fix build problem on nexus 4 bluedroid.
Attachment #8370676 - Attachment description: Bug 951634 - [bluedroid] Outgoing connection callback shall trigger OnConnect instead of OnDisconnect → Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect
Attachment #8371313 - Flags: review?(echou)
Attachment #8371313 - Flags: feedback?(gyeh)
Attachment #8371313 - Attachment is obsolete: true
Attachment #8371313 - Flags: review?(echou)
Attachment #8371313 - Flags: feedback?(gyeh)
Comment on attachment 8371348 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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

Some functions are defined but haven't been used. Did you miss any changes?
Check my comments below for details.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +46,5 @@
>    MOZ_ASSERT(aCallback);
> +  MOZ_ASSERT(!mTimer);
> +
> +  nsresult rv;
> +  nsCOMPtr<nsITimer> tmpTimer(do_CreateInstance(NS_TIMER_CONTRACTID, &rv));

Please check the return value |rv| and add a warning if it failed to create a timer.

@@ +216,5 @@
>    BT_LOGR_PROFILE(mProfiles[mProfilesIndex], "");
>  
> +  mCheckProfileStatusCallback->SetCurProfileIndex(mProfilesIndex);
> +  mTimer->InitWithCallback(mCheckProfileStatusCallback, kTimerInterval,
> +                           nsITimer::TYPE_ONE_SHOT);

|mTimer| might be nulled out somehow. I suggest to check the value and create a new one if needed.

@@ +231,3 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(!mDeviceAddress.IsEmpty());

It would be great if we can add an assertion for |mTimer|.

@@ +237,5 @@
>      BT_LOGR_PROFILE(mProfiles[mProfilesIndex], "");
>  
> +    mCheckProfileStatusCallback->SetCurProfileIndex(mProfilesIndex);
> +    mTimer->InitWithCallback(mCheckProfileStatusCallback, 10000,
> +                             nsITimer::TYPE_ONE_SHOT);

ditto.

::: dom/bluetooth/BluetoothProfileController.h
@@ +127,5 @@
>     */
>    void OnDisconnect(const nsAString& aErrorStr);
>  
> +  void GiveupAndContinue();
> +  bool IsProfileFinished(uint32_t aIndex);

Please add comments for these new functions.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +658,5 @@
> +BluetoothA2dpManager::ResetController()
> +{
> +  NS_ENSURE_TRUE_VOID(mController);
> +  mController = nullptr;
> +}

This function isn't used. Is there any changes are missed?

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +1290,5 @@
> +BluetoothHfpManager::ResetController()
> +{
> +  NS_ENSURE_TRUE_VOID(mController);
> +  mController = nullptr;
> +}

ditto.
Comment on attachment 8372046 [details] [diff] [review]
Bug 951634 - Patch 1: Add delay reply in Profile Controller (CancelableTask)

I've fixed my version's bug but seems Gina's patch is better so i left this patch.
Attachment #8371348 - Flags: review?(echou)
Attachment #8371348 - Flags: feedback?(gyeh)
Attachment #8372099 - Attachment is obsolete: true
Attachment #8372099 - Flags: feedback?(gyeh)
Attachment #8372102 - Flags: review?(echou)
Attachment #8372102 - Flags: feedback?(gyeh)
Attachment #8372102 - Flags: feedback?(btian)
Summary: [Bluetooth][Bluedroid] Unable to disconnect device after some actions → [Bluetooth][Bluedroid] Add TimerCallback in Profile Controller to guard callback never returned
Attachment #8370676 - Flags: review?(gyeh)
Attachment #8370676 - Flags: review?(echou)
Attachment #8370676 - Flags: feedback?(gyeh)
Attachment #8370676 - Flags: feedback?(btian)
Comment on attachment 8372102 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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

I think the patch can be further simplified by:
- making ProfileController inherit nsITimerCallback directly
- wrapping each pair of ProfileFinished flag and profile manager into a new class/struct, and keeping mProfiles as an array of the new class.  

Also I raised some questions and addressed some nits as following.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +12,5 @@
>  #include "BluetoothHidManager.h"
>  
>  #include "BluetoothUtils.h"
>  #include "mozilla/dom/bluetooth/BluetoothTypes.h"
>  

nit: extra newline

@@ +24,5 @@
>      mgr->GetName(name);                              \
>      BT_LOGR("[%s] " msg, name.get(), ##__VA_ARGS__); \
>    } while(0)
>  
> +static uint16_t kTimerInterval = 10000;

How did you determine the timeout value?

@@ +46,5 @@
>    MOZ_ASSERT(aCallback);
> +  MOZ_ASSERT(!mTimer);
> +
> +  nsresult rv;
> +  nsCOMPtr<nsITimer> tmpTimer(do_CreateInstance(NS_TIMER_CONTRACTID, &rv));

nit: name 'timer' instead of 'tmpTimer'.
--
BluetoothService.cpp also uses timer. Please refer to it to create timer in a consistent way:

    nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
    MOZ_ASSERT(timer);

    if (timer) {...

@@ +48,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsITimer> tmpTimer(do_CreateInstance(NS_TIMER_CONTRACTID, &rv));
> +  NS_ENSURE_TRUE_VOID(tmpTimer);
> +  NS_ENSURE_SUCCESS_VOID(rv);

Delete tmpTimer if NS_FAILED(rv).

@@ +49,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsITimer> tmpTimer(do_CreateInstance(NS_TIMER_CONTRACTID, &rv));
> +  NS_ENSURE_TRUE_VOID(tmpTimer);
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +  mTimer.swap(tmpTimer);

Why swap? Will original mTimer be accessed later?

@@ +242,5 @@
>      BT_LOGR_PROFILE(mProfiles[mProfilesIndex], "");
>  
> +    mCheckProfileStatusCallback->SetCurProfileIndex(mProfilesIndex);
> +    if (mTimer) {
> +      mTimer->InitWithCallback(mCheckProfileStatusCallback, 12000,

Why does the timeout become 12000 here?

::: dom/bluetooth/BluetoothProfileController.h
@@ +9,5 @@
>  
>  #include "BluetoothUuid.h"
>  #include "nsAutoPtr.h"
>  #include "mozilla/RefPtr.h"
> +#include "nsITimer.h"

nit: declare in alphabetic order.

@@ +151,5 @@
>  
>    bool mSuccess;
>    int8_t mProfilesIndex;
>    nsTArray<BluetoothProfileManagerBase*> mProfiles;
> +  nsTArray<bool> mProfileFinished;

nit: mProfile's'Finished

::: dom/bluetooth/BluetoothProfileManagerBase.h
@@ +68,5 @@
>     * Returns string of profile name
>     */
>    virtual void GetName(nsACString& aName) = 0;
> +
> +  virtual void Reset() = 0;

nit: declare Reset() before GetName() and write comment to explain usage.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +571,5 @@
>  BluetoothA2dpManager::Connect(const nsAString& aDeviceAddress,
>                                BluetoothProfileController* aController)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(!aDeviceAddress.IsEmpty());

Keep MOZ_ASSERT(aController).

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +1222,5 @@
>  void
>  BluetoothHfpManager::Connect(const nsAString& aDeviceAddress,
>                               BluetoothProfileController* aController)
>  {
>    MOZ_ASSERT(NS_IsMainThread());

ditto.

::: dom/bluetooth/bluez/BluetoothA2dpManager.cpp
@@ +54,5 @@
> +BluetoothA2dpManager::BluetoothA2dpManager()
> +{
> +  Reset();
> +}
> +

nit: write constructor before Reset() as bluez/BluetoothA2dpManager.cpp and BluetoothHidManager.cpp
Comment on attachment 8370676 [details] [diff] [review]
Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

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

Give f- since the name 'mConnectionPrevState' appears general but disconnection doesn't use it at all. I think a specific IsHfpConnectingFlag is clearer.
Attachment #8370676 - Flags: feedback?(btian) → feedback-
Attachment #8372102 - Flags: feedback?(btian) → feedback-
(In reply to Ben Tian [:btian] from comment #40)
> Comment on attachment 8370676 [details] [diff] [review]
> Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect
> instead of OnDisconnect
> 
> Review of attachment 8370676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Give f- since the name 'mConnectionPrevState' appears general but
> disconnection doesn't use it at all. I think a specific IsHfpConnectingFlag
> is clearer.

I saved previous state is because we probably need to compare previous state if needed somewhere else.
I think we shall check new state and previous state. Although I did not add this kind of check in this patch. Like BluetoothA2dpManager, it would try to validate new State and previous state are valid.
My original purpose is to extend to do validation here. Changing to 'IsHfpConnectingFlag' is more specific to this patch.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #41)
> I saved previous state is because we probably need to compare previous state
> if needed somewhere else.
> I think we shall check new state and previous state. Although I did not add
> this kind of check in this patch. Like BluetoothA2dpManager, it would try to
> validate new State and previous state are valid.
Then we should assign prevState each time state is changed (e.g., in ProcessConnectionState()). Also does DISCONNECTED -> DISCONNECTED imply connection failure?
Attachment #8370676 - Attachment is obsolete: true
Attachment #8370676 - Flags: review?(echou)
Attachment #8370676 - Flags: feedback?(gyeh)
Attachment #8372102 - Attachment is obsolete: true
Attachment #8372102 - Flags: feedback?(gyeh)
Comment on attachment 8377359 [details] [diff] [review]
Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

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

f+ with following comment addressed.

::: dom/bluetooth/bluedroid/BluetoothHfpManager.h
@@ +157,5 @@
>    void SendLine(const char* aMessage);
>    void SendResponse(bthf_at_response_t aResponseCode);
>  
>    int mConnectionState;
> +  int mConnectionPrevState;

I prefer to name it 'mPrevConnectionState'.
Attachment #8377359 - Flags: feedback?(btian) → feedback+
Attachment #8377423 - Attachment is obsolete: true
Attachment #8377423 - Flags: feedback?(btian)
Per discussion, I removed SetCurrIndex and simplify Timer callback logic, move logic to BluetoothProfileController. Regarding mProfilesFinished array, it's very light, and I don't want to create another struct/class to maintain this. Do you think is it really necessary?
Attachment #8378834 - Flags: feedback?(btian)
Attachment #8378834 - Attachment is obsolete: true
Attachment #8378834 - Flags: feedback?(btian)
Comment on attachment 8378865 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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

f=me with following comments addressed.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +23,5 @@
>      mgr->GetName(name);                              \
>      BT_LOGR("[%s] " msg, name.get(), ##__VA_ARGS__); \
>    } while(0)
>  
> +static uint16_t kTimerInterval = 10000;

Write comment to brief how kTimerInterval value is determined.

@@ +64,5 @@
>      mTarget.service = serviceClass;
>      SetupProfiles(true);
>    }
> +
> +  mCheckProfileStatusCallback = new CheckProfileStatusCallback(this);

nit: how about move this line up to right after 'MOZ_ASSERT(mTimer)', in order to group timer-related code?

@@ +182,5 @@
> +NS_IMPL_ISUPPORTS1(CheckProfileStatusCallback, nsITimerCallback)
> +
> +NS_IMETHODIMP
> +CheckProfileStatusCallback::Notify(nsITimer* aTimer)
> +{

Add MOZ_ASSERT(mController) check here.

@@ +206,5 @@
>  
> +  if (mTimer) {
> +    mTimer->InitWithCallback(mCheckProfileStatusCallback, kTimerInterval,
> +                             nsITimer::TYPE_ONE_SHOT);
> +  }

Print error message for 'else' case.

@@ +228,5 @@
>    if (++mProfilesIndex < (int)mProfiles.Length()) {
>      BT_LOGR_PROFILE(mProfiles[mProfilesIndex], "");
>  
> +    if (mTimer) {
> +      mTimer->InitWithCallback(mCheckProfileStatusCallback, 12000,

Declare 12000 as static variable (like kTimeInterval).

@@ +230,5 @@
>  
> +    if (mTimer) {
> +      mTimer->InitWithCallback(mCheckProfileStatusCallback, 12000,
> +                               nsITimer::TYPE_ONE_SHOT);
> +    }

Print error message for 'else' case.

nit: add newline.

@@ +300,5 @@
> +void
> +BluetoothProfileController::GiveupAndContinue()
> +{
> +  if (!mCurrentProfileFinished) {
> +    BT_WARNING("TimeoutError");

How about go to OnConnect/OnDisconnect as following? This way keeps original profile logs.

if(!mCurrentProfileFinished)
{
  mProfiles[mProfilesIndex]->Reset();
  if (mConnect) {
    OnConnect(NS_LITERAL_STRING"TimeoutError"));
  } else {
    OnDisconnect(NS_LITERAL_STRING"TimeoutError"));
  }
}

@@ +303,5 @@
> +  if (!mCurrentProfileFinished) {
> +    BT_WARNING("TimeoutError");
> +    mProfiles[mProfilesIndex]->Reset();
> +    Next();
> +  }

Print error message or MOZ_ASSERT(false) for 'else' case since it shouldn't get here.

::: dom/bluetooth/BluetoothProfileController.h
@@ +13,1 @@
>  #include "mozilla/RefPtr.h"

nit: alphabetical order.

@@ +56,5 @@
>  class BluetoothProfileManagerBase;
>  class BluetoothReplyRunnable;
>  typedef void (*BluetoothProfileControllerCallback)();
>  
> +class BluetoothProfileController;

nit: move this line to right before 'class BluetoothProfileManagerBase;'

@@ +76,5 @@
> +  }
> +
> +private:
> +  nsRefPtr<BluetoothProfileController> mController;
> +  int8_t mCurProfileIndex;

Remove mCurProfileIndex since it's useless.

@@ +125,5 @@
>     */
>    void OnDisconnect(const nsAString& aErrorStr);
>  
> +  /**
> +   * It is invoked after profile has reached timeout, reset ProfileController

I think it should be 'reset profile manager'

@@ +126,5 @@
>    void OnDisconnect(const nsAString& aErrorStr);
>  
> +  /**
> +   * It is invoked after profile has reached timeout, reset ProfileController
> +   *

nit: extra comment line.
Attachment #8378865 - Flags: feedback?(btian) → feedback+
(In reply to Ben Tian [:btian] from comment #49)
> Comment on attachment 8378865 [details] [diff] [review]
> Bug 951634 - Patch 1: Add TimerCallback in Profile Controller
> 
> Review of attachment 8378865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +300,5 @@
> > +void
> > +BluetoothProfileController::GiveupAndContinue()
> > +{
> > +  if (!mCurrentProfileFinished) {
> > +    BT_WARNING("TimeoutError");
> 
> How about go to OnConnect/OnDisconnect as following? This way keeps original
> profile logs.
> 
> if(!mCurrentProfileFinished)
> {
>   mProfiles[mProfilesIndex]->Reset();
>   if (mConnect) {
>     OnConnect(NS_LITERAL_STRING"TimeoutError"));
>   } else {
>     OnDisconnect(NS_LITERAL_STRING"TimeoutError"));
>   }
> }
I think we should not directly call OnConnect/OnDisconnect in GiveupAndContinue. Because OnConnect/OnDisconnect means bluetooth back-end actually returns connection results.
Comment on attachment 8377390 [details] [diff] [review]
Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

Per offline discussion, delegate review req to Gina.
Attachment #8377390 - Flags: review?(echou) → review?(gyeh)
Comment on attachment 8379645 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Sorry, I uploaded wrong version patch.
Attachment #8379645 - Attachment is obsolete: true
Attachment #8379645 - Flags: feedback?(gyeh)
Attachment #8379645 - Flags: feedback?(btian)
Comment on attachment 8380436 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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

f=me with comment addressed. Thanks.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +308,5 @@
> +    mProfiles[mProfilesIndex]->Reset();
> +    Next();
> +  } else {
> +    MOZ_ASSERT(false);
> +  }

Some suggestions:
1) Define "TimoutError" in BluetoothProfileManagerBase.h [1] since it's a common error.
   [1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothProfileManagerBase.h#22
2) Use BT_LOGR_PROFILE to always print out error message.
3) Simplify if-else as following:
{
  MOZ_ASSERT(!mCurrentProfileFinished);
  MOZ_ASSERT(mProfilesIndex < (int)mProfiles.Length());

  BT_LOGR_PROFILE(mProfiles[mProfilesIndex], "<TimeoutError>");
  BT_WARNING("TimeoutError");

  mProfiles[mProfilesIndex]->Reset();
  Next();
}
Attachment #8380436 - Flags: feedback?(btian) → feedback+
Comment on attachment 8377390 [details] [diff] [review]
Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

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

The patch is ready to ship.
Please ignore comment 56. :)
Comment on attachment 8381289 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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

Since Bug 939672 has been landed, the patch need to be updated. Please send the review request again, thanks.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +42,5 @@
>  {
>    MOZ_ASSERT(!aDeviceAddress.IsEmpty());
>    MOZ_ASSERT(aRunnable);
>    MOZ_ASSERT(aCallback);
> +  MOZ_ASSERT(!mTimer);

No need to keep this assertion. Please remove it.

@@ +52,2 @@
>    mProfiles.Clear();
> +  mCurrentProfileFinished = false;

Just put it in member initializer list.

@@ +69,5 @@
>  
>  BluetoothProfileController::~BluetoothProfileController()
>  {
>    mProfiles.Clear();
> +  mCurrentProfileFinished = false;

We don't need to reset |mCurrentProfileFinished| to false in destructor.

@@ +184,5 @@
> +CheckProfileStatusCallback::Notify(nsITimer* aTimer)
> +{
> +  MOZ_ASSERT(mController);
> +  // Continue on the next profile since we haven't got the callback after the
> +  // kTimerInterval timeout.

Please refine the above comment. There's no |kTimerInterval| anymore.

@@ +201,3 @@
>    NS_ENSURE_TRUE_VOID(mProfiles.Length() > 0);
>  
> +  mCurrentProfileFinished = false;

There's no need to reset the variable again since we've done it in the constructor. If you want to make sure its value, please use assertion.

@@ +280,5 @@
>  
>  void
>  BluetoothProfileController::OnDisconnect(const nsAString& aErrorStr)
>  {
>    MOZ_ASSERT(NS_IsMainThread());

Would you like to add an assertion for mTimer to keep consistency with Onconnect?

::: dom/bluetooth/BluetoothProfileController.h
@@ +52,5 @@
>  
>  // Pointing device: sub-field of minor device class (Bit 7)
>  #define IS_POINTING_DEVICE(cod)      ((GET_MINOR_DEVICE_CLASS(cod) & 0x20) >> 5)
>  
> +class BluetoothProfileController;

How about declaring class BluetoothProfileController before class CheckProfileStatusCallback? Then, we don't need this forward declaration which is a bit weird for me since we declare a class in its header file.

@@ +65,5 @@
> +  NS_DECL_NSITIMERCALLBACK
> +
> +  CheckProfileStatusCallback(BluetoothProfileController* aController)
> +    : mController(aController)
> +  {

It would be better to check the argument.
MOZ_ASSERT(!aController)

@@ +76,5 @@
> +
> +private:
> +  nsRefPtr<BluetoothProfileController> mController;
> +};
> +

If this class is only used for BluetoothProfileController, let's make it file-scoped. It's not necessary for profile managers which include BluetoothProfileController.h to know it, right?

@@ +123,5 @@
>     */
>    void OnDisconnect(const nsAString& aErrorStr);
>  
> +  /**
> +   * It is invoked after profile has reached timeout, reset profile manager

nit: "a" profile.

Btw, you might want to use profile array or the variable name |mProfiles| directly since "profile manager" is quite confusing.

@@ +149,5 @@
>  
>    bool mSuccess;
>    int8_t mProfilesIndex;
>    nsTArray<BluetoothProfileManagerBase*> mProfiles;
> +  bool mCurrentProfileFinished;

Suggest to put it with the other boolean variable (mSuccess).

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +1230,5 @@
>  BluetoothHfpManager::Connect(const nsAString& aDeviceAddress,
>                               BluetoothProfileController* aController)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aController);

Why should we make this change?

@@ +1259,5 @@
>    bt_bdaddr_t deviceBdAddress;
>    StringToBdAddressType(mDeviceAddress, &deviceBdAddress);
>    NS_ENSURE_TRUE_VOID(BT_STATUS_SUCCESS ==
>      sBluetoothHfpInterface->disconnect(&deviceBdAddress));
>  

Why should we make this change?

::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
@@ +392,5 @@
>    // Please see Bug 878728 for more information.
>    mBSIR = false;
>  
>    ResetCallArray();
> +

nit: empty line.
Attachment #8381289 - Flags: review?(gyeh)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #59)
> Comment on attachment 8381289 [details] [diff] [review]
> Bug 951634 - Patch 1: Add TimerCallback in Profile Controller
> 
> Review of attachment 8381289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
> @@ +1230,5 @@
> >  BluetoothHfpManager::Connect(const nsAString& aDeviceAddress,
> >                               BluetoothProfileController* aController)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aController);
> 
> Why should we make this change?
> 
> @@ +1259,5 @@
> >    bt_bdaddr_t deviceBdAddress;
> >    StringToBdAddressType(mDeviceAddress, &deviceBdAddress);
> >    NS_ENSURE_TRUE_VOID(BT_STATUS_SUCCESS ==
> >      sBluetoothHfpInterface->disconnect(&deviceBdAddress));
> >  
> 
> Why should we make this change?
See Comment 3, it's possible that mController has been assigned.
Is BlueDroid a device we support for 1.4?
No it's not a device that we support, bluedroid is bluetooth protocol stack. bluedroid is the Bluetooth back-end.
Comment on attachment 8382035 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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

dom/bluetooth/bluedroid/BluetoothHfpManager.cpp is no longer existed. Please rebase the patch.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +25,5 @@
>    } while(0)
>  
> +#define CONNECTION_TIMEOUT_MS 15000
> +
> +class mozilla::dom::bluetooth::CheckProfileStatusCallback : public nsITimerCallback

We don't have to specify a full namespace path since we've already declared the namespace before.

::: dom/bluetooth/BluetoothProfileController.h
@@ +52,5 @@
>  
>  // Pointing device: sub-field of minor device class (Bit 7)
>  #define IS_POINTING_DEVICE(cod)      ((GET_MINOR_DEVICE_CLASS(cod) & 0x20) >> 5)
>  
> +class BluetoothProfileController;

No need to do forward declaration here, right?

@@ +104,5 @@
>     */
>    void OnDisconnect(const nsAString& aErrorStr);
>  
> +  /**
> +   * It is invoked after a profile has reached timeout, reset mProfiles

nit: add period at the end.

::: dom/bluetooth/BluetoothProfileManagerBase.h
@@ +65,5 @@
>    virtual void OnConnect(const nsAString& aErrorStr) = 0;
>    virtual void OnDisconnect(const nsAString& aErrorStr) = 0;
>  
>    /**
> +   * Clean up profile resources and set BluetoothProfileController as null

nit: Set |mController| as null and add period at the end.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +497,5 @@
>  BluetoothA2dpManager::ResetA2dp()
>  {
>    mA2dpConnected = false;
>    mSinkState = SinkState::SINK_DISCONNECTED;
> +  NS_ENSURE_TRUE_VOID(mController);

Why should we add this macro for checking mController before nulling it out?

@@ +599,5 @@
>  void
>  BluetoothA2dpManager::Disconnect(BluetoothProfileController* aController)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aController && !mController);

The assertion should change a little bit.

When turning off Bluetooth, we disconnect all profiles before cutting off the power. In this case, no DOMRequest should be replied and we set aController to nullptr. Please check BluetoothService.cpp for further details.

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +1252,5 @@
>  void
>  BluetoothHfpManager::Disconnect(BluetoothProfileController* aController)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aController && !mController);

ditto.
Comment on attachment 8377390 [details] [diff] [review]
Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

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

Suggest to check the connection state in function ProcessConnectionState(). It's easier to perceive the outgoing connection failure (from BTHF_CONNECTION_STATE_CONNECTING to BTHF_CONNECTION_STATE_DISCONNECTED).

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +755,5 @@
>        OnConnect(EmptyString());
>      } else if (mConnectionState == BTHF_CONNECTION_STATE_DISCONNECTED) {
>        mDeviceAddress.AssignLiteral(BLUETOOTH_ADDRESS_NONE);
> +      if (mPrevConnectionState == BTHF_CONNECTION_STATE_DISCONNECTED) {
> +        // This implies outgoing connection failure

Implication of the state change from DISCONNECTED to DISCONNECTED is not easy to get.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #65)
> Comment on attachment 8377390 [details] [diff] [review]
> Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect
> instead of OnDisconnect
> 
> Review of attachment 8377390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Suggest to check the connection state in function ProcessConnectionState().
> It's easier to perceive the outgoing connection failure (from
> BTHF_CONNECTION_STATE_CONNECTING to BTHF_CONNECTION_STATE_DISCONNECTED).
> 
Actually in this case, bluedroid won't have BTHF_CONNECTION_STATE_CONNECTING state (callback won't give this state, only returns connected or disonnected, these two states), my original patch is set current state when doing |connect()|. Since Ben disagrees adding state that does not report by bluetooth stack.
> ::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
> @@ +755,5 @@
> >        OnConnect(EmptyString());
> >      } else if (mConnectionState == BTHF_CONNECTION_STATE_DISCONNECTED) {
> >        mDeviceAddress.AssignLiteral(BLUETOOTH_ADDRESS_NONE);
> > +      if (mPrevConnectionState == BTHF_CONNECTION_STATE_DISCONNECTED) {
> > +        // This implies outgoing connection failure
> 
> Implication of the state change from DISCONNECTED to DISCONNECTED is not
> easy to get.

I think so.
Attachment #8382035 - Attachment is obsolete: true
Attachment #8382035 - Flags: review?(gyeh)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #64)
> Comment on attachment 8382035 [details] [diff] [review]
> Bug 951634 - Patch 1: Add TimerCallback in Profile Controller
> 
> Review of attachment 8382035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> dom/bluetooth/bluedroid/BluetoothHfpManager.cpp is no longer existed. Please
> rebase the patch.
> 
> ::: dom/bluetooth/BluetoothProfileController.cpp
> @@ +25,5 @@
> >    } while(0)
> >  
> > +#define CONNECTION_TIMEOUT_MS 15000
> > +
> > +class mozilla::dom::bluetooth::CheckProfileStatusCallback : public nsITimerCallback
> 
> We don't have to specify a full namespace path since we've already declared
> the namespace before.
> 
I can't remove namespace because the header also use it. Remove namespace here can cause build break.
Attachment #8385186 - Flags: review?(echou) → review?(gyeh)
Open an another bug for Patch 2, see bug 979160
Comment on attachment 8385186 [details] [diff] [review]
Bug 951634: Add TimerCallback in Profile Controller

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

It's almost ready. :)

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +324,5 @@
> +{
> +  MOZ_ASSERT(!mCurrentProfileFinished);
> +  MOZ_ASSERT(mProfilesIndex < (int)mProfiles.Length());
> +
> +  BT_LOGR_PROFILE(mProfiles[mProfilesIndex], ERR_CONNECTION_TIMEOUT);

Suggest to use a more general term.
Attachment #8385186 - Flags: review?(gyeh) → review+
https://hg.mozilla.org/mozilla-central/rev/7ea33602118d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → ---
You need to log in before you can comment on or make changes to this bug.