Support OPP profile for bluedroid stack

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

Able to connect w/o discovery

Revise connect

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.

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

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

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.

> @@ +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;

Rebase on bug 874266 change.

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

Revise based on reviewer's comment.

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

Revise nits.

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

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

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

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

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

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

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

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

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.