Backed out 2 changesets (bug 1584695, bug 1584751) for Assertion failures at at /builds/worker/workspace/build/src/xpcom/threads/BlockingResourceBase.cpp on a CLOSED TREE
authorOana Pop Rus <opoprus@mozilla.com>
Thu, 03 Oct 2019 11:38:45 +0300
changeset 496159 6b35b1db71b319eac8fd707bae54d3434bee7ab0
parent 496158 4f1cf3253f14f7afc2df077945ad82bbc56976fe
child 496160 ec52c8aa2b8e2291f542b392a345a528f95d4407
push id97062
push useropoprus@mozilla.com
push dateThu, 03 Oct 2019 08:39:43 +0000
treeherderautoland@6b35b1db71b3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1584695, 1584751
milestone71.0a1
backs outfac79eb65900623382089842962e545aabb1a042
dd5211c4ded2f2212e9ca990f4026c9e4dd1b5b1
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
Backed out 2 changesets (bug 1584695, bug 1584751) for Assertion failures at at /builds/worker/workspace/build/src/xpcom/threads/BlockingResourceBase.cpp on a CLOSED TREE Backed out changeset fac79eb65900 (bug 1584751) Backed out changeset dd5211c4ded2 (bug 1584695)
netwerk/sctp/datachannel/DataChannel.cpp
netwerk/sctp/datachannel/DataChannel.h
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -283,17 +283,17 @@ static void debug_printf(const char* for
     }
     va_end(ap);
   }
 }
 
 DataChannelConnection::~DataChannelConnection() {
   DC_DEBUG(("Deleting DataChannelConnection %p", (void*)this));
   // This may die on the MainThread, or on the STS thread
-  ASSERT_WEBRTC(GetReadyState() == CLOSED);
+  ASSERT_WEBRTC(mState == CLOSED);
   MOZ_ASSERT(!mMasterSocket);
   MOZ_ASSERT(mPending.GetSize() == 0);
 
   if (!IsSTSThread()) {
     ASSERT_WEBRTC(NS_IsMainThread());
 
     if (mInternalIOThread) {
       // Avoid spinning the event thread from here (which if we're mainthread
@@ -640,63 +640,29 @@ void DataChannelConnection::SetMaxMessag
             aMaxMessageSize != mMaxMessageSize ? "yes" : "no"));
 }
 
 uint64_t DataChannelConnection::GetMaxMessageSize() { return mMaxMessageSize; }
 
 #ifdef MOZ_PEERCONNECTION
 
 bool DataChannelConnection::ConnectToTransport(const std::string& aTransportId,
-                                               const bool aClient,
-                                               const uint16_t aLocalPort,
-                                               const uint16_t aRemotePort) {
-  MutexAutoLock lock(mLock);
+                                               bool aClient, uint16_t localport,
+                                               uint16_t remoteport) {
+  DC_DEBUG(("Connect DTLS local %u, remote %u", localport, remoteport));
+
   MOZ_ASSERT(mMasterSocket,
              "SCTP wasn't initialized before ConnectToTransport!");
-  static const auto paramString =
-      [](const std::string& tId, const Maybe<bool>& client,
-         const uint16_t localPort, const uint16_t remotePort) -> std::string {
-    std::ostringstream stream;
-    stream << "Transport ID: '" << tId << "', Role: '"
-           << (client ? (client.value() ? "client" : "server") : "")
-           << "', Local Port: '" << localPort << "', Remote Port: '"
-           << remotePort << "'";
-    return stream.str();
-  };
-
-  const auto params =
-      paramString(aTransportId, Some(aClient), aLocalPort, aRemotePort);
-  DC_DEBUG(("ConnectToTransport connecting DTLS transport with parameters: %s",
-            params.c_str()));
-
-  const auto currentReadyState = GetReadyState();
-  if (currentReadyState == OPEN) {
-    if (aTransportId == mTransportId && mAllocateEven.isSome() &&
-        mAllocateEven.value() == aClient && mLocalPort == aLocalPort &&
-        mRemotePort == aRemotePort) {
-      DC_WARN(
-          ("Skipping attempt to connect to an already OPEN transport with "
-           "identical parameters."));
-      return true;
-    }
-    DC_WARN(
-        ("Attempting to connect to an already OPEN transport, because "
-         "different parameters were provided."));
-    DC_WARN(("Original transport parameters: %s",
-             paramString(mTransportId, mAllocateEven, mLocalPort, aRemotePort)
-                 .c_str()));
-    DC_WARN(("New transport parameters: %s", params.c_str()));
-  }
   if (NS_WARN_IF(aTransportId.empty())) {
     return false;
   }
 
-  mLocalPort = aLocalPort;
-  mRemotePort = aRemotePort;
-  SetReadyState(CONNECTING);
+  mLocalPort = localport;
+  mRemotePort = remoteport;
+  mState = CONNECTING;
   mAllocateEven = Some(aClient);
 
   // Could be faster. Probably doesn't matter.
   while (auto channel = mChannels.Get(INVALID_STREAM)) {
     mChannels.Remove(channel);
     channel->mStream = FindFreeStream();
     if (channel->mStream != INVALID_STREAM) {
       mChannels.Insert(channel);
@@ -730,19 +696,18 @@ void DataChannelConnection::SetSignals(c
 void DataChannelConnection::TransportStateChange(
     const std::string& aTransportId, TransportLayer::State aState) {
   if (aState == TransportLayer::TS_OPEN && aTransportId == mTransportId) {
     CompleteConnect();
   }
 }
 
 void DataChannelConnection::CompleteConnect() {
+  DC_DEBUG(("dtls open"));
   MutexAutoLock lock(mLock);
-
-  DC_DEBUG(("dtls open"));
   ASSERT_WEBRTC(IsSTSThread());
   if (!mMasterSocket) {
     return;
   }
 
   struct sockaddr_conn addr;
   memset(&addr, 0, sizeof(addr));
   addr.sconn_family = AF_CONN;
@@ -792,17 +757,17 @@ void DataChannelConnection::CompleteConn
       }
     }
     if (r < 0) {
       if (errno == EINPROGRESS) {
         // non-blocking
         return;
       }
       DC_ERROR(("usrsctp_connect failed: %d", errno));
-      SetReadyState(CLOSED);
+      mState = CLOSED;
     } else {
       // We fire ON_CONNECTION via SCTP_COMM_UP when we get that
       return;
     }
   }
   // Note: currently this doesn't actually notify the application
   Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
       DataChannelOnMessageAvailable::ON_CONNECTION, this)));
@@ -926,20 +891,17 @@ bool DataChannelConnection::Listen(unsig
   memset((void*)&addr, 0, sizeof(addr));
 #  ifdef HAVE_SIN_LEN
   addr.sin_len = sizeof(struct sockaddr_in);
 #  endif
   addr.sin_family = AF_INET;
   addr.sin_port = htons(port);
   addr.sin_addr.s_addr = htonl(INADDR_ANY);
   DC_DEBUG(("Waiting for connections on port %u", ntohs(addr.sin_port)));
-  {
-    MutexAutoLock lock(mLock);
-    SetReadyState(CONNECTING);
-  }
+  mState = CONNECTING;
   if (usrsctp_bind(mMasterSocket, reinterpret_cast<struct sockaddr*>(&addr),
                    sizeof(struct sockaddr_in)) < 0) {
     DC_ERROR(("***Failed userspace_bind"));
     return false;
   }
   if (usrsctp_listen(mMasterSocket, 1) < 0) {
     DC_ERROR(("***Failed userspace_listen"));
     return false;
@@ -947,21 +909,17 @@ bool DataChannelConnection::Listen(unsig
 
   DC_DEBUG(("Accepting connection"));
   addr_len = 0;
   if ((mSocket = usrsctp_accept(mMasterSocket, nullptr, &addr_len)) ==
       nullptr) {
     DC_ERROR(("***Failed accept"));
     return false;
   }
-
-  {
-    MutexAutoLock lock(mLock);
-    SetReadyState(OPEN);
-  }
+  mState = OPEN;
 
   struct linger l;
   l.l_onoff = 1;
   l.l_linger = 0;
   if (usrsctp_setsockopt(mSocket, SOL_SOCKET, SO_LINGER, (const void*)&l,
                          (socklen_t)sizeof(struct linger)) < 0) {
     DC_WARN(("Couldn't set SO_LINGER on SCTP socket"));
   }
@@ -993,20 +951,18 @@ bool DataChannelConnection::Connect(cons
 #  endif
 #  ifdef HAVE_SIN6_LEN
   addr6.sin6_len = sizeof(struct sockaddr_in6);
 #  endif
   addr4.sin_family = AF_INET;
   addr6.sin6_family = AF_INET6;
   addr4.sin_port = htons(port);
   addr6.sin6_port = htons(port);
-  {
-    MutexAutoLock lock(mLock);
-    SetReadyState(CONNECTING);
-  }
+  mState = CONNECTING;
+
 #  if !defined(__Userspace_os_Windows)
   if (inet_pton(AF_INET6, addr, &addr6.sin6_addr) == 1) {
     if (usrsctp_connect(mMasterSocket,
                         reinterpret_cast<struct sockaddr*>(&addr6),
                         sizeof(struct sockaddr_in6)) < 0) {
       DC_ERROR(("*** Failed userspace_connect"));
       return false;
     }
@@ -1048,20 +1004,18 @@ bool DataChannelConnection::Connect(cons
       DC_ERROR(("*** Illegal destination address."));
     }
   }
 #  endif
 
   mSocket = mMasterSocket;
 
   DC_DEBUG(("connect() succeeded!  Entering connected mode"));
-  {
-    MutexAutoLock lock(mLock);
-    SetReadyState(OPEN);
-  }
+  mState = OPEN;
+
   // Notify Connection open
   // XXX We need to make sure connection sticks around until the message is
   // delivered
   DC_DEBUG(("%s: sending ON_CONNECTION for %p", __FUNCTION__, this));
   Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
       DataChannelOnMessageAvailable::ON_CONNECTION, this,
       (DataChannel*)nullptr)));
   return true;
@@ -1791,26 +1745,24 @@ void DataChannelConnection::HandleMessag
           "Unhandled message of length %zu PPID %u on stream %u received (%s).",
           length, ppid, stream, (flags & MSG_EOR) ? "complete" : "partial"));
       break;
   }
 }
 
 void DataChannelConnection::HandleAssociationChangeEvent(
     const struct sctp_assoc_change* sac) {
-  mLock.AssertCurrentThreadOwns();
-
   uint32_t i, n;
-  const auto readyState = GetReadyState();
+
   switch (sac->sac_state) {
     case SCTP_COMM_UP:
       DC_DEBUG(("Association change: SCTP_COMM_UP"));
-      if (readyState == CONNECTING) {
+      if (mState == CONNECTING) {
         mSocket = mMasterSocket;
-        SetReadyState(OPEN);
+        mState = OPEN;
 
         DC_DEBUG(("Negotiated number of incoming streams: %" PRIu16,
                   sac->sac_inbound_streams));
         DC_DEBUG(("Negotiated number of outgoing streams: %" PRIu16,
                   sac->sac_outbound_streams));
         mNegotiatedIdLimit =
             std::max(mNegotiatedIdLimit,
                      static_cast<size_t>(std::max(sac->sac_outbound_streams,
@@ -1818,20 +1770,20 @@ void DataChannelConnection::HandleAssoci
 
         Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
             DataChannelOnMessageAvailable::ON_CONNECTION, this)));
         DC_DEBUG(("DTLS connect() succeeded!  Entering connected mode"));
 
         // Open any streams pending...
         ProcessQueuedOpens();
 
-      } else if (readyState == OPEN) {
+      } else if (mState == OPEN) {
         DC_DEBUG(("DataConnection Already OPEN"));
       } else {
-        DC_ERROR(("Unexpected state: %d", readyState));
+        DC_ERROR(("Unexpected state: %d", mState));
       }
       break;
     case SCTP_COMM_LOST:
       DC_DEBUG(("Association change: SCTP_COMM_LOST"));
       // This association is toast, so also close all the channels -- from
       // mainthread!
       Stop();
       break;
@@ -2397,19 +2349,19 @@ already_AddRefed<DataChannel> DataChanne
   //      -> Try to get a stream
   //      Doesn't fit:
   //         -> RequestMoreStreams && queue
   //      Does fit:
   //         -> open
   // So the Open cases are basically the same
   // Not Open cases are simply queue for non-negotiated, and
   // either change the initial ask or possibly renegotiate after open.
-  const auto readyState = GetReadyState();
-  if (readyState != OPEN || stream >= mNegotiatedIdLimit) {
-    if (readyState == OPEN) {
+
+  if (mState != OPEN || stream >= mNegotiatedIdLimit) {
+    if (mState == OPEN) {
       MOZ_ASSERT(stream != INVALID_STREAM);
       // RequestMoreStreams() limits to MAX_NUM_STREAMS -- allocate extra
       // streams to avoid going back immediately for more if the ask to N, N+1,
       // etc
       int32_t more_needed = stream - ((int32_t)mNegotiatedIdLimit) + 16;
       if (!RequestMoreStreams(more_needed)) {
         // Something bad happened... we're done
         goto request_error_cleanup;
@@ -2625,17 +2577,17 @@ int DataChannelConnection::SendMsgIntern
 }
 
 // Caller must ensure that length <= UINT32_MAX
 // Returns a POSIX error code.
 int DataChannelConnection::SendDataMsgInternalOrBuffer(DataChannel& channel,
                                                        const uint8_t* data,
                                                        size_t len,
                                                        uint32_t ppid) {
-  if (NS_WARN_IF(channel.GetReadyState() != OPEN)) {
+  if (NS_WARN_IF(channel.mReadyState != OPEN)) {
     return EINVAL;  // TODO: Find a better error code
   }
 
   struct sctp_sendv_spa info = {0};
 
   // General flags
   info.sendv_flags = SCTP_SEND_SNDINFO_VALID;
 
@@ -2784,45 +2736,16 @@ class DataChannelBlobSendRunnable : publ
 
  private:
   // Note: we can be destroyed off the target thread, so be careful not to let
   // this get Released()ed on the temp thread!
   RefPtr<DataChannelConnection> mConnection;
   uint16_t mStream;
 };
 
-static auto readyStateToCStr(const uint16_t state) -> const char* {
-  switch (state) {
-    case DataChannelConnection::CONNECTING:
-      return "CONNECTING";
-    case DataChannelConnection::OPEN:
-      return "OPEN";
-    case DataChannelConnection::CLOSING:
-      return "CLOSING";
-    case DataChannelConnection::CLOSED:
-      return "CLOSED";
-    default: {
-      MOZ_ASSERT(false);
-      return "UNKNOWW";
-    }
-  }
-};
-
-void DataChannelConnection::SetReadyState(const uint16_t aState) {
-  mLock.AssertCurrentThreadOwns();
-
-  DC_DEBUG(
-      ("DataChannelConnection labeled %s (%p) switching connection state %s -> "
-       "%s",
-       mTransportId.c_str(), this, readyStateToCStr(mState),
-       readyStateToCStr(aState)));
-
-  mState = aState;
-}
-
 void DataChannelConnection::ReadBlob(
     already_AddRefed<DataChannelConnection> aThis, uint16_t aStream,
     nsIInputStream* aBlob) {
   // NOTE: 'aThis' has been forgotten by the caller to avoid releasing
   // it off mainthread; if PeerConnectionImpl has released then we want
   // ~DataChannelConnection() to run on MainThread
 
   // XXX to do this safely, we must enqueue these atomically onto the
@@ -2903,53 +2826,52 @@ void DataChannelConnection::CloseInt(Dat
   MOZ_ASSERT(aChannel);
   RefPtr<DataChannel> channel(aChannel);  // make sure it doesn't go away on us
 
   mLock.AssertCurrentThreadOwns();
   DC_DEBUG(("Connection %p/Channel %p: Closing stream %u",
             channel->mConnection.get(), channel.get(), channel->mStream));
 
   aChannel->mBufferedData.Clear();
-  if (GetReadyState() == CLOSED) {
+  if (mState == CLOSED) {
     // If we're CLOSING, we might leave this in place until we can send a
     // reset.
     mChannels.Remove(channel);
   }
 
-  auto channelState = aChannel->GetReadyState();
   // re-test since it may have closed before the lock was grabbed
-  if (channelState == CLOSED || channelState == CLOSING) {
-    DC_DEBUG(("Channel already closing/closed (%u)", channelState));
+  if (aChannel->mReadyState == CLOSED || aChannel->mReadyState == CLOSING) {
+    DC_DEBUG(("Channel already closing/closed (%u)", aChannel->mReadyState));
     return;
   }
 
   if (channel->mStream != INVALID_STREAM) {
     ResetOutgoingStream(channel->mStream);
-    if (GetReadyState() != CLOSED) {
+    if (mState != CLOSED) {
       // Individual channel is being closed, send reset now.
       SendOutgoingStreamReset();
     }
   }
-  aChannel->SetReadyState(CLOSING);
-  if (GetReadyState() == CLOSED) {
+  aChannel->mReadyState = CLOSING;
+  if (mState == CLOSED) {
     // we're not going to hang around waiting
     channel->StreamClosedLocked();
   }
   // At this point when we leave here, the object is a zombie held alive only by
   // the DOM object
 }
 
 void DataChannelConnection::CloseAll() {
   DC_DEBUG(("Closing all channels (connection %p)", (void*)this));
   // Don't need to lock here
 
   // Make sure no more channels will be opened
   {
     MutexAutoLock lock(mLock);
-    SetReadyState(CLOSED);
+    mState = CLOSED;
   }
 
   // Close current channels
   // If there are runnables, they hold a strong ref and keep the channel
   // and/or connection alive (even if in a CLOSED state)
   for (auto& channel : mChannels.GetAll()) {
     channel->Close();
   }
@@ -3116,57 +3038,43 @@ void DataChannel::DecrementBufferedAmoun
           mListener->NotBuffered(mContext);
         }
       }));
 }
 
 void DataChannel::AnnounceOpen() {
   mMainThreadEventTarget->Dispatch(NS_NewRunnableFunction(
       "DataChannel::AnnounceOpen", [this, self = RefPtr<DataChannel>(this)] {
-        auto state = GetReadyState();
         // Special-case; spec says to put brand-new remote-created DataChannel
         // in "open", but queue the firing of the "open" event.
-        if (state != CLOSING && state != CLOSED && mListener) {
-          SetReadyState(OPEN);
+        if (mReadyState != CLOSING && mReadyState != CLOSED && mListener) {
+          mReadyState = OPEN;
           DC_DEBUG(("%s: sending ON_CHANNEL_OPEN for %s/%s: %u", __FUNCTION__,
                     mLabel.get(), mProtocol.get(), mStream));
           mListener->OnChannelConnected(mContext);
         }
       }));
 }
 
 void DataChannel::AnnounceClosed() {
   mMainThreadEventTarget->Dispatch(NS_NewRunnableFunction(
       "DataChannel::AnnounceClosed", [this, self = RefPtr<DataChannel>(this)] {
-        if (GetReadyState() == CLOSED) {
+        if (mReadyState == CLOSED) {
           return;
         }
-        SetReadyState(CLOSED);
+        mReadyState = CLOSED;
         mBufferedData.Clear();
         if (mListener) {
           DC_DEBUG(("%s: sending ON_CHANNEL_CLOSED for %s/%s: %u", __FUNCTION__,
                     mLabel.get(), mProtocol.get(), mStream));
           mListener->OnChannelClosed(mContext);
         }
       }));
 }
 
-// Set ready state
-void DataChannel::SetReadyState(const uint16_t aState) {
-  MOZ_ASSERT(NS_IsMainThread());
-
-  DC_DEBUG(
-      ("DataChannelConnection labeled %s(%p) (stream %d) changing ready state "
-       "%s -> %s",
-       mLabel.get(), this, mStream, readyStateToCStr(mReadyState),
-       readyStateToCStr(aState)));
-
-  mReadyState = aState;
-}
-
 void DataChannel::SendMsg(const nsACString& aMsg, ErrorResult& aRv) {
   if (!EnsureValidStream(aRv)) {
     return;
   }
 
   SendErrnoToErrorResult(mConnection->SendMsg(mStream, aMsg), aRv);
   if (!aRv.Failed()) {
     IncrementBufferedAmount(aMsg.Length(), aRv);
--- a/netwerk/sctp/datachannel/DataChannel.h
+++ b/netwerk/sctp/datachannel/DataChannel.h
@@ -152,19 +152,18 @@ class DataChannelConnection final : publ
   // These block; they require something to decide on listener/connector
   // (though you can do simultaneous Connect()).  Do not call these from
   // the main thread!
   bool Listen(unsigned short port);
   bool Connect(const char* addr, unsigned short port);
 #endif
 
 #ifdef SCTP_DTLS_SUPPORTED
-  bool ConnectToTransport(const std::string& aTransportId, const bool aClient,
-                          const uint16_t aLocalPort,
-                          const uint16_t aRemotePort);
+  bool ConnectToTransport(const std::string& aTransportId, bool aClient,
+                          uint16_t localport, uint16_t remoteport);
   void TransportStateChange(const std::string& aTransportId,
                             TransportLayer::State aState);
   void CompleteConnect();
   void SetSignals(const std::string& aTransportId);
 #endif
 
   typedef enum {
     RELIABLE = 0,
@@ -199,16 +198,20 @@ class DataChannelConnection final : publ
 
   // Called on data reception from the SCTP library
   // must(?) be public so my c->c++ trampoline can call it
   int ReceiveCallback(struct socket* sock, void* data, size_t datalen,
                       struct sctp_rcvinfo rcv, int flags);
 
   // Find out state
   enum { CONNECTING = 0U, OPEN = 1U, CLOSING = 2U, CLOSED = 3U };
+  uint16_t GetReadyState() {
+    MutexAutoLock lock(mLock);
+    return mState;
+  }
 
   friend class DataChannel;
   Mutex mLock;
 
   void ReadBlob(already_AddRefed<DataChannelConnection> aThis, uint16_t aStream,
                 nsIInputStream* aBlob);
 
   bool SendDeferredMessages();
@@ -224,26 +227,16 @@ class DataChannelConnection final : publ
 
   DataChannelConnection(DataConnectionListener* aListener,
                         nsIEventTarget* aTarget,
                         MediaTransportHandler* aHandler);
 
   bool Init(const uint16_t aLocalPort, const uint16_t aNumStreams,
             const Maybe<uint64_t>& aMaxMessageSize);
 
-  // Caller must hold mLock
-  uint16_t GetReadyState() const {
-    mLock.AssertCurrentThreadOwns();
-
-    return mState;
-  }
-
-  // Caller must hold mLock
-  void SetReadyState(const uint16_t aState);
-
 #ifdef SCTP_DTLS_SUPPORTED
   static void DTLSConnectThread(void* data);
   void SendPacket(std::unique_ptr<MediaPacket>&& packet);
   void SctpDtlsInput(const std::string& aTransportId, MediaPacket& packet);
   static int SctpDtlsOutput(void* addr, void* buffer, size_t length,
                             uint8_t tos, uint8_t set_df);
 #endif
   DataChannel* FindChannelByStream(uint16_t stream);
@@ -482,24 +475,21 @@ class DataChannel {
   uint32_t GetBufferedAmountLowThreshold();
   void SetBufferedAmountLowThreshold(uint32_t aThreshold);
 
   void AnnounceOpen();
   // TODO(bug 843625): Optionally pass an error here.
   void AnnounceClosed();
 
   // Find out state
-  uint16_t GetReadyState() const {
+  uint16_t GetReadyState() {
     MOZ_ASSERT(NS_IsMainThread());
     return mReadyState;
   }
 
-  // Set ready state
-  void SetReadyState(const uint16_t aState);
-
   void GetLabel(nsAString& aLabel) { CopyUTF8toUTF16(mLabel, aLabel); }
   void GetProtocol(nsAString& aProtocol) {
     CopyUTF8toUTF16(mProtocol, aProtocol);
   }
   uint16_t GetStream() { return mStream; }
 
   void SendOrQueue(DataChannelOnMessageAvailable* aMessage);