Closed Bug 830213 Opened 7 years ago Closed 6 years ago

[Bluetooth][Certification]HFP PTS TC_AG_ATH_BV_04_I failed due to there is no API for disconnect SCO connection

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 IOT1 (10may)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: shawnjohnjr, Assigned: gyeh)

References

Details

(Whiteboard: [fixed-in-birch] QARegressExclude)

Attachments

(11 files, 8 obsolete files)

12.03 KB, patch
Details | Diff | Splinter Review
15.58 KB, patch
Details | Diff | Splinter Review
12.31 KB, patch
Details | Diff | Splinter Review
11.98 KB, patch
Details | Diff | Splinter Review
13.21 KB, patch
Details | Diff | Splinter Review
15.65 KB, patch
Details | Diff | Splinter Review
13.29 KB, patch
Details | Diff | Splinter Review
12.39 KB, patch
Details | Diff | Splinter Review
16.02 KB, patch
Details | Diff | Splinter Review
757 bytes, patch
Details | Diff | Splinter Review
3.07 KB, patch
Details | Diff | Splinter Review
Summary: [] → [Bluetooth]HFP PTS TC_AG_ATH_BV_04_I failed due to there is no API for disconnect SCO connection
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee: nobody → gyeh
This also blocked "TC_AG_TCA" series test cases.
Summary: [Bluetooth]HFP PTS TC_AG_ATH_BV_04_I failed due to there is no API for disconnect SCO connection → [Bluetooth][Certification]HFP PTS TC_AG_ATH_BV_04_I failed due to there is no API for disconnect SCO connection
Sorry, I don't really understand this title. Does it mean that this is a must have item for the Bluetooth certification that we're having right now? Thanks.
Yes, Kevin. This is mandatory for the Bluetooth Hfp certification.

I'm working on the implementation for platform side, and I will file another one for Dialer App.

When we switch audio stream from Bluetooth headset to speaker, Dialer app can close Bluetooth Sco socket by new API. When switching back to Bluetooth headset, it can also create Bluetooth Sco socket by new API.
Thank you, Gina. 

In this case, I think it looks like a tef+ case.
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+
Target Milestone: --- → 1.0.1 IOT1 (10may)
Whiteboard: [status: needs patch]
Comment on attachment 747266 [details] [diff] [review]
Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

Three interfaces are added in this patch.
Attachment #747266 - Flags: superreview?(mrbkap)
Attachment #747266 - Flags: review?(echou)
Comment on attachment 747262 [details] [diff] [review]
Patch 2(v1): Implementation in BluetoothHfpManager

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

The general idea for this patch is let BluetoothHfpManager owns a SCO socket, and move functions from BluetoothScoManager to BluetoothHfpManager.
Remove class BluetoothScoManager
Attachment #747263 - Attachment is obsolete: true
Attachment #747270 - Flags: review?(echou)
Attachment #747262 - Flags: superreview?(mrbkap)
Attachment #747262 - Flags: review?(echou)
Comment on attachment 747266 [details] [diff] [review]
Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +150,5 @@
> +
> +  void
> +  ReleaseMembers()
> +  {
> +    BluetoothReplyRunnable::ReleaseMembers();

Since we don't do anything else than BluetoothReplyRunnable::ReleaseMembers(), no need to implement this function.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2846,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  NS_ENSURE_TRUE_VOID(hfp);
> +  if(!hfp->ConnectSco(aRunnable)) {

nit: need space after 'if'
Attachment #747266 - Flags: review?(echou) → review+
Comment on attachment 747262 [details] [diff] [review]
Patch 2(v1): Implementation in BluetoothHfpManager

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

Hi Gina, please have me review again once revision is done. Thanks!

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +519,5 @@
> +    do_GetService("@mozilla.org/observer-service;1");
> +  NS_ENSURE_TRUE_VOID(obs);
> +
> +  const PRUnichar* addr =
> +    mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading();

There are two problems:

1. Once mDevicePath is set in OnConnectSuccess(), it seems not to be cleaned at any other places, like OnDisconnect().

The original implementation seems to work well. How about making addr as an argument of this function?

2. If mDevicePath.BeginReading() returns nullptr when it is empty, then we don't need the IsEmpty() check.

@@ +523,5 @@
> +    mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading();
> +
> +  if (NS_FAILED(obs->NotifyObservers(nullptr,
> +          BLUETOOTH_SCO_STATUS_CHANGED,
> +          addr))) {

nit: weird indent

@@ +1489,5 @@
> +BluetoothHfpManager::OnScoConnectSuccess()
> +{
> +  // For active connection request, we need to reply the DOMRequest
> +  if (mScoRunnable) {
> +    DispatchBluetoothReply(mRunnable,

This should be mScoRunnable, right?

@@ +1491,5 @@
> +  // For active connection request, we need to reply the DOMRequest
> +  if (mScoRunnable) {
> +    DispatchBluetoothReply(mRunnable,
> +                           BluetoothValue(true), NS_LITERAL_STRING(""));
> +    mScoRunnable.forget();

I think you should do

  mScoRunnable = nullptr;

or mScoRunnable may leak.

@@ +1506,5 @@
> +  if (mScoRunnable) {
> +    NS_NAMED_LITERAL_STRING(replyError, "Failed to create SCO socket!");
> +    DispatchBluetoothReply(mScoRunnable, BluetoothValue(), replyError);
> +
> +    mScoRunnable.forget();

I think you should do

  mScoRunnable = nullptr;

or mScoRunnable may leak.

@@ +1510,5 @@
> +    mScoRunnable.forget();
> +  }
> +
> +  mScoSocket->Disconnect();
> +  mScoSocketStatus = mScoSocket->GetConnectionStatus();

Since it's "ConnectError", it means that SCO has not been established. Maybe we don't need to call Disconnect() and update mScoSocketStatus. ListenSco() would be just fine.

@@ +1551,5 @@
> +    return false;
> +  }
> +
> +  if (!mSocket) {
> +    NS_WARNING("BluetoothHfpManager is not connected");

If you want to check whether there is a 'connected' SLC or not, using IsConnected() would be more accurate than using !mSocket

@@ +1565,5 @@
> +  }
> +
> +  mScoSocket->Disconnect();
> +
> +  mRunnable = aRunnable;

mRunnable or mScoRunnable?

@@ +1578,5 @@
> +bool
> +BluetoothHfpManager::DisconnectSco()
> +{
> +  if (!mSocket) {
> +    NS_WARNING("BluetoothHfpManager is not connected");

If you want to check whether there is a 'connected' SLC or not, using IsConnected() would be more accurate than using !mSocket

@@ +1592,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (gInShutdown) {
> +    MOZ_ASSERT(false, "ListenSco called while in shutdown!");

Why we use MOZ_ASSERT here and NS_WARNING at other places? :|

@@ +1599,5 @@
> +
> +  if (mScoSocket->GetConnectionStatus() ==
> +      SocketConnectionStatus::SOCKET_LISTENING) {
> +    NS_WARNING("SCO socket has been already listening");
> +    return true;

It's not the same strategy as HFP. In BluetoothHfpManager::Listen(), once we found that there is a connected HFP connection, false is returned. We should make these two profiles act in the same way.
Attachment #747262 - Flags: review?(echou) → review-
Update patch as comments from echou.
Attachment #747262 - Attachment is obsolete: true
Attachment #747262 - Flags: superreview?(mrbkap)
Attachment #747377 - Flags: superreview?(mrbkap)
Attachment #747377 - Flags: review?(echou)
Comment on attachment 747262 [details] [diff] [review]
Patch 2(v1): Implementation in BluetoothHfpManager

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +519,5 @@
> +    do_GetService("@mozilla.org/observer-service;1");
> +  NS_ENSURE_TRUE_VOID(obs);
> +
> +  const PRUnichar* addr =
> +    mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading();

Fixed.

@@ +523,5 @@
> +    mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading();
> +
> +  if (NS_FAILED(obs->NotifyObservers(nullptr,
> +          BLUETOOTH_SCO_STATUS_CHANGED,
> +          addr))) {

Fixed.

@@ +1489,5 @@
> +BluetoothHfpManager::OnScoConnectSuccess()
> +{
> +  // For active connection request, we need to reply the DOMRequest
> +  if (mScoRunnable) {
> +    DispatchBluetoothReply(mRunnable,

Fixed.

@@ +1491,5 @@
> +  // For active connection request, we need to reply the DOMRequest
> +  if (mScoRunnable) {
> +    DispatchBluetoothReply(mRunnable,
> +                           BluetoothValue(true), NS_LITERAL_STRING(""));
> +    mScoRunnable.forget();

This should be fine, right?

@@ +1506,5 @@
> +  if (mScoRunnable) {
> +    NS_NAMED_LITERAL_STRING(replyError, "Failed to create SCO socket!");
> +    DispatchBluetoothReply(mScoRunnable, BluetoothValue(), replyError);
> +
> +    mScoRunnable.forget();

This should be fine, right?

@@ +1510,5 @@
> +    mScoRunnable.forget();
> +  }
> +
> +  mScoSocket->Disconnect();
> +  mScoSocketStatus = mScoSocket->GetConnectionStatus();

Fixed.

@@ +1551,5 @@
> +    return false;
> +  }
> +
> +  if (!mSocket) {
> +    NS_WARNING("BluetoothHfpManager is not connected");

Fixed.

@@ +1565,5 @@
> +  }
> +
> +  mScoSocket->Disconnect();
> +
> +  mRunnable = aRunnable;

Fixed.

@@ +1578,5 @@
> +bool
> +BluetoothHfpManager::DisconnectSco()
> +{
> +  if (!mSocket) {
> +    NS_WARNING("BluetoothHfpManager is not connected");

Fixed.

@@ +1592,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (gInShutdown) {
> +    MOZ_ASSERT(false, "ListenSco called while in shutdown!");

I also modified this part in Listen().

@@ +1599,5 @@
> +
> +  if (mScoSocket->GetConnectionStatus() ==
> +      SocketConnectionStatus::SOCKET_LISTENING) {
> +    NS_WARNING("SCO socket has been already listening");
> +    return true;

Fixed.
Comment on attachment 747377 [details] [diff] [review]
Patch 2(v2): Implementation in BluetoothHfpManager

Note that some logic is changed in OnDisconnect().
Comment on attachment 747266 [details] [diff] [review]
Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2867,5 @@
> +    return;
> +  }
> +
> +  NS_NAMED_LITERAL_STRING(replyError,
> +    "SCO socket doesn't exist or HFP is not  connected");

Super-nit: there's an extra space between "not" and "connected".

@@ +2879,5 @@
> +
> +  BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  NS_ENSURE_TRUE_VOID(hfp);
> +  DispatchBluetoothReply(aRunnable,
> +                         hfp->IsScoConnected(), NS_LITERAL_STRING(""));

Use EmptyString() instead of NS_LITERAL_STRING("") here.
Attachment #747266 - Flags: superreview?(mrbkap) → superreview+
Comment on attachment 747377 [details] [diff] [review]
Patch 2(v2): Implementation in BluetoothHfpManager

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1490,5 @@
> +  // For active connection request, we need to reply the DOMRequest
> +  if (mScoRunnable) {
> +    DispatchBluetoothReply(mScoRunnable,
> +                           BluetoothValue(true), NS_LITERAL_STRING(""));
> +    mScoRunnable.forget();

This should be: mScoRunnable = nullptr; otherwise it'll leak.

@@ +1505,5 @@
> +  if (mScoRunnable) {
> +    NS_NAMED_LITERAL_STRING(replyError, "Failed to create SCO socket!");
> +    DispatchBluetoothReply(mScoRunnable, BluetoothValue(), replyError);
> +
> +    mScoRunnable.forget();

This also leaks.

@@ +1516,5 @@
> +BluetoothHfpManager::OnScoDisconnect()
> +{
> +  if (mScoSocketStatus == SocketConnectionStatus::SOCKET_CONNECTED) {
> +    ListenSco();
> +    NotifyAudioManager(NS_LITERAL_STRING(""));

All of the places where you use NS_LITERAL_STRING("") should be changed to use EmptyString().

@@ +1568,5 @@
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, false);
> +  nsresult rv = bs->GetScoSocket(mDeviceAddress, true, false, mScoSocket);
> +
> +  return NS_FAILED(rv) ? false : true;

Please change this to: return NS_SUCCEEDED(rv).
Attachment #747377 - Flags: superreview?(mrbkap) → superreview+
Whiteboard: [status: needs patch] → [status: needs review]
Just a suggestion, Gaia dialer also needs to change in order to let user have Bluetooth switch button for audio path. It is required to have an "icon" added into Dailer callscreen. In order to speed up progress, I suggest Gaia side can go in parallel.
After using |mRunnable = nullptr;| rather than |mRunnable.forget();|, I got a crash. 

Here's the backtrace:

mozilla::dom::bluetooth::PBluetoothRequestParent::Write (this=0x5a5a5a5a, __v=0x5a5a5a5a, __msg=0x480deb00, __nullable=false)
    at /objdir-gecko/unagi/central_debug/ipc/ipdl/PBluetoothRequestParent.cpp:635
635	        id = (__v)->mId;
(gdb) bt
#0  mozilla::dom::bluetooth::PBluetoothRequestParent::Write (this=0x5a5a5a5a, __v=0x5a5a5a5a, __msg=0x480deb00, __nullable=false)
    at /objdir-gecko/unagi/central_debug/ipc/ipdl/PBluetoothRequestParent.cpp:635
#1  0x41a0afb2 in mozilla::dom::bluetooth::PBluetoothRequestParent::Send__delete__ (actor=0x5a5a5a5a, response=...)
    at /objdir-gecko/unagi/central_debug/ipc/ipdl/PBluetoothRequestParent.cpp:78
#2  0x415e754e in mozilla::dom::bluetooth::BluetoothRequestParent::ReplyRunnable::Run (this=0x45326dc0)
    at /mozilla-central/dom/bluetooth/ipc/BluetoothParent.cpp:59
#3  0x41d3bb1a in nsThread::ProcessNextEvent (this=0x40404320, mayWait=<value optimized out>, result=0xbeb1a6ef)
    at /mozilla-central/xpcom/threads/nsThread.cpp:627
#4  0x41d02400 in NS_ProcessNextEvent (thread=0x5a5a5a5a, mayWait=false)
    at /objdir-gecko/unagi/central_debug/xpcom/build/nsThreadUtils.cpp:238
#5  0x419ee652 in mozilla::ipc::MessagePump::Run (this=0x404024c0, aDelegate=0x404310c0)
    at /mozilla-central/ipc/glue/MessagePump.cpp:82
#6  0x41d80e46 in MessageLoop::RunInternal (this=0x404310c0)
    at /mozilla-central/ipc/chromium/src/base/message_loop.cc:219
#7  0x41d80ea6 in MessageLoop::RunHandler (this=0x404310c0)
    at /mozilla-central/ipc/chromium/src/base/message_loop.cc:212
#8  MessageLoop::Run (this=0x404310c0) at /mozilla-central/ipc/chromium/src/base/message_loop.cc:186
#9  0x41924206 in nsBaseAppShell::Run (this=0x44025c40) at /mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:163
#10 0x418169ca in nsAppStartup::Run (this=0x44194b20)
    at /mozilla-central/toolkit/components/startup/nsAppStartup.cpp:289
#11 0x40d53316 in XREMain::XRE_mainRun (this=0xbeb1a984) at /mozilla-central/toolkit/xre/nsAppRunner.cpp:3879
#12 0x40d56022 in XREMain::XRE_main (this=0xbeb1a984, argc=<value optimized out>, argv=<value optimized out>, aAppData=0x21174)
    at /mozilla-central/toolkit/xre/nsAppRunner.cpp:3946
#13 0x40d561d2 in XRE_main (argc=1, argv=0xbeb1cb84, aAppData=0x21174, aFlags=<value optimized out>)
    at /mozilla-central/toolkit/xre/nsAppRunner.cpp:4147
#14 0x00009b2e in do_main (argc=1, argv=0xbeb1cb84) at /mozilla-central/b2g/app/nsBrowserApp.cpp:168
#15 main (argc=1, argv=0xbeb1cb84) at /mozilla-central/b2g/app/nsBrowserApp.cpp:261
(gdb)
Comment on attachment 747377 [details] [diff] [review]
Patch 2(v2): Implementation in BluetoothHfpManager

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

Looks good. You may need to rebase because bug 860166 has landed.
Attachment #747377 - Flags: review?(echou) → review+
Attachment #747270 - Flags: review?(echou) → review+
The crash I mentioned in comment 21 should be also fixed in bug 860166. Will try again after rebasing.
We need Gaia support to call the new Bluetooth DOM API to control SCO. The followup is bug 870683.
Whiteboard: [status: needs review] → [fix-in-birch]
Attached patch b2g18 patch 1Splinter Review
Attached patch b2g18 patch 2Splinter Review
Attached patch b2g18 patch 3Splinter Review
Note that these patches must be landed after Bug 860166.
Ryan, thanks for your help. I did miss the changes in nsIDOMBluetoothAdapter.idl in b2g18 patch 1.

I remember that we should roll the uuid for idl when interface changes. So, I'm going to provide one more patch for b2g18 branch to fix it. Please help me to land it b2g18. If it's not necessary to roll the uuid, then it should be fine and just ignore my new patch. Thanks. :)
Flags: needinfo?(ryanvm)
I checked the latest b2g18_v1_0_1, b2g18, and m-c, and I found something is missing in m-c patch 2. :(
Thanks, Ryan.
Unable to verify. Lack of resources. Need Bluetooth PTS tool to verify.
Marking as QARegressExclude.
Whiteboard: [fixed-in-birch] → [fixed-in-birch] QARegressExclude
You need to log in before you can comment on or make changes to this bug.