Bug 1548272: Don't look at mFlags in GetOrdered (since this is called on main). r=mjf
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 09 May 2019 15:37:55 +0000
changeset 532093 c75e0b9c3a1cbe4956e3cfd384ab0321a0d37774
parent 532092 074678d618bbbd75055a45de59c0d810bd80f8b0
child 532094 cefcbfd25b82fedbbb010b9499f6334c89bdf18c
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmjf
bugs1548272
milestone68.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 1548272: Don't look at mFlags in GetOrdered (since this is called on main). r=mjf Differential Revision: https://phabricator.services.mozilla.com/D29529
netwerk/sctp/datachannel/DataChannel.cpp
netwerk/sctp/datachannel/DataChannel.h
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -1334,17 +1334,16 @@ bool DataChannelConnection::SendBuffered
 
 // Caller must ensure that length <= SIZE_MAX
 void DataChannelConnection::HandleOpenRequestMessage(
     const struct rtcweb_datachannel_open_request* req, uint32_t length,
     uint16_t stream) {
   RefPtr<DataChannel> channel;
   uint32_t prValue;
   uint16_t prPolicy;
-  uint32_t flags;
 
   mLock.AssertCurrentThreadOwns();
 
   const size_t requiredLength = (sizeof(*req) - 1) + ntohs(req->label_length) +
                                 ntohs(req->protocol_length);
   if (((size_t)length) != requiredLength) {
     LOG(("%s: Inconsistent length: %u, should be %zu", __FUNCTION__, length,
          requiredLength));
@@ -1368,37 +1367,35 @@ void DataChannelConnection::HandleOpenRe
       prPolicy = SCTP_PR_SCTP_TTL;
       break;
     default:
       LOG(("Unknown channel type %d", req->channel_type));
       /* XXX error handling */
       return;
   }
   prValue = ntohl(req->reliability_param);
-  flags =
-      (req->channel_type & 0x80) ? DATA_CHANNEL_FLAGS_OUT_OF_ORDER_ALLOWED : 0;
+  bool ordered = !(req->channel_type & 0x80);
 
   if ((channel = FindChannelByStream(stream))) {
     if (!(channel->mFlags & DATA_CHANNEL_FLAGS_EXTERNAL_NEGOTIATED)) {
       LOG(
           ("ERROR: HandleOpenRequestMessage: channel for stream %u is in state "
            "%d instead of CLOSED.",
            stream, channel->mState));
       /* XXX: some error handling */
     } else {
       LOG(("Open for externally negotiated channel %u", stream));
       // XXX should also check protocol, maybe label
       if (prPolicy != channel->mPrPolicy || prValue != channel->mPrValue ||
-          flags !=
-              (channel->mFlags & DATA_CHANNEL_FLAGS_OUT_OF_ORDER_ALLOWED)) {
+          ordered != channel->mOrdered) {
         LOG(
             ("WARNING: external negotiation mismatch with OpenRequest:"
-             "channel %u, policy %u/%u, value %u/%u, flags %x/%x",
+             "channel %u, policy %u/%u, value %u/%u, ordered %d/%d",
              stream, prPolicy, channel->mPrPolicy, prValue, channel->mPrValue,
-             flags, channel->mFlags));
+             static_cast<int>(ordered), static_cast<int>(channel->mOrdered)));
       }
     }
     return;
   }
   if (stream >= mStreams.Length()) {
     LOG(("%s: stream %u out of bounds (%zu)", __FUNCTION__, stream,
          mStreams.Length()));
     return;
@@ -1406,17 +1403,17 @@ void DataChannelConnection::HandleOpenRe
 
   nsCString label(
       nsDependentCSubstring(&req->label[0], ntohs(req->label_length)));
   nsCString protocol(nsDependentCSubstring(
       &req->label[ntohs(req->label_length)], ntohs(req->protocol_length)));
 
   channel =
       new DataChannel(this, stream, DataChannel::CONNECTING, label, protocol,
-                      prPolicy, prValue, flags, nullptr, nullptr);
+                      prPolicy, prValue, ordered, false, nullptr, nullptr);
   mStreams[stream] = channel;
 
   channel->mState = DataChannel::WAITING_TO_OPEN;
 
   LOG(("%s: sending ON_CHANNEL_CREATED for %s/%s: %u (state %u)", __FUNCTION__,
        channel->mLabel.get(), channel->mProtocol.get(), stream,
        channel->mState));
   Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
@@ -2336,17 +2333,16 @@ already_AddRefed<DataChannel> DataChanne
     const nsACString& label, const nsACString& protocol, Type type,
     bool inOrder, uint32_t prValue, DataChannelListener* aListener,
     nsISupports* aContext, bool aExternalNegotiated, uint16_t aStream) {
   if (!aExternalNegotiated) {
     // aStream == INVALID_STREAM to have the protocol allocate
     aStream = INVALID_STREAM;
   }
   uint16_t prPolicy = SCTP_PR_SCTP_NONE;
-  uint32_t flags;
 
   LOG(
       ("DC Open: label %s/%s, type %u, inorder %d, prValue %u, listener %p, "
        "context %p, external: %s, stream %u",
        PromiseFlatCString(label).get(), PromiseFlatCString(protocol).get(),
        type, inOrder, prValue, aListener, aContext,
        aExternalNegotiated ? "true" : "false", aStream));
   switch (type) {
@@ -2372,24 +2368,19 @@ already_AddRefed<DataChannel> DataChanne
   if (aStream != INVALID_STREAM && aStream < mStreams.Length() &&
       mStreams[aStream]) {
     LOG(("ERROR: external negotiation of already-open channel %u", aStream));
     // XXX How do we indicate this up to the application?  Probably the
     // caller's job, but we may need to return an error code.
     return nullptr;
   }
 
-  flags = !inOrder ? DATA_CHANNEL_FLAGS_OUT_OF_ORDER_ALLOWED : 0;
-
-  RefPtr<DataChannel> channel(
-      new DataChannel(this, aStream, DataChannel::CONNECTING, label, protocol,
-                      prPolicy, prValue, flags, aListener, aContext));
-  if (aExternalNegotiated) {
-    channel->mFlags |= DATA_CHANNEL_FLAGS_EXTERNAL_NEGOTIATED;
-  }
+  RefPtr<DataChannel> channel(new DataChannel(
+      this, aStream, DataChannel::CONNECTING, label, protocol, prPolicy,
+      prValue, inOrder, aExternalNegotiated, aListener, aContext));
 
   MutexAutoLock lock(mLock);  // OpenFinish assumes this
   return OpenFinish(channel.forget());
 }
 
 // Separate routine so we can also call it to finish up from pending opens
 already_AddRefed<DataChannel> DataChannelConnection::OpenFinish(
     already_AddRefed<DataChannel>&& aChannel) {
--- a/netwerk/sctp/datachannel/DataChannel.h
+++ b/netwerk/sctp/datachannel/DataChannel.h
@@ -367,34 +367,41 @@ class DataChannel {
     CLOSING = 2U,
     CLOSED = 3U,
     WAITING_TO_OPEN = 4U
   };
 
   DataChannel(DataChannelConnection* connection, uint16_t stream,
               uint16_t state, const nsACString& label,
               const nsACString& protocol, uint16_t policy, uint32_t value,
-              uint32_t flags, DataChannelListener* aListener,
+              bool ordered, bool negotiated, DataChannelListener* aListener,
               nsISupports* aContext)
       : mListenerLock("netwerk::sctp::DataChannel"),
         mListener(aListener),
         mContext(aContext),
         mConnection(connection),
         mLabel(label),
         mProtocol(protocol),
         mState(state),
         mStream(stream),
         mPrPolicy(policy),
         mPrValue(value),
-        mFlags(flags),
+        mOrdered(ordered),
+        mFlags(0),
         mId(0),
         mIsRecvBinary(false),
         mBufferedThreshold(0),  // default from spec
         mBufferedAmount(0),
         mMainThreadEventTarget(connection->GetNeckoTarget()) {
+    if (!ordered) {
+      mFlags |= DATA_CHANNEL_FLAGS_OUT_OF_ORDER_ALLOWED;
+    }
+    if (negotiated) {
+      mFlags |= DATA_CHANNEL_FLAGS_EXTERNAL_NEGOTIATED;
+    }
     NS_ASSERTION(mConnection, "NULL connection");
   }
 
  private:
   ~DataChannel();
 
  public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DataChannel)
@@ -427,19 +434,17 @@ class DataChannel {
   void SendBinaryBlob(dom::Blob& aBlob, ErrorResult& aRv);
 
   uint16_t GetType() { return mPrPolicy; }
 
   dom::Nullable<uint16_t> GetMaxPacketLifeTime() const;
 
   dom::Nullable<uint16_t> GetMaxRetransmits() const;
 
-  bool GetOrdered() {
-    return !(mFlags & DATA_CHANNEL_FLAGS_OUT_OF_ORDER_ALLOWED);
-  }
+  bool GetOrdered() { return mOrdered; }
 
   void IncrementBufferedAmount(uint32_t aSize, ErrorResult& aRv);
   void DecrementBufferedAmount(uint32_t aSize);
 
   // Amount of data buffered to send
   uint32_t GetBufferedAmount() {
     MOZ_ASSERT(NS_IsMainThread());
     return mBufferedAmount;
@@ -483,16 +488,17 @@ class DataChannel {
 
   RefPtr<DataChannelConnection> mConnection;
   nsCString mLabel;
   nsCString mProtocol;
   uint16_t mState;
   uint16_t mStream;
   uint16_t mPrPolicy;
   uint32_t mPrValue;
+  const bool mOrdered;
   uint32_t mFlags;
   uint32_t mId;
   bool mIsRecvBinary;
   size_t mBufferedThreshold;
   // Read/written on main only. Decremented via message-passing, because the
   // spec requires us to queue a task for this.
   size_t mBufferedAmount;
   nsCString mRecvBuffer;