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.

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

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

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

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.

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

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

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

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

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.

Review comments addressed.

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

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

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

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

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

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