839370 - [b2g-bluetooth] Update SDP if we failed to get service channel during the process of creating sockets

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

If we failed to get create a socket since no proper service channel is returned from bluez, we try to refresh SDP and retry to create the socket again.

Comment 1

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

We may need this feature in some scenarios. For example, we pair our phone with a ubuntu laptop, in which the default setting of personal file sharing over Bluetooth is disabled. So we don't have the service channel of Oop recorded in our bluez. After enabling the setting, we don't need to unpair and pair again and can set file to the laptop.

Comment 2

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

I should describe the patch in this way.

With this patch, we don't need to unpair and pair again to get new service record in bluez after enabling the setting on the laptop.

Comment 3

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

Attachment: Patch 1(v1): Update SDP if we failed to get service channel during the process of creating sockets

Comment 4

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

Since Bluetooth code has been largely revised, such as profile managers are not UnixSocketConsumer anymore, we need a new patch which is rebased on current codebase. The patches are almost ready.

Comment 5

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

Attachment: patch 1: v1: Implement function UpdateSdpRecords()

Comment 6

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

Attachment: patch 2: v1: Implement callback function OnUpdateSdpRecords() in BluetoothProfileManagers

Comment 7

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

Comment on attachment 751532 [details] [diff] [review]
patch 1: v1: Implement function UpdateSdpRecords()

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

Would like to review again after patch updated and questions answered.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2754,5 @@
> +  BluetoothProfileManagerBase* mManager;
> +};
> +
> +void
> +DiscoverServicesCb(DBusMessage* aMsg, void* aData)

Since names of other callback functions are ended with Callback, can we rename this function to keep consistency?

@@ +2777,5 @@
> +                                       BluetoothProfileManagerBase* aManager)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mConnection);
> +  MOZ_ASSERT(!aDeviceAddress.IsEmpty());

Maybe we can also check aManger here to make sure it isn't null.

@@ +2784,5 @@
> +
> +  UpdateSdpRecordsParameters* params = new UpdateSdpRecordsParameters();
> +  params->mDeviceObjectPath = objectPath;
> +  params->mManager = aManager;
> +

Can we create an instance of OnUpdateSdpRecordsRunnable and keep |objectPath| and |aManager| as data member? Then, pass it into the following dbus call as the 4th argument.

In this way, in the callback function, which is DiscoverServicesCb, we can cast the runnble back to type |OnUpdateSdpRecordsRunnable| and dispatch it to main thread. No need to create a struct for parameters, right?

Comment 8

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

Attachment: patch 1: v2: Implement function UpdateSdpRecords()

* Problems fixed and suggestion is taken!

Comment 9

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

Attachment: patch 2: v2: Implement callback function OnUpdateSdpRecords() in BluetoothProfileManagers

Comment 10

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

Comment on attachment 752127 [details] [diff] [review]
patch 2: v2: Implement callback function OnUpdateSdpRecords() in BluetoothProfileManagers

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

Overall is good. Comments for hfp manager and opp manager are pretty similar, so I only write comments on hfp part. Would like to see the patch again after updated.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1010,4 @@
>      BluetoothUuidHelper::GetString(BluetoothServiceClass::HEADSET, uuid);
>    }
>  
> +  mUpdateSdpRecordsTicket = 1;

I think we don't need to assign |mUpdateSdpRecordsTicket| here again, right?

@@ +1512,5 @@
> +  // Since we have updated SDP records of the target device, we should
> +  // try to get the channel of target service again.
> +  if (NS_FAILED(bs->GetServiceChannel(aDeviceAddress, uuid, this))) {
> +    BluetoothValue v;
> +    DispatchBluetoothReply(mRunnable, v,

How about this?

DispatchBluetoothReply(mRunnable, BluetoothValue(), ...);

So that we don't have to declare a BluetoothValue object before dispatching reply.

@@ +1514,5 @@
> +  if (NS_FAILED(bs->GetServiceChannel(aDeviceAddress, uuid, this))) {
> +    BluetoothValue v;
> +    DispatchBluetoothReply(mRunnable, v,
> +                           NS_LITERAL_STRING(ERR_SERVICE_CHANNEL_NOT_FOUND));
> +    return;

The return statement can be discarded.

@@ +1534,4 @@
>    BluetoothValue v;
>  
>    if (aChannel < 0) {
> +    if (mUpdateSdpRecordsTicket-- > 0) {

The conditional statement can be simplified to |mUpdateSdpRecordsTicket--|

Or, if we make mUpdateSdpRecordsTicket as a boolean variable, the conditional statement would be |mUpdateSdpRecordsTicket| and re-assign it to false.

@@ +1546,3 @@
>      return;
>    }
>  

I found there's a redundant return statement a few lines after. Can you help to remove it?


  if (!mSocket->Connect(NS_ConvertUTF16toUTF8(aDeviceAddress), aChannel)) {
    DispatchBluetoothReply(mRunnable, v,
                           NS_LITERAL_STRING("SocketConnectionError"));
    mSocket = nullptr;
    Listen();
    return; // not necessarily
  }

::: dom/bluetooth/BluetoothHfpManager.h
@@ +132,5 @@
>    int mNetworkSelectionMode;
>    bool mReceiveVgsFlag;
>    bool mBLDNProcessed;
> +  bool mIsHandsfree;
> +  int mUpdateSdpRecordsTicket;

Can we just make it a boolean variable?

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +270,2 @@
>      BluetoothValue v;
>      DispatchBluetoothReply(aRunnable, v,

Again, we probably can use ctor of BluetoothValue to create an object as parameter.

Comment 11

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

Comment on attachment 752126 [details] [diff] [review]
patch 1: v2: Implement function UpdateSdpRecords()

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

Perfect!

Comment 12

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

Comment on attachment 752126 [details] [diff] [review]
patch 1: v2: Implement function UpdateSdpRecords()

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

Perfect!

Comment 13

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

Attachment: patch 2: v3: Implement callback function OnUpdateSdpRecords() in BluetoothProfileManagers

* nits picked

Comment 14

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

Comment on attachment 752609 [details] [diff] [review]
patch 2: v3: Implement callback function OnUpdateSdpRecords() in BluetoothProfileManagers

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

r+'d with nits addressed.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1014,2 @@
>      BluetoothValue v;
>      DispatchBluetoothReply(aRunnable, v,

You might want to use ctor of BluetoothValue here?

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +1477,5 @@
> +  BluetoothUuidHelper::GetString(BluetoothServiceClass::OBJECT_PUSH, uuid);
> +
> +  if (NS_FAILED(bs->GetServiceChannel(aDeviceAddress, uuid, this))) {
> +    BluetoothValue v;
> +    DispatchBluetoothReply(mRunnable, v,

The same with above.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2531,5 @@
>      opp->Connect(aDeviceAddress, aRunnable);
>    } else {
>      BluetoothValue v;
> +    DispatchBluetoothReply(aRunnable, v,
> +                           NS_LITERAL_STRING("UnknownProfileError"));

The same with above.

Comment 15

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

birch:

https://hg.mozilla.org/projects/birch/rev/3aae540739b8
https://hg.mozilla.org/projects/birch/rev/f8aadf7fa01b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8aadf7fa01b
https://hg.mozilla.org/mozilla-central/rev/3aae540739b8

Comment 17

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

Nominate this as leo+ since we need this to fix bug 884253 in b2g18.

Comment 18

[leo.bugzilla.gecko]

Nominate this as leo+.
Thanks.

Comment 19

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

Attachment: patch 1: for b2g18

* b2g18-specific patch

Comment 20

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

Attachment: patch 2: for b2g18

* b2g18-specific patch

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7a85f4b71618
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5967e6bd0d64

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7a85f4b71618
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5967e6bd0d64