Closed Bug 984284 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: xinhe.yan, Assigned: ben.tian)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached image 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
Attached file main log from tarako
Gecko did not send sink_disconnect to bluez

Please focus log after 03-17 16:26:00.
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
disable bt when you see "connected to phone audio". At this time, a2dp headset is not appear on notification bar.
blocking-b2g: --- → 1.3T?
Flags: needinfo?(styang)
Hi Shawn, would you mind taking a look on this issue?
Flags: needinfo?(styang) → needinfo?(shuang)
Ben will take this bug. Thanks.
Flags: needinfo?(shuang)
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.
Assignee: nobody → btian
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
is this tarako specific? or 1.3 bug in general? thanks
Flags: needinfo?(btian)
hamachi gecko 1.3 branch have this bug too.
(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.
Flags: needinfo?(btian)
Attachment #8392783 - Flags: review?(echou)
Attachment #8392783 - Flags: feedback?(shuang)
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?
Attachment #8392783 - Flags: feedback?(shuang)
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
(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.
take this for 1.3T
Fabrice, can you do the approval for 1.3? thanks
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(fabrice)
Sure, I'll uplift once the patch is r+
Flags: needinfo?(fabrice)
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.
Revise based on reviewer's comment.
Attachment #8392783 - Attachment is obsolete: true
Attachment #8392783 - Flags: review?(echou)
Attachment #8393305 - Flags: review?(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. :)
Attachment #8393305 - Flags: review?(echou) → review+
Attachment #8393305 - Attachment description: [b2g28-only] Patch 1 (v2): Reset A2dp state when BT is disabled → [b2g28-only] Patch 1 (final): Reset A2dp state when BT is disabled, f=shuang, r=echou, a=1.3T+
> 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?
Flags: needinfo?(jcheng)
since Fabrice approves it for 1.3+, 1.3+ goes to both 1.3 and 1.3T
thanks
blocking-b2g: 1.3T+ → 1.3+
Flags: needinfo?(jcheng)
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
Attachment #8393305 - Flags: approval-mozilla-b2g28?
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.
blocking-b2g: 1.3+ → 1.3?
Keywords: qawanted
(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.
Backlog for now.
blocking-b2g: 1.3? → backlog
Keywords: qawanted
(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.
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.
Flags: needinfo?(praghunath)
(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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(praghunath)
Resolution: --- → FIXED
oops, sorry for wrong bug status. Change it back to open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(jcheng)
Flags: needinfo?(jcheng)
Flags: needinfo?(styang)
It's a blocker for Tarako. nom 1.3T+
blocking-b2g: backlog → 1.3T+
Flags: needinfo?(styang)
Please help to prepare the patch for 1.3T. Thanks.
Flags: needinfo?(echou)
Flags: needinfo?(btian)
(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.
Flags: needinfo?(echou)
Flags: needinfo?(btian)
(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.
Keywords: checkin-needed
Hi Ying, please uplift the patch. thanks!
Flags: needinfo?(ying.xu)
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.
Attachment #8396983 - Flags: review?(echou)
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).
Flags: needinfo?(btian)
Keywords: checkin-needed
(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.
Flags: needinfo?(btian)
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
Attachment #8393305 - Flags: approval-mozilla-b2g28?
Since 1.3T now is level 2 access, partners no longer have access on it. Hi Fabrice, could you help this uplift? thanks!
Flags: needinfo?(ying.xu) → needinfo?(fabrice)
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
Attachment #8396983 - Flags: review?(echou) → review-
(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 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+.
Attachment #8396983 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/713e4011cd84
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
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
Duplicate of this bug: 993922
You need to log in before you can comment on or make changes to this bug.