915533 - [Bluetoooth][User story] bluedroid stack OPP feature

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Support OPP profile for bluedroid stack

Comment 1

[Ben Tian (inactive)]

Attachment: WIP patch v1

Move socket files. Plan to integrate bludroid-version UnixSocket.cpp into BluetoothSocket.cpp and discard BluetoothSocketConnector.

Comment 2

[Ben Tian (inactive)]

Attachment: WIP patch v2

Comment 3

[Ben Tian (inactive)]

Attachment: WIP patch v3

Comment 4

[Ben Tian (inactive)]

Attachment: WIP patch v4

Able to connect w/o discovery

Comment 6

[Ben Tian (inactive)]

Attachment: WIP patch v5

Revise connect

Comment 7

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

We need to get accept srv_sock fd.
I just trace the flow:
on_srv_rfc_connect->send_app_connect_signal->socket_send_fd

And socket fd will pass through cmsg.

Comment 8

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

If there is an incoming rfcomm connection, bluedroid will accept connection, the flow of bluedroid accept socket as the following:
1. Call create_srv_accept__rfc_slot
2. Send Connect Signal to fd (from listen)
3. Sending app file descriptor to app server to accept() the connection.
See btif_sock_rfc.c, send_app_connect_signal
4. In btif_sock_util.c, the function sock_send_fd will send fd through cmsg
    cmsg = CMSG_FIRSTHDR(&msg);
    cmsg->cmsg_level = SOL_SOCKET;
    cmsg->cmsg_type = SCM_RIGHTS;
    cmsg->cmsg_len = CMSG_LEN(sizeof send_fd);
    memcpy(CMSG_DATA(cmsg), &send_fd, sizeof send_fd);

Comment 9

[Ben Tian (inactive)]

Attachment: WIP patch v6 (1/2): move files

Comment 10

[Ben Tian (inactive)]

Attachment: WIP patch v6 (2/2): rfcomm socket

Comment 11

[Ben Tian (inactive)]

Attachment: Patch 1/2 (v1): Move files

Comment 12

[Ben Tian (inactive)]

Attachment: Patch 2/2 (v1): Bluetooth Socket

Please refer to bluedroid/BluetoothSocket.h for connect/listen steps in overview.

Comment 13

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

Comment on attachment 8339840 [details] [diff] [review]
Patch 2/2 (v1): Bluetooth Socket

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

Hi Ben, please see my comments below. Thanks.

::: dom/bluetooth/bluedroid/BluetoothOppManager.h
@@ +216,1 @@
>    // versa).

super-nit: please merge this line to the previous line.

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +21,5 @@
>  using namespace mozilla::ipc;
>  USING_BLUETOOTH_NAMESPACE
>  
> +static const size_t MAX_READ_SIZE = 1 << 16;
> +const btsock_interface_t* sBluetoothSocketInterface = nullptr;

Please make it 'static const' as well.

@@ +24,5 @@
> +static const size_t MAX_READ_SIZE = 1 << 16;
> +const btsock_interface_t* sBluetoothSocketInterface = nullptr;
> +
> +// helper functions
> +bool EnsureBluetoothSocketHalLoad()

Please make it static.

@@ +321,5 @@
> +  DroidSocketImpl* mImpl;
> +  UnixSocketRawData* mData;
> +};
> +
> +

super-nit: extra line.

@@ +581,5 @@
>  
> +  // TODO: uuid and service name as arguments
> +  nsAutoCString service_name("OBEX Object Push");
> +  uint8_t UUID_OBEX_OBJECT_PUSH[] = {0x00, 0x00, 0x11, 0x05, 0x00, 0x00, 0x10, 0x00,
> +                                     0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB};

UUID_OBEX_OBJECT_PUSH is used in both Listen() and Connect(). We can put it as a static const uint8_t array outside these two functions.

@@ +608,5 @@
> +  return true;
> +}
> +
> +int16_t
> +BluetoothSocket::ReadInt16(const uint8_t* aData, size_t* aOffset)

ReadInt16/ReadInt32 are helper functions. Since no member variable is used in this function, could it be static in unnamed namespace?

@@ +628,5 @@
> +  return value;
> +}
> +
> +void
> +BluetoothSocket::ReadBdAddress(const uint8_t* aData, size_t* aOffset)

If the reference of mDeviceAddress could be passed into ReadBdAddress(), then this function could be static as well.

@@ +648,5 @@
> +   * 2 socket info messages (20 bytes) to receive at the beginning:
> +   * - 1st message: [channel:4]
> +   * - 2nd message: [size:2][bd address:6][channel:4][connection status:4]
> +   */
> +  if (mReceivedSocketInfoLength >= 20) {

I would suggest that you could define a const varaible (or just #define) to represent magic number '4' and '20'.

@@ +671,5 @@
> +    BT_LOGR("%s: size %d channel %d remote addr %s status %d", __FUNCTION__,
> +      size, channel, NS_ConvertUTF16toUTF8(mDeviceAddress).get(), connectionStatus);
> +
> +    if (connectionStatus != 0) {
> +      OnConnectError();

We can return earlier here.

@@ +720,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mObserver);
>    mObserver->OnSocketDisconnect(this);
>  }

nit: Please leave an extra line at the end of file.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1294,3 @@
>  
> +  // Force to stop discovery, otherwise socket connecting would fail
> +  sBtInterface->cancel_discovery();

This may fail. Do we need to check the return value?

@@ +1299,5 @@
> +  // so we don't need aDeviceAddress here because the target device
> +  // has been determined when calling 'Connect()'. Nevertheless, keep
> +  // it for future use.
> +  BluetoothOppManager* opp = BluetoothOppManager::Get();
> +  NS_ENSURE_TRUE_VOID(opp);

We have to deal with aRunnable before leaving this function.

@@ +1320,5 @@
> +  // so we don't need aDeviceAddress here because the target device
> +  // has been determined when calling 'Connect()'. Nevertheless, keep
> +  // it for future use.
> +  BluetoothOppManager* opp = BluetoothOppManager::Get();
> +  NS_ENSURE_TRUE_VOID(opp);

Ditto.

@@ +1335,5 @@
>  BluetoothServiceBluedroid::ConfirmReceivingFile(
>    const nsAString& aDeviceAddress, bool aConfirm,
>    BluetoothReplyRunnable* aRunnable)
>  {
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");

nit: MOZ_ASSERT(), please.

@@ +1342,5 @@
> +  // so we don't need aDeviceAddress here because the target device
> +  // has been determined when calling 'Connect()'. Nevertheless, keep
> +  // it for future use.
> +  BluetoothOppManager* opp = BluetoothOppManager::Get();
> +  NS_ENSURE_TRUE_VOID(opp);

Ditto.

Comment 14

[Ben Tian (inactive)]

> @@ +671,5 @@
> > +    BT_LOGR("%s: size %d channel %d remote addr %s status %d", __FUNCTION__,
> > +      size, channel, NS_ConvertUTF16toUTF8(mDeviceAddress).get(), connectionStatus);
> > +
> > +    if (connectionStatus != 0) {
> > +      OnConnectError();
> 
> We can return earlier here.
> 
But we return right after the if-statement. Do you think it's clearer to write as following?

  } else if (mReceivedSocketInfoLength == TOTAL_SOCKET_INFO_LENGTH) {
    ...

    if (connectionStatus != 0) {
      OnConnectError();
      return true;
    }

    if (mIsServer) {
      // Connect client fd on IO thread
      XRE_GetIOMessageLoop()->PostTask(FROM_HERE,
                                       new SocketConnectClientFdTask(mImpl));
    }
    OnConnectSuccess();
  }
  return true;

Comment 15

[Ben Tian (inactive)]

Attachment: [final] Patch 1/2: Move files, r=echou

Rebase on bug 874266 change.

Comment 16

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

>   But we return right after the if-statement. Do you think it's clearer to write as following?
>
>   } else if (mReceivedSocketInfoLength == TOTAL_SOCKET_INFO_LENGTH) {
>     ...
> 
>     if (connectionStatus != 0) {
>       OnConnectError();
>       return true;
>     }
> 
>     if (mIsServer) {
>       // Connect client fd on IO thread
>       XRE_GetIOMessageLoop()->PostTask(FROM_HERE,
>                                        new SocketConnectClientFdTask(mImpl));
>     }
>     OnConnectSuccess();
>   }
>   return true;

Yes, it seems better to me. Nested if-statement is really bothering and can't be read easily.

Comment 17

[Ben Tian (inactive)]

Attachment: Patch 2/2 (v2): Bluetooth Socket

Revise based on reviewer's comment.

Comment 18

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

Comment on attachment 8340282 [details] [diff] [review]
Patch 2/2 (v2): Bluetooth Socket

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

r=me with nits addressed. Nevertheless, in this patch, a part of implementation of class UnixSocketImpl is copied to class BluetoothSocket, and the code has been slightly modified to fit the need of completing Bluedroid porting. It's ok to me for now since it does work but apparently not good. I would like to suggest that you should file a followup for refining related code with a more generic solution. That would be definitely better for further maintainence.

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +651,5 @@
> +}
> +
> +bool
> +BluetoothSocket::ReceiveSocketInfo(
> +                  nsAutoPtr<mozilla::ipc::UnixSocketRawData>& aMessage)

nit: since "using namespace mozilla::ipc" his used, you can just use UnixSocketRawData here.

@@ +730,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mObserver);
>    mObserver->OnSocketDisconnect(this);
>  }

nit: please leave an extra line at the end of file

Comment 19

[Ben Tian (inactive)]

Attachment: [final] Patch 2/2: Bluetooth Socket, r=echou

Revise nits.

Comment 20

[Ben Tian (inactive)]

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

Comment 21

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

https://hg.mozilla.org/integration/b2g-inbound/rev/2faed81ad05f
https://hg.mozilla.org/integration/b2g-inbound/rev/22ad4bbd595d

Comment 22

[Carsten Book [:Tomcat]]

had to backout this changes for failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=31307523&tree=B2g-Inbound - not sure might be also clobber related at least for unagi maybe

Comment 23

[Ben Tian (inactive)]

Attachment: Patch 3: clobber change

Comment 24

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

Comment on attachment 8340943 [details] [diff] [review]
Patch 3: clobber change

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

r=me and also confirmed with sheriff Tomcat.

Comment 25

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

https://hg.mozilla.org/integration/b2g-inbound/rev/89917e35e1b9
https://hg.mozilla.org/integration/b2g-inbound/rev/78489815fc56
https://hg.mozilla.org/integration/b2g-inbound/rev/73b504802a4b

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89917e35e1b9
https://hg.mozilla.org/mozilla-central/rev/78489815fc56
https://hg.mozilla.org/mozilla-central/rev/73b504802a4b

Comment 27

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

Attachment: patch 4: v1: Remove files that were accidentally restored

Files which should have been removed in patch 1 were accidentally restored because of code tree merge. This followup removes those files again.

Comment 28

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

Attachment: patch 4: v2: Remove files that were accidentally restored

* Removed 2 extra files: BluetoothUnixSocketConnector.h & BluetoothUnixSocketConnector.cpp

Comment 29

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

Comment on attachment 8341549 [details] [diff] [review]
patch 4: v2: Remove files that were accidentally restored

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

Should be safe. :)

Comment 30

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

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=eb1504f7b6a3

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb1504f7b6a3

Comment 32

[Walter Chen[:ypwalter][:wachen]]

I don't see issues on nexus4 for OPP profile related tests.
Tests tagged are done, and test cases modified minorly according to this change.

Verified fixed.