Bug 936402: Duplicate connection status in UnixSocketImpl, r=bent
authorThomas Zimmermann <tdz@users.sourceforge.net>
Mon, 09 Dec 2013 14:21:18 +0100
changeset 176559 66acce483c48dfd3869aa1240a89a620b1e8aa86
parent 176558 96fad9bdc012439b4462333c72d2503bf6c5335d
child 176560 180befb0718ecc0837860838f6be529735d1144b
push id462
push userraliiev@mozilla.com
push dateTue, 22 Apr 2014 00:22:30 +0000
treeherdermozilla-release@ac5db8c74ac0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbent
bugs936402
milestone28.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 936402: Duplicate connection status in UnixSocketImpl, r=bent 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
@@ -15,17 +15,16 @@
 
 #include "base/eintr_wrapper.h"
 #include "base/message_loop.h"
 
 #include "mozilla/Monitor.h"
 #include "mozilla/Util.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>
@@ -39,23 +38,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());
   }
@@ -241,16 +242,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)
@@ -521,16 +528,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();
 }
 
@@ -558,40 +566,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,
@@ -605,31 +616,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
@@ -716,18 +729,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) {
@@ -760,19 +772,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!");
@@ -787,30 +797,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;
@@ -841,50 +851,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)
 {
@@ -931,17 +945,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));
@@ -957,17 +971,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.