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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

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)

Comment 1

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

found in full run of buri v1.3
added burirun1.3-1

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

> 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.

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

> 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.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Part 1: [bluedroid] Reset mController array after bluetooth is disabled

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: a2dp_stuck_logcat.txt based on Comment 5

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: btsnoop_hci(1).cfa

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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'

Comment 13

[Jean Gong :jgong]

PM triaged this bug and it be a 1.3 blocker.

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: part2.patch (WIP)

I'm still working on this patch. Even though I already replied error, the problem still not been resolved.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

It looks like I need to inherit CancelableTask, whever get connected.

Comment 17

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

(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.

Comment 18

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

Renominate as non-blocking 1.3 according to comment 17. No devices which takes 1.3 will use Bluedroid as its Bluetooth backend.

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug951634-wip.patch

Comment 20

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 21

[Bruce Huang [:bhuang] (inactive)]

(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 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug951634-wip.patch 2nd version (CanableTask version)

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 24

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

Attachment: [WIP] Add TimerCallback in Profile Controller

Comment 25

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: [WIP]Bug951634 3rd version

Comment 26

[Preeti Raghunath(:Preeti)]

moving to 1.4 per comment 21.

Comment 27

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 28

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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

Comment 29

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

Comment 30

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 31

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Add TimerCallback in Profile Controller

Fix build problem on nexus 4 bluedroid.

Comment 32

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 33

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 34

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

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 35

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add delay reply in Profile Controller (CancelableTask)

Comment 36

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 37

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 38

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 39

[Ben Tian (inactive)]

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 40

[Ben Tian (inactive)]

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.

Comment 41

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 42

[Ben Tian (inactive)]

(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?

Comment 43

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

Comment 44

[Ben Tian (inactive)]

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'.

Comment 45

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 2: Outgoing connection callback shall trigger OnConnect instead of OnDisconnect

Comment 46

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: 0001-Bug-951634-Add-TimerCallback-in-Profile-Controller.patch

Comment 47

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

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?

Comment 48

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 49

[Ben Tian (inactive)]

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.

Comment 50

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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 51

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 52

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

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.

Comment 53

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8379645 [details] [diff] [review]
Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Sorry, I uploaded wrong version patch.

Comment 54

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 55

[Ben Tian (inactive)]

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();
}

Comment 56

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

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.

Comment 57

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

Please ignore comment 56. :)

Comment 58

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 59

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

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.

Comment 60

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 61

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 62

[Preeti Raghunath(:Preeti)]

Is BlueDroid a device we support for 1.4?

Comment 63

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

No it's not a device that we support, bluedroid is bluetooth protocol stack. bluedroid is the Bluetooth back-end.

Comment 64

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

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 65

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

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.

Comment 66

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 67

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634 - Patch 1: Add TimerCallback in Profile Controller

Comment 68

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 69

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634: Add TimerCallback in Profile Controller

Comment 70

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Open an another bug for Patch 2, see bug 979160

Comment 71

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

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.

Comment 72

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 73

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 951634: Add TimerCallback in Profile Controller, r=gyeh

Change error message to 'ERR_OPERATION_TIMEOUT'

Comment 74

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

https://hg.mozilla.org/integration/b2g-inbound/rev/7ea33602118d

Comment 75

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ea33602118d