776182 - [b2g-bluetooth] Socket communication for bluetooth devices (Needed for HFP/FTP Support)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

From echou's original comment in bug 756299:

---

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

---

Bug 756299 will give us the basic ability to create a socket, and listen on a setting for when we should create a socket to filter audio through on the phone. This bug adds read/write capabilities to the sockets so that we can communicate with other devices.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: WIP (v1) - Socket I/O for Bluetooth

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Socket I/O for Bluetooth (v2)

Since this is mostly IPC socket code that will at some point be a more general XPCOM socket handler, pointing at cjones.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The part 1 patch here is going to be applying the changes I would have requested in bug 756299.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

>diff --git a/ipc/Makefile.in b/ipc/Makefile.in

> ifdef MOZ_B2G_BT #{
>-DIRS += dbus
>+DIRS += dbus socket
> endif #}

If this is going to be our UNIX-ish socket API, we need to build it
unconditionally there.  Put your HAVE_BLUETOOTHs in the impl.  Build
this code if XP_UNIX.

>diff --git a/ipc/socket/Makefile.in b/ipc/socket/Makefile.in

This is somewhat bikeshed-y, but calling this "socket" is
overpromising a bit.  The functionality here is a superset of "IPC
sockets" (ipc/socket), but a subset of full network sockets.  It's
also not obvious on inspection why this code would only be
conditionally built (Hey, I'm windows, I have sockets too!).

Let's |hg mv| this to ipc/unixsocket.  That's also not entirely
accurate but gets closer, IMHO.

>diff --git a/ipc/socket/Socket.cpp b/ipc/socket/Socket.cpp

Only skimmed this but it looks mostly OK.

>diff --git a/ipc/socket/Socket.h b/ipc/socket/Socket.h

>+int
>+GetNewSocket(int type, const char* aAddress, int channel, bool auth, bool encrypt);

This is changing enough that it's not worth spending too much time on,
but in case some of it stays around

 - s/Get/Create/ --- you can't return an existing socket from here, so
   "Get" is confusing.

 - |int type| --- always use strong types in C++ when possible.

 - assuming that |channel|/|auth|/|encrypt| are BT-specific?  If so,
   the better thing to do here is have a "connect parameters" struct
   that's subclassed for each socket type.  This captures both dynamic
   type (no |type| param needed) and specific configuration.

 - prefer returning a strong type wrapping fd to bare int.  But the
   later patch does that, sort of, I think.

 - documentation, dammit! ;)

>+int
>+CloseSocket(int aFd);

Without a matching strong type returned by Create(), there's no reason
to use this instead of POSIX close(), and indeed there's no way to
enforce this API contract.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 660552 [details] [diff] [review]
Socket I/O for Bluetooth (v2)

>diff --git a/ipc/socket/Socket.h b/ipc/socket/Socket.h

>+struct SocketRawData
>+{

>+  SocketRawData(const char* str) :
>+    mCurrentWriteOffset(0)
>+  {
>+    memcpy(mData, str, strlen(str));
>+    mSize = strlen(str);   

Is there a socket consumer that wants to always send valid
null-terminated C strings of length < MAX_DATA_SIZE?  Either way,
let's get rid of this interface, because it's a giant footgun
security-wise.  If such a consumer exists, let's give it a separate
interface for sending string data.

>+  }
>+};
>+
>+class SocketConsumer;
>+
>+bool
>+ConnectSocket(SocketConsumer* s, int aType, const char* aAddress, int aChannel, bool aAuth, bool aEncrypt);
>+

Let's use a more C++-y interface with stronger types

 // Base of all address types.
 struct SocketAddress
 {
   UnixSocketAddress* AsUnix() {
     return (mType == UNIX) ? static_cast<...>(this) : nullptr;
   }
   // etc. for other types

 private:
   SocketAddress(AddressType aType) : mType(aType) {}
   enum AddressType { UNIX, ... } mType;
 }

 struct UnixSocketAddress : public SocketAddress
 {
    UnixSocketAddress(/* blah .*/)
     : SocketAddress(UNIX)
     , /* blah */
    {}
 };

 // etc for other address types

Then we have one function for opening a socket

  bool Dial(const SocketAddress& aAddress, SocketConsumer* aConsumer);

Clients would do something like

  RefPtr<SocketConsumer> me(new Blah());
  if (!Dial(UnixAddress("/dev/socket/rild", ..), me)) {
    return;
  }

and internal to Socket.cpp, you can switch on the dynamic type of
SocketAddress

 if (UnixSocketAddress* unix = aAddress.AsUnix()) {
   // ...
 } else if () {
   // ...

Make sense?

>+bool
>+CloseSocket(SocketConsumer* s);
> 

We don't need this anymore.

>+class SocketConsumer : public RefCounted<SocketConsumer>
>+{
>+public:
>+  SocketConsumer()
>+  {}
>+  virtual ~SocketConsumer()
>+  {
>+    CloseSocket(this);

It's pretty hard to get this kind of nontrivial-cleanup-from-dtor code
right.  I'll make an alternate suggestion below.

>+  }
>+  virtual void ReceiveSocketData(SocketRawData* aMessage) = 0;
>+  void SendSocketData(SocketRawData* aMessage);
>+  int mFd;

Instead of storing the fd directly here, let's
 - add an explicit Close() method here
 - keep around a private |SocketImpl* mImpl| pointer (weak ref)
 - have SockImpl keep a strong ref to its SocketConsumer, and also
   store all the implementation state like the fd and file watcher
   data

We need to be very careful about racy destruction here, since there's
state spread across two threads.  Having a separate impl object lets
us run all the teardown code on it, after just nulling out the
reference to the SocketConsumer.  So any incoming data packets will
just be dropped on the main thread until the SocketImpl is cleaned up
on the IO thread.

The actual meat of this patch looks mostly like what I was expecting,
but I think the requested reorganization will change things enough
that I'd prefer to closely review the meat there.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, meant to make this explicit: since SocketImpl is just a bare pointer in Socket.h, we only need to forward declare it and its impl can all live in Socket.cpp.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Move ipc/socket to ipc/unixsocket

Dividing out the dir/file moves from the actual patch, since I don't know if hg has the same problems git does with combination moves/changes losing history.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Move Socket.* to UnixSocket.*

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v3) - Socket I/O and Connector Implementation

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 663652 [details] [diff] [review]
Patch 1 (v1) - Move ipc/socket to ipc/unixsocket

Bugzilla doesn't think this is actually a patch, which is worrisome.  But sure.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 663655 [details] [diff] [review]
Patch 3 (v3) - Socket I/O and Connector Implementation

I'm having a little trouble figuring what's supposed to go where, and there aren't docs in UnixSocket.h to straighten me out.  Let's get that documented and then re-review.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v4) - Socket I/O and Connector Implementation

Added comments for UnixSocket.h, fixed some race issues, shuffled functions.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v5) - Socket I/O and Connector Implementation

Fixed initialization conditions that caused crash when socket connection was not successful. Updated blatantly wrong function comments to be slightly less so.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v6) - Socket I/O and Connector Implementation

Change UnixSocketImpl to an autoptr to make semantics similar to other pointers.`1

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 664282 [details] [diff] [review]
Patch 3 (v6) - Socket I/O and Connector Implementation


>diff --git a/dom/bluetooth/BluetoothUnixSocketConnector.cpp b/dom/bluetooth/BluetoothUnixSocketConnector.cpp
>new file mode 100644
>index 0000000..eae977a
>--- /dev/null
>+++ b/dom/bluetooth/BluetoothUnixSocketConnector.cpp
>@@ -0,0 +1,159 @@
>+/* -*- 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/. */
>+
>+#include "BluetoothUnixSocketConnector.h"
>+

Move this #include above "nsThreadUtils.h" below.

I don't feel comfortable reviewing this code.  Let's have Eric review
this part.  The usage of the API looks OK.

>diff --git a/dom/bluetooth/BluetoothUnixSocketConnector.h b/dom/bluetooth/BluetoothUnixSocketConnector.h

>+class BluetoothUnixSocketConnector : public mozilla::ipc::UnixSocketConnector
>+{
>+public:
>+  BluetoothUnixSocketConnector(BluetoothSocketType aType, int aChannel,
>+                               bool aAuth, bool aEncrypt);
>+  virtual ~BluetoothUnixSocketConnector()
>+  { 
>+  }  

Nit: |virtual ~BluetoothUnixSocketConnector() { }|

>+  virtual int Create();
>+  virtual bool ConnectInternal(int aFd, const char* aAddress);

These should be MOZ_OVERRIDE.

>diff --git a/dom/bluetooth/linux/BluetoothDBusService.cpp b/dom/bluetooth/linux/BluetoothDBusService.cpp

>+  // xxx qdot: Just return something for desktop, until we have a parser for the

File a followup for this and "FIXME/bug XXXXXX" here.

>diff --git a/ipc/unixsocket/Makefile.in b/ipc/unixsocket/Makefile.in

> EXPORTS_mozilla/ipc = \
>-  Socket.h \
>+	UnixSocket.h \
>   $(NULL)
> 
> CPPSRCS += \
>-  Socket.cpp \
>+	UnixSocket.cpp \

Nit: the build system style is to use two spaces instead of a tab.

>diff --git a/ipc/unixsocket/UnixSocket.cpp b/ipc/unixsocket/UnixSocket.cpp

>+#include "nsString.h"
> #include "nsThreadUtils.h"

>+#include "nsTArray.h"
>+#include "mozilla/Monitor.h"
>+#include "mozilla/Util.h"
>+#include "mozilla/FileUtils.h"
>+#include "nsXULAppAPI.h"

Keep these in alphabetical order.

>+class UnixSocketImpl : public MessageLoopForIO::Watcher
>+{
>+public:
>+  UnixSocketImpl(UnixSocketConsumer* aConsumer,
>+                 int aFd) : mConsumer(aConsumer)
>+                          , mFd(aFd)

Nit:

   UnixSocketImpl(UnixSocketConsumer* aConsumer,
                  int aFd)
     : mConsumer(aConsumer)
     , mFd(aFd)

You're not explicitly initializing mIOLoop to null, which is a
potential security bug.

>+  void SetUpIO()
>+  {
>+    mIOLoop = MessageLoopForIO::current();

Assert that mIOLoop is null before we get here.  (Which would have
caught the bug above ;) .)

>+private:
>+  MessageLoopForIO* mIOLoop;
>+  typedef nsTArray<UnixSocketRawData* > UnixSocketRawDataQueue;
>+  UnixSocketRawDataQueue mOutgoingQ;
>+  nsRefPtr<UnixSocketConsumer> mConsumer;
>+  ScopedClose mFd;
>+  nsAutoPtr<UnixSocketRawData> mIncoming;
>+  MessageLoopForIO::FileDescriptorWatcher mReadWatcher;
>+  MessageLoopForIO::FileDescriptorWatcher mWriteWatcher;

You need to document the threading model of this object: which methods
and members can be called/accessed on which thread.

Nit: newline here.

>+  virtual void OnFileCanReadWithoutBlocking(int aFd);
>+  virtual void OnFileCanWriteWithoutBlocking(int aFd);

MOZ_OVERRIDE

>+class KillImplTask : public Task

Nit: DestroyImpl.

Instead of making full subclasses for these little trivial tasks, you
can instead do

static void
DestroyImpl(UnixSocketImpl* aImpl)
{
  delete mImpl;
}

//...

loop->PostTask(NewRunnableFunction(DestroyImpl, aImpl));

which is much simpler and will be easier to maintain.

This doesn't work for tasks that need to own strong refs, though, like
the one below.

>+class SocketReceiveTask : public nsRunnable

mConsumer isn't thread-safe refcounted, so this is going to be a
security bug.  We shouldn't touch it on the IO thread; that's the
point of the UnixSocketImpl indirection.

In general, use RefPtr<> for RefCounted<> objects.  That's the
long-term supported path.

>+  NS_IMETHOD
>+  Run()
>+  {
>+    mConsumer->ReceiveSocketData(mRawData);

When this is fixed up, you'll want to null-check the consumer before
delivering the data.

>+class SocketSendTask : public Task
>+{
>+public:
>+  SocketSendTask(UnixSocketConsumer* aConsumer, UnixSocketImpl* aImpl,
>+                 UnixSocketRawData* aData)
>+    : mConsumer(aConsumer),

This has the same problem as above, unref off the consumer's thread.
You don't use it here so I'm not sure why we need to pass it around.

>+bool
>+UnixSocketConsumer::SendSocketData(const nsAString& aStr)
>+{
>+  return SendSocketData(NS_ConvertUTF16toUTF8(aStr));

This implementation doesn't make sense --- you're writing different
bytes to the socket than the client requested.

>+bool
>+UnixSocketConsumer::ConnectSocket(UnixSocketConnector& aConnector,
>+                                  const char* aAddress)
> {

>+  if (mImpl) {
>+    NS_WARNING("Socket already connected!");
>+    return false;
>+  }

Any reason not to assert this?  This seems like a pretty bad logic
bug.

>diff --git a/ipc/unixsocket/UnixSocket.h b/ipc/unixsocket/UnixSocket.h

>+struct UnixSocketRawData
>+{

>+  UnixSocketRawData(int aSize) :

size_t

>+class UnixSocketImpl;
>+
>+/** 
>+ * UnixSocketConnector defines the socket creation and connection/listening
>+ * functions for a UnixSocketConsumer. Due to the fact that socket setup can
>+ * vary between protocols (unix sockets, tcp sockets, bluetooth sockets, etc),
>+ * this allows the user to create whatever connection mechanism they need while
>+ * still depending on libevent for non-blocking communication handling.
>+ *
>+ * TODO: Currently only virtual, since we only support bluetooth. Should make
>+ * connection functions for other unix sockets so we can support things like
>+ * RIL.

This approach is fine for bluetooth, but we should have a default set
of connectors for simpler use cases (plain-jane unix sockets, e.g.),
implemented here.

>+ */
>+class UnixSocketConnector
>+{
>+public:
>+  UnixSocketConnector()
>+  {
>+
>+  }

Nit: |UnixSocketConnector() { }|.  and below.

>+  /** 
>+   * Establishs a file descriptor for a socket.
>+   *
>+   *

Nit: remove extra newline here.

>+class UnixSocketConsumer : public RefCounted<UnixSocketConsumer>
>+{
>+public:
>+  UnixSocketConsumer() : mImpl(nullptr)

Nit:

  UnixSocketConsumer()
    : mImpl(nullptr)

>+  virtual ~UnixSocketConsumer()
>+  {
>+    CloseSocket();
>+  }

This can't be called except by the process initiated by an earlier
call to CloseSocket(), right?

>+  
>+  /** 
>+   * Function to be called whenever data is received. This should only be called
>+   * on the main thread.

s/should only/is only/.  Your implementation guarantees this :).

It's not main-thread only, right, but originating-thread-only?  If
this is main-thread-only for now, that's fine.  But if we use this for
RIL we want schlep data off to a non-main-thread.

>+  /** 
>+   * Queue data to be sent to the socket on the IO thread.

This method can only be called on the originating thread.  This is the
client's responsibility.  Note this in the comment.

>+  /** 
>+   * Convenience function for sending strings to the socket (common in bluetooth
>+   * profile usage). Converts to a UnixSocketRawData struct.

Also add a note about threading model here, and below.

>+  /** 
>+   * Queues the internal representation of socket for deletion. Can be called
>+   * from main thread.
>+   *
>+   */
>+  void CloseSocket();

Nit: add a newline here.

>+private:
>+  // This will complain since our implementation is hidden.

Huh?

This is looking pretty good but there are some pretty serious bugs we
need to fix.  r- for security bugs.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v7) - Socket I/O and Connector Implementation

Review comments addressed.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 664387 [details] [diff] [review]
Patch 3 (v7) - Socket I/O and Connector Implementation

Eric, if you could take a look at the BluetoothUnixSocketConnector, that'd be great. It's based on your old Bluetooth code anyways. :) Let me know if you need any explanation on what's going on, though it's actually decently documented in UnixSocket now.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 664387 [details] [diff] [review]
Patch 3 (v7) - Socket I/O and Connector Implementation

>diff --git a/ipc/unixsocket/UnixSocket.cpp b/ipc/unixsocket/UnixSocket.cpp

>+class SocketReceiveTask : public nsRunnable

>+  NS_IMETHOD
>+  Run()

Nit: style is |NS_IMETHOD Run()|, on one line.

>+  {
>+    MOZ_ASSERT(mImpl->mConsumer);

We can't assert this, because Close() might race with incoming data.
Need to null-check and bail.

>diff --git a/ipc/unixsocket/UnixSocket.h b/ipc/unixsocket/UnixSocket.h

>+private:
>+  // This will complain at compile time on gcc since our implementation is
>+  // hidden.
>+  nsAutoPtr<UnixSocketImpl> mImpl;

Ah --- that's because your UnixSocketConsumer dtor can't see the
UnixSocketImpl dtor that it's trying to call.  Move the definition of
the dtor into UnixSocket.cpp.

Very nice!  r=me with those.

Comment 19

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

Comment on attachment 664387 [details] [diff] [review]
Patch 3 (v7) - Socket I/O and Connector Implementation

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

Looks good to me. r+ after questions are explained or code revised. Thank you.

::: dom/bluetooth/BluetoothUnixSocketConnector.cpp
@@ +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/. */

Do we need to use Android's Apache license as same as here? I said that because this file is very similar to android_bluetooth_BluetoothSocket.cpp. Sorry if it's a stupid question, I don't know much about license stuff. :)

::: ipc/unixsocket/UnixSocket.h
@@ +44,5 @@
> +    mSize(aSize),
> +    mCurrentWriteOffset(0)
> +  {
> +  }
> +

It seems that users are supposed to assign mData on their own. Could there be another constructor to initialize mData?

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #19)
> Comment on attachment 664387 [details] [diff] [review]
> Patch 3 (v7) - Socket I/O and Connector Implementation
> 
> Review of attachment 664387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. r+ after questions are explained or code revised. Thank
> you.
> 
> ::: dom/bluetooth/BluetoothUnixSocketConnector.cpp
> @@ +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/. */
> 
> Do we need to use Android's Apache license as same as here? I said that
> because this file is very similar to android_bluetooth_BluetoothSocket.cpp.
> Sorry if it's a stupid question, I don't know much about license stuff. :)

There's never such a thing as a stupid license question, they're notoriously difficult to figure out. And, you're completely right. I've updated the license. Thanks for pointing that out!

> ::: ipc/unixsocket/UnixSocket.h
> @@ +44,5 @@
> > +    mSize(aSize),
> > +    mCurrentWriteOffset(0)
> > +  {
> > +  }
> > +
> 
> It seems that users are supposed to assign mData on their own. Could there
> be another constructor to initialize mData?

Sure, the question is, what should it take? uint8_t* + size? I was thinking we'd add these as we added profiles, since it's hard to derive context otherwise.

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> >+private:
> >+  // This will complain at compile time on gcc since our implementation is
> >+  // hidden.
> >+  nsAutoPtr<UnixSocketImpl> mImpl;
> 
> Ah --- that's because your UnixSocketConsumer dtor can't see the
> UnixSocketImpl dtor that it's trying to call.  Move the definition of
> the dtor into UnixSocket.cpp.

Nah, it's because when I wrapped it in an nsAutoPtr template, the template's destructor can't see the dtor yet.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Huh, I thought that's what I said :).

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=7b837b196ca1

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43ed8da00ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4abff203299
https://hg.mozilla.org/integration/mozilla-inbound/rev/4489b74f8399

Comment 24

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/a43ed8da00ac
https://hg.mozilla.org/mozilla-central/rev/b4abff203299
https://hg.mozilla.org/mozilla-central/rev/4489b74f8399