Bug 936402 - Duplicate connection status in UnixSocketImpl. r=bent, a=koi+
authorThomas Zimmermann <tdz@users.sourceforge.net>
Mon, 09 Dec 2013 14:21:18 +0100
changeset 175203 14ee95c5c5239baeb3fab6e5ecbbc4c9d257f57a
parent 175202 6bb84d0bc17078bb6f1f846efde5f7d2072c032d
child 175204 cd7afa66a5b4532893af85c6c4613df4c3c9b8b7
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbent, koi
bugs936402
milestone28.0a2
Bug 936402 - Duplicate connection status in UnixSocketImpl. r=bent, a=koi+ UnixSocketImpl, which mostly runs on the I/O thread, doesn't control its reference to UnixSocketConsumer. If the connection status is stored in UnixSocketConsumer, the I/O thread can't read it safely. This patch duplicates the connection status in UnixSocketImpl, where reading from the I/O thread is safe. Methods of UnixSocketImpl don't need to access mConsumer any longer to obtain the connection status.
ipc/unixsocket/UnixSocket.cpp
ipc/unixsocket/UnixSocket.h
--- a/ipc/unixsocket/UnixSocket.cpp
+++ b/ipc/unixsocket/UnixSocket.cpp
@@ -14,17 +14,16 @@
 #include <sys/socket.h>
 
 #include "base/eintr_wrapper.h"
 #include "base/message_loop.h"
 
 #include "mozilla/Monitor.h"
 #include "mozilla/FileUtils.h"
 #include "nsString.h"
-#include "nsThreadUtils.h"
 #include "nsTArray.h"
 #include "nsXULAppAPI.h"
 
 static const size_t MAX_READ_SIZE = 1 << 16;
 
 #undef LOG
 #if defined(MOZ_WIDGET_GONK)
 #include <android/log.h>
@@ -38,23 +37,25 @@ static const int SOCKET_RETRY_TIME_MS = 
 
 namespace mozilla {
 namespace ipc {
 
 class UnixSocketImpl : public MessageLoopForIO::Watcher
 {
 public:
   UnixSocketImpl(UnixSocketConsumer* aConsumer, UnixSocketConnector* aConnector,
-                 const nsACString& aAddress)
+                 const nsACString& aAddress,
+                 SocketConnectionStatus aConnectionStatus)
     : mConsumer(aConsumer)
     , mIOLoop(nullptr)
     , mConnector(aConnector)
     , mShuttingDownOnIOThread(false)
     , mAddress(aAddress)
     , mDelayedConnectTask(nullptr)
+    , mConnectionStatus(aConnectionStatus)
   {
   }
 
   ~UnixSocketImpl()
   {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(IsShutdownOnMainThread());
   }
@@ -240,16 +241,22 @@ private:
    * Address struct of the socket currently in use
    */
   sockaddr_any mAddr;
 
   /**
    * Task member for delayed connect task. Should only be access on main thread.
    */
   CancelableTask* mDelayedConnectTask;
+
+  /**
+   * Socket connection status. Duplicate from UnixSocketConsumer. Should only
+   * be accessed on I/O thread.
+   */
+  SocketConnectionStatus mConnectionStatus;
 };
 
 template<class T>
 class DeleteInstanceRunnable : public nsRunnable
 {
 public:
   DeleteInstanceRunnable(T* aInstance)
   : mInstance(aInstance)
@@ -520,16 +527,17 @@ UnixSocketImpl::Accept()
       return;
     }
 
     if (!mConnector->SetUpListenSocket(mFd)) {
       NS_WARNING("Could not set up listen socket!");
       nsRefPtr<OnSocketEventTask> t =
         new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
       NS_DispatchToMainThread(t);
+      mConnectionStatus = SOCKET_DISCONNECTED;
       return;
     }
 
   }
 
   SetUpIO();
 }
 
@@ -557,40 +565,43 @@ UnixSocketImpl::Connect()
     return;
   }
 
   // Select non-blocking IO.
   if (-1 == fcntl(mFd.get(), F_SETFL, O_NONBLOCK)) {
     nsRefPtr<OnSocketEventTask> t =
       new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
     NS_DispatchToMainThread(t);
+    mConnectionStatus = SOCKET_DISCONNECTED;
     return;
   }
 
   ret = connect(mFd.get(), (struct sockaddr*)&mAddr, mAddrSize);
 
   if (ret) {
     if (errno == EINPROGRESS) {
       // Select blocking IO again, since we've now at least queue'd the connect
       // as nonblock.
       int current_opts = fcntl(mFd.get(), F_GETFL, 0);
       if (-1 == current_opts) {
         NS_WARNING("Cannot get socket opts!");
         mFd.reset(-1);
         nsRefPtr<OnSocketEventTask> t =
           new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
         NS_DispatchToMainThread(t);
+        mConnectionStatus = SOCKET_DISCONNECTED;
         return;
       }
       if (-1 == fcntl(mFd.get(), F_SETFL, current_opts & ~O_NONBLOCK)) {
         NS_WARNING("Cannot set socket opts to blocking!");
         mFd.reset(-1);
         nsRefPtr<OnSocketEventTask> t =
           new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
         NS_DispatchToMainThread(t);
+        mConnectionStatus = SOCKET_DISCONNECTED;
         return;
       }
 
       // Set up a write watch to make sure we receive the connect signal
       MessageLoopForIO::current()->WatchFileDescriptor(
         mFd.get(),
         false,
         MessageLoopForIO::WATCH_WRITE,
@@ -604,31 +615,33 @@ UnixSocketImpl::Connect()
     }
 #if DEBUG
     LOG("Socket connect errno=%d\n", errno);
 #endif
     mFd.reset(-1);
     nsRefPtr<OnSocketEventTask> t =
       new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
     NS_DispatchToMainThread(t);
+    mConnectionStatus = SOCKET_DISCONNECTED;
     return;
   }
 
   if (!SetSocketFlags()) {
     return;
   }
 
   if (!mConnector->SetUp(mFd)) {
     NS_WARNING("Could not set up socket!");
     return;
   }
 
   nsRefPtr<OnSocketEventTask> t =
     new OnSocketEventTask(this, OnSocketEventTask::CONNECT_SUCCESS);
   NS_DispatchToMainThread(t);
+  mConnectionStatus = SOCKET_CONNECTED;
 
   SetUpIO();
 }
 
 bool
 UnixSocketImpl::SetSocketFlags()
 {
   // Set socket addr to be reused even if kernel is still waiting to close
@@ -715,18 +728,17 @@ UnixSocketConsumer::CloseSocket()
 }
 
 void
 UnixSocketImpl::OnFileCanReadWithoutBlocking(int aFd)
 {
   MOZ_ASSERT(!NS_IsMainThread());
   MOZ_ASSERT(!mShuttingDownOnIOThread);
 
-  SocketConnectionStatus status = mConsumer->GetConnectionStatus();
-  if (status == SOCKET_CONNECTED) {
+  if (mConnectionStatus == SOCKET_CONNECTED) {
     // Read all of the incoming data.
     while (true) {
       nsAutoPtr<UnixSocketRawData> incoming(new UnixSocketRawData(MAX_READ_SIZE));
 
       ssize_t ret = read(aFd, incoming->mData, incoming->mSize);
       if (ret <= 0) {
         if (ret == -1) {
           if (errno == EINTR) {
@@ -759,19 +771,17 @@ UnixSocketImpl::OnFileCanReadWithoutBloc
       // If ret is less than MAX_READ_SIZE, there's no
       // more data in the socket for us to read now.
       if (ret < ssize_t(MAX_READ_SIZE)) {
         return;
       }
     }
 
     MOZ_CRASH("We returned early");
-  }
-
-  if (status == SOCKET_LISTENING) {
+  } else if (mConnectionStatus == SOCKET_LISTENING) {
     int client_fd = accept(mFd.get(), (struct sockaddr*)&mAddr, &mAddrSize);
 
     if (client_fd < 0) {
       return;
     }
 
     if (!mConnector->SetUp(client_fd)) {
       NS_WARNING("Could not set up socket!");
@@ -786,30 +796,30 @@ UnixSocketImpl::OnFileCanReadWithoutBloc
       return;
     }
 
     mIOLoop = nullptr;
 
     nsRefPtr<OnSocketEventTask> t =
       new OnSocketEventTask(this, OnSocketEventTask::CONNECT_SUCCESS);
     NS_DispatchToMainThread(t);
+    mConnectionStatus = SOCKET_CONNECTED;
 
     SetUpIO();
   }
 }
 
 void
 UnixSocketImpl::OnFileCanWriteWithoutBlocking(int aFd)
 {
   MOZ_ASSERT(!NS_IsMainThread());
   MOZ_ASSERT(!mShuttingDownOnIOThread);
 
   MOZ_ASSERT(aFd >= 0);
-  SocketConnectionStatus status = mConsumer->GetConnectionStatus();
-  if (status == SOCKET_CONNECTED) {
+  if (mConnectionStatus == SOCKET_CONNECTED) {
     // Try to write the bytes of mCurrentRilRawData.  If all were written, continue.
     //
     // Otherwise, save the byte position of the next byte to write
     // within mCurrentWriteOffset, and request another write when the
     // system won't block.
     //
     while (true) {
       UnixSocketRawData* data;
@@ -840,50 +850,54 @@ UnixSocketImpl::OnFileCanWriteWithoutBlo
           MessageLoopForIO::WATCH_WRITE,
           &mWriteWatcher,
           this);
         return;
       }
       mOutgoingQ.RemoveElementAt(0);
       delete data;
     }
-  } else if (status == SOCKET_CONNECTING) {
+  } else if (mConnectionStatus == SOCKET_CONNECTING) {
     int error, ret;
     socklen_t len = sizeof(error);
     ret = getsockopt(mFd.get(), SOL_SOCKET, SO_ERROR, &error, &len);
 
     if (ret || error) {
       NS_WARNING("getsockopt failure on async socket connect!");
       mFd.reset(-1);
       nsRefPtr<OnSocketEventTask> t =
         new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
       NS_DispatchToMainThread(t);
+      mConnectionStatus = SOCKET_DISCONNECTED;
       return;
     }
 
     if (!SetSocketFlags()) {
       mFd.reset(-1);
       nsRefPtr<OnSocketEventTask> t =
         new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
       NS_DispatchToMainThread(t);
+      mConnectionStatus = SOCKET_DISCONNECTED;
       return;
     }
 
     if (!mConnector->SetUp(mFd)) {
       NS_WARNING("Could not set up socket!");
       mFd.reset(-1);
       nsRefPtr<OnSocketEventTask> t =
         new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
       NS_DispatchToMainThread(t);
+      mConnectionStatus = SOCKET_DISCONNECTED;
       return;
     }
 
     nsRefPtr<OnSocketEventTask> t =
       new OnSocketEventTask(this, OnSocketEventTask::CONNECT_SUCCESS);
     NS_DispatchToMainThread(t);
+    mConnectionStatus = SOCKET_CONNECTED;
 
     SetUpIO();
   }
 }
 
 void
 UnixSocketConsumer::GetSocketAddr(nsAString& aAddrStr)
 {
@@ -930,17 +944,17 @@ UnixSocketConsumer::ConnectSocket(UnixSo
   nsAutoPtr<UnixSocketConnector> connector(aConnector);
 
   if (mImpl) {
     NS_WARNING("Socket already connecting/connected!");
     return false;
   }
 
   nsCString addr(aAddress);
-  mImpl = new UnixSocketImpl(this, connector.forget(), addr);
+  mImpl = new UnixSocketImpl(this, connector.forget(), addr, SOCKET_CONNECTING);
   MessageLoop* ioLoop = XRE_GetIOMessageLoop();
   mConnectionStatus = SOCKET_CONNECTING;
   if (aDelayMs > 0) {
     SocketDelayedConnectTask* connectTask = new SocketDelayedConnectTask(mImpl);
     mImpl->SetDelayedConnectTask(connectTask);
     MessageLoop::current()->PostDelayedTask(FROM_HERE, connectTask, aDelayMs);
   } else {
     ioLoop->PostTask(FROM_HERE, new SocketConnectTask(mImpl));
@@ -956,17 +970,18 @@ UnixSocketConsumer::ListenSocket(UnixSoc
 
   nsAutoPtr<UnixSocketConnector> connector(aConnector);
 
   if (mImpl) {
     NS_WARNING("Socket already connecting/connected!");
     return false;
   }
 
-  mImpl = new UnixSocketImpl(this, connector.forget(), EmptyCString());
+  mImpl = new UnixSocketImpl(this, connector.forget(), EmptyCString(),
+                             SOCKET_LISTENING);
   mConnectionStatus = SOCKET_LISTENING;
   XRE_GetIOMessageLoop()->PostTask(FROM_HERE,
                                    new SocketAcceptTask(mImpl));
   return true;
 }
 
 } // namespace ipc
 } // namespace mozilla
--- a/ipc/unixsocket/UnixSocket.h
+++ b/ipc/unixsocket/UnixSocket.h
@@ -17,16 +17,17 @@
 #include <bluetooth/sco.h>
 #include <bluetooth/l2cap.h>
 #include <bluetooth/rfcomm.h>
 #endif
 #include <stdlib.h>
 #include "nsString.h"
 #include "nsAutoPtr.h"
 #include "mozilla/RefPtr.h"
+#include "nsThreadUtils.h"
 
 namespace mozilla {
 namespace ipc {
 
 union sockaddr_any {
   sockaddr_storage storage; // address-family only
   sockaddr_un un;
   sockaddr_in in;
@@ -160,16 +161,17 @@ class UnixSocketConsumer : public RefCou
 {
 public:
   UnixSocketConsumer();
 
   virtual ~UnixSocketConsumer();
 
   SocketConnectionStatus GetConnectionStatus() const
   {
+    MOZ_ASSERT(NS_IsMainThread());
     return mConnectionStatus;
   }
 
   /**
    * Function to be called whenever data is received. This is only called on the
    * main thread.
    *
    * @param aMessage Data received from the socket.