984284 - [Tarako][Hamachi]a2dp headset icon show on the notification bar

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Xinhe Yan]

Attachment: screenshot of hamachi

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.19 (KHTML, like Gecko) Ubuntu/11.04 Chromium/18.0.1025.151 Chrome/18.0.1025.151 Safari/535.19

Steps to reproduce:

1 connect a2dp headset
2 close bt in setting
3 open bt
4 close bt just after connect a2dp


Actual results:

a2dp headset icon show on the notification bar


Expected results:

a2dp headset icon should remove after turn off bt

Comment 1

[Xinhe Yan]

Attachment: main log from tarako

Gecko did not send sink_disconnect to bluez

Please focus log after 03-17 16:26:00.

Comment 2

[Xinhe Yan]

disable bt when you see "connected to phone audio". At this time, a2dp headset is not appear on notification bar.

Comment 3

[Steven Yang [:styang]]

Hi Shawn, would you mind taking a look on this issue?

Comment 4

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

Ben will take this bug. Thanks.

Comment 5

[Ben Tian (inactive)]

The cause is that A2DP manager doesn't reject the "connected" state change when BT is turned off during connecting, so A2DP manager still notifies settings app of "A2DP connected".

I'll come up a fix for this bug.

Comment 6

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

is this tarako specific? or 1.3 bug in general? thanks

Comment 7

[Xinhe Yan]

hamachi gecko 1.3 branch have this bug too.

Comment 8

[Ben Tian (inactive)]

(In reply to Joe Cheng [:jcheng] from comment #6)
> is this tarako specific? or 1.3 bug in general? thanks

Joe: this is 1.3 bug in general.

Comment 9

[Ben Tian (inactive)]

Attachment: [b2g28-only] Patch 1 (v1): Reset A2dp state when BT is disabled

Comment 10

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

Comment on attachment 8392783 [details] [diff] [review]
[b2g28-only] Patch 1 (v1): Reset A2dp state when BT is disabled

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

::: dom/bluetooth/bluez/BluetoothA2dpManager.cpp
@@ +299,5 @@
>          break;
>        }
> +
> +      // case 7: Successfully connected but bluetooth is already disabled
> +      if (prevState != SinkState::SINK_CONNECTING) {

Will this valid for an incoming connection.
For an incoming connection case, preState == DISCONNECTED, mSinkState == CONNECTED, can you confirm this part?

Comment 11

[Ben Tian (inactive)]

Revise STR:

> Steps to reproduce:
> 
> 1 connect a2dp headset
> 2 close bt in setting
> 3 open bt
> 4 close bt just after connect a2dp
4. close BT just after HFP is connected ("Connected to phone audio" shows).

Note "Connected to media/phone audio" would show if A2DP is also connected.
> 
> Actual results:
> 
> a2dp headset icon show on the notification bar
> 
> 
> Expected results:
> 
> a2dp headset icon should remove after turn off bt

Comment 12

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #10)
> Comment on attachment 8392783 [details] [diff] [review]
> [b2g28-only] Patch 1 (v1): Reset A2dp state when BT is disabled
> 
> Review of attachment 8392783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluez/BluetoothA2dpManager.cpp
> @@ +299,5 @@
> >          break;
> >        }
> > +
> > +      // case 7: Successfully connected but bluetooth is already disabled
> > +      if (prevState != SinkState::SINK_CONNECTING) {
> 
> Will this valid for an incoming connection.
> For an incoming connection case, preState == DISCONNECTED, mSinkState ==
> CONNECTED, can you confirm this part?

The fix is valid for incoming connections since the sink state also transit from DISCONNECTED -> CONNECTING -> CONNECTED for incoming connections.

Comment 13

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

take this for 1.3T
Fabrice, can you do the approval for 1.3? thanks

Comment 14

[[:fabrice] Fabrice Desré]

Sure, I'll uplift once the patch is r+

Comment 15

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

Comment on attachment 8392783 [details] [diff] [review]
[b2g28-only] Patch 1 (v1): Reset A2dp state when BT is disabled

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

Hi Ben, please see my comments below.

::: dom/bluetooth/BluetoothService.cpp
@@ +495,5 @@
> +    NS_ENSURE_TRUE(a2dp, NS_ERROR_FAILURE);
> +    if (a2dp->IsConnected()) {
> +      a2dp->Disconnect(nullptr);
> +    } else {
> +      a2dp->ResetA2dp();

Since ResetA2dp() and ResetAvrcp() are always appear simultaneously, please make Reset() public and make ResetA2dp()/ResetAvrcp() private.

::: dom/bluetooth/bluez/BluetoothA2dpManager.cpp
@@ +249,5 @@
>   * 6. "connected" -> "disconnected"
>   *    "playing" -> "disconnected"
>   *    Disconnected from local or the remote device
> + * 7. "disconnected" -> "connected"
> + *    Successfully connected but bluetooth is already disabled

Actually this state change is totally an edge case to me. Please at least mention the bug number here to make this more clear.

Comment 16

[Ben Tian (inactive)]

Attachment: [b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled, f=shuang, r=echou, a=1.3T+

Revise based on reviewer's comment.

Comment 17

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

Comment on attachment 8393305 [details] [diff] [review]
[b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled, f=shuang, r=echou, a=1.3T+

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

LGTM. Thanks. :)

Comment 18

[Ben Tian (inactive)]

> Comment on attachment 8393305 [details] [diff] [review]
> [b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled,
> f=shuang, r=echou, a=1.3T+
Joe, should I request b2g28 approval or just set "checkin-needed" for this b2g28-only 1.3T+ fix?

Comment 19

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

since Fabrice approves it for 1.3+, 1.3+ goes to both 1.3 and 1.3T
thanks

Comment 20

[Ben Tian (inactive)]

Comment on attachment 8393305 [details] [diff] [review]
[b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled, f=shuang, r=echou, a=1.3T+

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  None
User impact if declined:
  The A2DP headset icon shows if user turns off BT when A2DP (media audio) is reconnecting.
Testing completed: 
  Verified on Hamachi 1.3 build.
Risk to taking this patch (and alternatives if risky): 
  None
String or UUID changes made by this patch:
  None

Comment 21

[Jason Smith [:jsmith]]

This is missing a key piece of information to justify the blocking decision - whether this is a regression or not. Renoming to find this out.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #21)
> This is missing a key piece of information to justify the blocking decision
> - whether this is a regression or not. Renoming to find this out.

Oh and - QA Wanted to find out if this reproduces on 1.1.

Comment 23

[Preeti Raghunath(:Preeti)]

Backlog for now.

Comment 24

[Ben Tian (inactive)]

(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Jason Smith [:jsmith] from comment #21)
> > This is missing a key piece of information to justify the blocking decision
> > - whether this is a regression or not. Renoming to find this out.
> 
> Oh and - QA Wanted to find out if this reproduces on 1.1.

Jason: 1.1 doesn't have the bug because A2DP (media audio) is supported since 1.2.

Comment 25

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

Hi Preeti,

Does 'backlog' mean we won't fix this for 1.3T? If it does, then we should just close this as 'won't fix' since this is a 1.3/1.3T specific issue. (Actually this can happen easily and we're confident with our solution, please reconsider to make it land on 1.3/1.3T.)

Thank you.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #25)
> Hi Preeti,
> 
> Does 'backlog' mean we won't fix this for 1.3T? If it does, then we should
> just close this as 'won't fix' since this is a 1.3/1.3T specific issue.
> (Actually this can happen easily and we're confident with our solution,
> please reconsider to make it land on 1.3/1.3T.)
> 
> Thank you.

Yes - only blockers can land for 1.3 & 1.3T.

Comment 27

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

(In reply to Jason Smith [:jsmith] from comment #26)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #25)
> > Hi Preeti,
> > 
> > Does 'backlog' mean we won't fix this for 1.3T? If it does, then we should
> > just close this as 'won't fix' since this is a 1.3/1.3T specific issue.
> > (Actually this can happen easily and we're confident with our solution,
> > please reconsider to make it land on 1.3/1.3T.)
> > 
> > Thank you.
> 
> Yes - only blockers can land for 1.3 & 1.3T.

ok, thanks.

Just talked to Ben and we decided to leave this open because we need a patch for m-c as well.

Comment 28

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

oops, sorry for wrong bug status. Change it back to open.

Comment 29

[Steven Yang [:styang]]

It's a blocker for Tarako. nom 1.3T+

Comment 30

[Steven Yang [:styang]]

Please help to prepare the patch for 1.3T. Thanks.

Comment 31

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #20)
> Comment on attachment 8393305 [details] [diff] [review]
> [b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled,
> f=shuang, r=echou, a=1.3T+
> 
> NOTE: This flag is now for security issues only. Please see
> https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand
> the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
>   None
> User impact if declined:
>   The A2DP headset icon shows if user turns off BT when A2DP (media audio)
> is reconnecting.
> Testing completed: 
>   Verified on Hamachi 1.3 build.
> Risk to taking this patch (and alternatives if risky): 
>   None
> String or UUID changes made by this patch:
>   None

Steven, the 1.3T patch is already here.

Comment 32

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #20)
> Comment on attachment 8393305 [details] [diff] [review]
> [b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled,
> f=shuang, r=echou, a=1.3T+
> 
> NOTE: This flag is now for security issues only. Please see
> https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand
> the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
>   None
> User impact if declined:
>   The A2DP headset icon shows if user turns off BT when A2DP (media audio)
> is reconnecting.
> Testing completed: 
>   Verified on Hamachi 1.3 build.
> Risk to taking this patch (and alternatives if risky): 
>   None
> String or UUID changes made by this patch:
>   None

Please help land this 1.3T specific patch.

Comment 33

[Steven Yang [:styang]]

Hi Ying, please uplift the patch. thanks!

Comment 34

[Ben Tian (inactive)]

Attachment: [m-c] Patch 2 (v1): Reject 'connected' sink state change when BT is already disabled

The patch is for blueZ only. Bluedroid doesn't have this bug but sometimes cannot recover connection after re-enable BT. Will open another bug to track.

Changes:
- [blueZ] Reject A2DP 'connected' sink state when BT is already disabled.
- Integrate functions |ResetA2dp| and |ResetAvrcp| into one function |Reset|.
- Use pre-defined error message in A2DP and HFP managers.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Don't we usually try to land the patch on trunk before uplifting to branches? Also, why does the 1.3t patch have a b2g28 approval request on it? That's for landing on v1.3 (not v1.3t).

Comment 36

[Ben Tian (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> Don't we usually try to land the patch on trunk before uplifting to
> branches?
1.3/1.3T codebase requires different fix from m-c. To have a 1.3 fix with minimal stability impact first, I customized this b2g28-only patch. The m-c patch is ongoing as in comment 34. Please keep this bug open until I fix in m-c.

> Also, why does the 1.3t patch have a b2g28 approval request on it?
> That's for landing on v1.3 (not v1.3t).
This bug was 1.3+ so I requested approval in comment 20. Later it became backlog and now is 1.3T+. I just reply based on the latest patch.

Comment 37

[bhavana bajaj [:bajaj]]

Comment on attachment 8393305 [details] [diff] [review]
[b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled, f=shuang, r=echou, a=1.3T+

Clearing the b2g28 approval request here as this in not a 1.3+ bug and will not be needed on that branch.

To resolve this on tarako, it will be cherry picked by partner, lets keep the bug open to address the issue on central

Comment 38

[Steven Yang [:styang]]

Since 1.3T now is level 2 access, partners no longer have access on it. Hi Fabrice, could you help this uplift? thanks!

Comment 39

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/49fc2445ad0e

Comment 40

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

Comment on attachment 8396983 [details] [diff] [review]
[m-c] Patch 2 (v1): Reject 'connected' sink state change when BT is already disabled

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

Hi Ben,

Overall looks good. r- mainly because I couldn't find Reset() defined in BluetoothA2dpManager.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +598,5 @@
>      }
>  
>      BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();
>      NS_ENSURE_TRUE(a2dp, NS_ERROR_FAILURE);
> +    a2dp->Reset();

This seems not work because I can't find Reset() defined in BluetoothA2dpManager.h

Comment 41

[Ben Tian (inactive)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #40)
> Comment on attachment 8396983 [details] [diff] [review]
> [m-c] Patch 2 (v1): Reject 'connected' sink state change when BT is already
> disabled
> 
> Review of attachment 8396983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ben,
> 
> Overall looks good. r- mainly because I couldn't find Reset() defined in
> BluetoothA2dpManager.
> 
> ::: dom/bluetooth/bluez/BluetoothDBusService.cpp
> @@ +598,5 @@
> >      }
> >  
> >      BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();
> >      NS_ENSURE_TRUE(a2dp, NS_ERROR_FAILURE);
> > +    a2dp->Reset();
> 
> This seems not work because I can't find Reset() defined in
> BluetoothA2dpManager.h

Eric, the Reset() function is defined in BluetoothProfileManagerBase [1], which is inherited by BluetoothA2dpManager [2].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothProfileManagerBase.h#71
[2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/BluetoothA2dpManager.h#16

Comment 42

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

Comment on attachment 8396983 [details] [diff] [review]
[m-c] Patch 2 (v1): Reject 'connected' sink state change when BT is already disabled

Ah, ok then. Sorry for my mistake. r+.

Comment 43

[Ben Tian (inactive)]

try server: https://tbpl.mozilla.org/?tree=Try&rev=efb3fe5c23bb

Comment 44

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/713e4011cd84

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/713e4011cd84

Comment 46

[Firefox Commit Bot]

The following changeset is now in Firefox Nightly:

> 713e4011cd84 Bug 984284 - [m-c] a2dp headset icon show on the notification bar, r=echou

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central