Closed Bug 756299 Opened 8 years ago Closed 7 years ago

[b2g-bluetooth] Socket connection for bluetooth devices (Needed for SCO/HSP Support)

Categories

(Core :: DOM: Device Interfaces, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro ?
blocking-basecamp +

People

(Reporter: qdot, Assigned: qdot)

References

Details

(Whiteboard: [LOE:M])

Attachments

(2 files, 10 obsolete files)

9.27 KB, patch
Details | Diff | Splinter Review
25.87 KB, patch
Details | Diff | Splinter Review
No description provided.
Summary: Implement Bluetooth Socket object → Socket object for bluetooth
Description: We need the socket interface for bluetooth exposed so that we can implement profiles in JS.
Summary: Socket object for bluetooth → B2G Bluetooth: Socket object for bluetooth
BluetoothSocket will only be created by BluetoothDevice by calling device.createRfcommSocket(). Common web pages cannot create sockets of other types, like L2CAP or SCO. Besides, we have another interface called BluetoothServerSocket. The idea is from Android. Profiles need a server socket to accept connection requests from remote devices.

According to our design, BluetoothSocket and BluetoothServerSocket API should be like:

 // Created by nsIDOMBluetoothDevice
 interface nsIDOMBluetoothSocket : nsISupports
 {
   void close();
   void connect();
   jsval read(in unsigned long length);
   void write(in jsval dataBuffer, in unsigned long offset, in unsigned long length);

   nsIDOMBluetoothDevice getRemoteDevice();
 };

 // Created by nsIDOMBluetoothAdapter
 interface nsIDOMBluetoothServerSocket : nsISupports
 {
   void close();
   nsIDOMBluetoothSocket accept();
 };
Assignee: nobody → echou
Depends on: 730992
No longer depends on: 730990
Assignee: echou → kyle
Ratcheting down the requirements a bit for this specific bug, and creating a new bug on top of it that will involve server sockets and read/write operations to the sockets. This way we can land HSP platform support, and work on audio routing/gaia issues while further HFP/FTP work happens in parallel.
Summary: B2G Bluetooth: Socket object for bluetooth → [b2g-bluetooth] Socket connection for bluetooth devices (HSP Support)
Blocks: 776170
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Summary: [b2g-bluetooth] Socket connection for bluetooth devices (HSP Support) → [b2g-bluetooth] Socket connection for bluetooth devices (SCO/HSP Support)
Priority: -- → P1
Whiteboard: [LOE:M]
Attached patch Patch 1 (v1) - IPC Socket Class (obsolete) — Splinter Review
Attachment #654098 - Flags: review?(mrbkap)
Comment on attachment 654098 [details] [diff] [review]
Patch 1 (v1) - IPC Socket Class

Oooooh this needs some work nevermind.
Attachment #654098 - Flags: review?(mrbkap)
Attachment #654099 - Flags: review?(mrbkap)
Attachment #654100 - Flags: review?(mrbkap)
Attached patch Patch 1 (v2) - IPC Socket Class (obsolete) — Splinter Review
Attachment #654098 - Attachment is obsolete: true
Attachment #654103 - Flags: review?(mrbkap)
Attachment #654099 - Attachment is obsolete: true
Attachment #654106 - Flags: review?(mrbkap)
Attachment #654106 - Attachment description: Patch 2 (v1) - BluetoothSocket DOM Object Boilerplate → Patch 2 (v2) - BluetoothSocket DOM Object Boilerplate
Attachment #654108 - Attachment description: Patch 3 (v1) - Hook up BluetoothSocket creation to BluetoothDevice → Patch 3 (v2) - Hook up BluetoothSocket creation to BluetoothDevice
Forgot to replace macro'd value with enum'd value.
Attachment #654108 - Attachment is obsolete: true
Attachment #654108 - Flags: review?(mrbkap)
Attachment #654110 - Flags: review?(mrbkap)
Comment on attachment 654103 [details] [diff] [review]
Patch 1 (v2) - IPC Socket Class

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

There are a bunch of nits and two real questions here.

::: ipc/socket/Socket.cpp
@@ +1,2 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */

Here and elsewhere: in vim modelines, tw is textwidth, it should be 80.

@@ +36,5 @@
> +
> +static const int TYPE_RFCOMM = 1;
> +static const int TYPE_SCO = 2;
> +static const int TYPE_L2CAP = 3;
> +int get_bdaddr(const char *str, bdaddr_t *ba)

The two helper functions should be static.

@@ +38,5 @@
> +static const int TYPE_SCO = 2;
> +static const int TYPE_L2CAP = 3;
> +int get_bdaddr(const char *str, bdaddr_t *ba)
> +{
> +  char *d = ((char *)ba) + 5, *endp;

Why not use ba->b to get this pointer? Why the reinterpret cast?

@@ +40,5 @@
> +int get_bdaddr(const char *str, bdaddr_t *ba)
> +{
> +  char *d = ((char *)ba) + 5, *endp;
> +  int i;
> +  for(i = 0; i < 6; i++) {

Nit: Space after for. Also, please declare |i| in the header of the loop instead of outside.

@@ +42,5 @@
> +  char *d = ((char *)ba) + 5, *endp;
> +  int i;
> +  for(i = 0; i < 6; i++) {
> +    *d-- = strtol(str, &endp, 16);
> +    if (*endp != ':' && i != 5) {

Is it worth asserting that if i == 5 that *endp == '\0'?

@@ +53,5 @@
> +}
> +
> +void get_bdaddr_as_string(const bdaddr_t *ba, char *str)
> +{
> +  const uint8_t *b = (const uint8_t *)ba;

Same question about the cast.

@@ +55,5 @@
> +void get_bdaddr_as_string(const bdaddr_t *ba, char *str)
> +{
> +  const uint8_t *b = (const uint8_t *)ba;
> +  sprintf(str, "%2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X",
> +      b[5], b[4], b[3], b[2], b[1], b[0]);

Nit: Second line seems underindented.

@@ +66,5 @@
> +  int lm = 0;
> +  int fd = -1;
> +  int sndbuf;
> +
> +    switch (type) {

This switch statement is misindented.

@@ +117,5 @@
> +
> +  int n = 1;
> +  setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &n, sizeof(n));
> +
> +  

Nit: extra newline here.
Attachment #654103 - Flags: review?(mrbkap)
Comment on attachment 654106 [details] [diff] [review]
Patch 2 (v2) - BluetoothSocket DOM Object Boilerplate

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

Nits only.

::: dom/bluetooth/BluetoothServiceUuid.h
@@ +1,5 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   * You can obtain one at http://mozilla.org/MPL/2.0/. */

Super-nit: This is mis-indented.

::: dom/bluetooth/BluetoothSocket.cpp
@@ +62,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1");
> +
> +  if (!rs) {

Another super-nit: the newline should probably go before the declaration here.
Attachment #654106 - Flags: review?(mrbkap) → review+
Comment on attachment 654110 [details] [diff] [review]
Patch 3 (v3) - Hook up BluetoothSocket creation to BluetoothDevice

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

::: dom/bluetooth/BluetoothDevice.cpp
@@ +166,2 @@
>      }
> +    else if (NS_FAILED(bs->RegisterBluetoothSignalHandler(mPath, this))) {

Nit: here and below, you're mixing else styles.

@@ +347,5 @@
> +    nsPIDOMWindow* aWindow = GetOwner();
> +    nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
> +      aWindow :
> +      aWindow->GetCurrentInnerWindow();
> +    NS_ENSURE_TRUE(innerWindow, NS_ERROR_FAILURE);

Why do we only do this security check if we're creating a socket for a headset?

@@ +363,5 @@
> +      do_QueryInterface(innerWindow->GetExtantDocument());
> +    NS_ENSURE_TRUE(document, NS_NOINTERFACE);
> +
> +    // Do security checks. We assume that chrome is always allowed.
> +    if (!nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) {

Isn't this going to have to change for B2G?

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +162,5 @@
> +{
> +  // The object path would be like /org/bluez/2906/hci0/dev_00_23_7F_CB_B4_F1,
> +  // and the adapter path would be the first part of the object path, accoring
> +  // to the example above, it's /org/bluez/2906/hci0.
> +  nsString address(aObjectPath);

You're moving this around, ok. But you've made both of these functions non-static again and undid a bunch of changes that I asked about the last time through. Why?

@@ +248,5 @@
> +                 BluetoothValue& aValue, nsAString& aErrorStr)
> +{
> +  DBusError err;
> +  dbus_error_init(&err);
> +  NS_WARNING("Unpoacking int!");

This probably shouldn't be checked in. Also, "Unpoacking" :)

@@ +916,5 @@
> +                               DBUS_TYPE_INVALID)) {
> +      LOG_AND_FREE_DBUS_ERROR_WITH_MSG(&err, aMsg);
> +      errorStr.AssignLiteral("Cannot parse device path!");
> +    }
> +    v = NS_ConvertUTF8toUTF16(str);

Is str valid if dbus_message_get_args returned false?

@@ +1424,5 @@
>                     DBUS_TYPE_UINT16, &aAttributeId,
>                     DBUS_TYPE_INVALID);
>  
> +  //return reply ? dbus_returns_int32(reply) : -1;
> +  return 1;

??

@@ +1742,5 @@
> +    int fd = mozilla::ipc::GetNewSocket(mType, NS_ConvertUTF16toUTF8(address).get(),
> +                                        channel, mAuth, mEncrypt);
> +    BluetoothValue v;
> +    nsString replyError;
> +    if(fd < 0) {

Space after if, please.
Attachment #654110 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #14)
> @@ +347,5 @@
> > +    nsPIDOMWindow* aWindow = GetOwner();
> > +    nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
> > +      aWindow :
> > +      aWindow->GetCurrentInnerWindow();
> > +    NS_ENSURE_TRUE(innerWindow, NS_ERROR_FAILURE);
> 
> Why do we only do this security check if we're creating a socket for a
> headset?
> 
> @@ +363,5 @@
> > +      do_QueryInterface(innerWindow->GetExtantDocument());
> > +    NS_ENSURE_TRUE(document, NS_NOINTERFACE);
> > +
> > +    // Do security checks. We assume that chrome is always allowed.
> > +    if (!nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) {
> 
> Isn't this going to have to change for B2G?

This use case is weird. Basically, users can't really do anything with an SCO socket, because it has to be set up to use with the android AudioManager. However, I figured we'd want to be able to create sockets the same way no matter what. The way I see this working is:

- The user connects to an HFP socket (required to use the SCO portion of HFP)
- After that, to route to the SCO socket, they access something that uses the Settings API
- There's a settings watcher in shell.js or something that opens the needed socket and does the necessary changes to the AudioManager to route audio to it.

So, the permissions check is basically to make sure that users only create RFCOMM sockets. Any type can be handled in chrome. However, both users and chrome can create the sockets in the same way, users will just be denied access to creating the SCO sockets.
Attached patch Patch 1 (v3) - IPC Socket Class (obsolete) — Splinter Review
Review issues addressed. Only thing not addressed is cast, since we require the explicit cast back to char* for c stdlib string functions (strtok) to work correctly. We could also just convert these out to XPCOM string calls.
Attachment #654103 - Attachment is obsolete: true
Attachment #654761 - Flags: review?(mrbkap)
Nits addressed
Attachment #654106 - Attachment is obsolete: true
Review issues addressed, permissions questions answered earlier.
Attachment #654110 - Attachment is obsolete: true
Attachment #654763 - Flags: review?(mrbkap)
Attachment #654761 - Flags: review?(mrbkap) → review+
Comment on attachment 654763 [details] [diff] [review]
Patch 3 (v4) - Hook up BluetoothSocket creation to BluetoothDevice

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

::: dom/bluetooth/BluetoothDevice.cpp
@@ +325,5 @@
> +    NS_WARNING("BluetoothService not available!");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1");

Nit: extra newline here.

@@ +349,5 @@
> +    NS_ENSURE_TRUE(innerWindow, NS_ERROR_FAILURE);
> +
> +    // Make sure we're being called from a window that we have permission to
> +    // access.
> +    if (!nsContentUtils::CanCallerAccess(innerWindow)) {

This check is redundant with the check for chrome that follows (CanCallerAccess ends up short-circuiting "true" if the caller is chrome).

I'm still not terribly happy about this approach, though. In general, we're trying to reduce the number of C++ security checks. I'd much prefer a chrome-only way of creating SCO sockets (another interface or contractID), as that's more in line with the objcaps approach we've been using elsewhere. On the other hand, I don't want to block this bug on that, so please file a followup bug on doing this more cleanly (i.e. without adding a security check in C++) and let's get this in now.

::: dom/bluetooth/BluetoothService.cpp
@@ +157,5 @@
>    BluetoothSignalObserverList* ol;
>    if (!mBluetoothSignalObserverTable.Get(signal.path(), &ol)) {
> +#if DEBUG
> +    NS_WARNING("No observer registered for path");
> +    NS_WARNING(NS_ConvertUTF16toUTF8(signal.path()).get());

It'd be cleaner to make this a single NS_WARNING.

::: dom/bluetooth/BluetoothService.h
@@ +196,4 @@
>                  const nsAString& aDeviceAddress,
>                  nsAString& aDevicePath) = 0;
>  
> +  virtual nsTArray<PRUint32>

I know this wasn't introduced in this patch, but don't we normally try to avoid returning nsTArrays by value to avoid extra copies?

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +142,5 @@
>  static nsDataHashtable<nsStringHashKey, DBusMessage* > sAuthorizeReqTable;
>  
> +typedef void (*UnpackFunc)(DBusMessage*, DBusError*, BluetoothValue&, nsAString&);
> +
> +static nsString		

Uber-nit: trailing whitespace here and in GetAddressFromObjectPath.
Attachment #654763 - Flags: review?(mrbkap) → review+
Will clean up and submit final patches, but still waiting on bug 783426 to pass before I can land.
Follwups:

- Bug 788526 - BluetoothSocket DOM
- Bug 788524 - nsTArray return cleanup
Follwups, this time without the numeric dyslexia:

- Bug 788256 - BluetoothSocket DOM
- Bug 788254 - nsTArray return cleanup
Attachment #654762 - Attachment is obsolete: true
Attachment #654763 - Attachment is obsolete: true
Added final patches for bringing up IPC socket connections at least, removed DOM classes, patches will go to Bug 788256
Summary: [b2g-bluetooth] Socket connection for bluetooth devices (SCO/HSP Support) → [b2g-bluetooth] Socket connection for bluetooth devices (Needed for SCO/HSP Support)
https://hg.mozilla.org/mozilla-central/rev/b5f9c880652a
https://hg.mozilla.org/mozilla-central/rev/740dd374fca7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
For future reference, this should have had sr from the ipc module owner (me).  We're going to need to clean some things up ...
You need to log in before you can comment on or make changes to this bug.