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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

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

This also blocked "TC_AG_TCA" series test cases.

Comment 2

[Kai-Chih Hu [:khu]]

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.

Comment 3

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

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.

Comment 4

[Kai-Chih Hu [:khu]]

Thank you, Gina. 

In this case, I think it looks like a tef+ case.

Comment 5

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

Attachment: Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

Comment 6

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

Attachment: Patch 2(v1): Implementation in BluetoothHfpManager

Comment 7

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

Attachment: Patch 3(v1): Remove BluetoothScoManager

Comment 8

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

Attachment: Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

Comment 9

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

Attachment: Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

Comment 10

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

Comment on attachment 747266 [details] [diff] [review]
Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter

Three interfaces are added in this patch.

Comment 11

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

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.

Comment 12

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

Attachment: Patch 3(v1): Remove BluetoothScoManager

Remove class BluetoothScoManager

Comment 13

[Eric Chou [:ericchou] [: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'

Comment 14

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

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.

Comment 15

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

Attachment: Patch 2(v2): Implementation in BluetoothHfpManager

Update patch as comments from echou.

Comment 16

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

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 17

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

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

Note that some logic is changed in OnDisconnect().

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 20

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

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.

Comment 21

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

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 22

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

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.

Comment 23

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

The crash I mentioned in comment 21 should be also fixed in bug 860166. Will try again after rebasing.

Comment 24

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

We need Gaia support to call the new Bluetooth DOM API to control SCO. The followup is bug 870683.

Comment 25

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

Attachment: Final patch 1, Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter, r=echou, sr=mrbkap

Comment 26

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

Attachment: Final patch 2, Implementation of ConnectSco/DisconnectSco/IsScoConnected in BluetoothHfpManager, r=echou, sr=mrbkap

Comment 27

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

Attachment: Final patch 3, Remove BluetoothScoManager, r=echou

Comment 28

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

Attachment: Final patch 2, Implementation of ConnectSco/DisconnectSco/IsScoConnected in BluetoothHfpManager, r=echou, sr=mrbkap

Comment 29

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

try server:
https://tbpl.mozilla.org/?tree=Try&rev=fb5bb0c0aa7d
https://tbpl.mozilla.org/?tree=Try&rev=8b837c3d1382

Comment 30

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

http://hg.mozilla.org/projects/birch/rev/7c517de515f6
http://hg.mozilla.org/projects/birch/rev/d9e378c9a11b
http://hg.mozilla.org/projects/birch/rev/aa56d0c01f07

Comment 31

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

Attachment: b2g18 patch 1

Comment 32

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

Attachment: b2g18 patch 2

Comment 33

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

Attachment: b2g18 patch 3

Comment 34

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

Attachment: b2g18_v1_0_1 patch 1

Comment 35

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

Attachment: b2g18_v1_0_1 patch 2

Comment 36

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

Attachment: b2g18_v1_0_1 patch 3

Comment 37

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

Note that these patches must be landed after Bug 860166.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c517de515f6
https://hg.mozilla.org/mozilla-central/rev/d9e378c9a11b
https://hg.mozilla.org/mozilla-central/rev/aa56d0c01f07

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9bcf08f31a56
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e6991923537
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c1835326ac3a

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/98bb9be2bbba
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/226258283ff7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f8b5fb65f5a5

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage. Bad time to mention that the branch-specific patches also didn't apply cleanly?

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4c06280df29a
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fab75f5db17d

Comment 41

[Ryan VanderMeulen [:RyanVM]]

The b2g18 patch 1 was missing a few rather important lines in for nsIDOMBluetoothAdapter.idl. Details details...

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0590172e959f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6a86c543b604
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed914f801af6

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a2cff60baac2
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8d20b1c20aa7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f45effe15482

Comment 42

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

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

Comment 43

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

Attachment: b2g18 patch 1 (roll uuid in idl)

Comment 44

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

Attachment: Final patch 2 (add function NotifyStatusChaged in BluetoothHfpManager)

I checked the latest b2g18_v1_0_1, b2g18, and m-c, and I found something is missing in m-c patch 2. :(

Comment 45

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

Pushed to birch.

http://hg.mozilla.org/projects/birch/rev/2745b9feab44

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2745b9feab44

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bfba8196af9d

Comment 48

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

Thanks, Ryan.

Comment 49

[Mila Davydova]

Unable to verify. Lack of resources. Need Bluetooth PTS tool to verify.
Marking as QARegressExclude.