855623 - Change DataChannel protocol to be Declarative 0 RTT per IETF 86 decision

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Per the decision at IETF 86, change the DataChannel protocol to be a 1-way declarative Open, with support for external negotiation.

Glare is avoided by having sides initiate channels on even or odd.

For the rest, see draft-jesup-rtcweb-data-protocol-04

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) (WIP)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Interdiffs for Queued incoming data support

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) including queuing (WIP)

This is the full patch including the QueuedData interdiff (which I've tested and works)

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add 'protocol' field, and support external negotiation

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: DataChannel DOM changes for adding 'protocol' and external negotiation

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 730959 [details] [diff] [review]
Add 'protocol' field, and support external negotiation

These apply on top of the other patch.  They add the 'protocol' field, and add support for external negotiation.  (I've tested this with the DOM patch and it works.)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 730960 [details] [diff] [review]
DataChannel DOM changes for adding 'protocol' and external negotiation

These are the DOM changes for adding the protocol field (per W3 Interim in Boston), and adding support for external negotiation (per IETF 86 and discussion in the W3)

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Queue createDataChannel() calls made before onconnection and process later

This is in preparation for use with the SDP negotiation (bug 837035), so we can create DataChannelConnections only when the application calls createDataChannel before createOffer().

Comment 9

[Michael Tüxen]

Comment on attachment 730740 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) including queuing (WIP)

Review of attachment 730740 [details] [diff] [review]:
-----------------------------------------------------------------

I think your code doesn't handle the case where the number
of incoming streams and outgoing streams doesn't match.

* Shouldn't take RequestMoreStreams() incoming and outgoing streams
  into account?
* SendOpenRequestMessage() still uses flags, which got removed in the
  message format. The unordered flag has been "integrated" in the
  channel_type.
* HandleOpenRequestMessage also doesn't use the new message format.
  It should check if this there are not duplicate messages on this
  stream and if the stream is odd/even such that reception of it is OK.
  It also assumes that the number of incoming and outgoing streams is the
  same.
* I don't think that req->label is a zero terminated string, at least
  this is not written in the ID. Even if it were, the code should not
  rely on it. You need to take the length of the request into account
  and do input validation on it.

Comment 10

[Michael Tüxen]

Comment on attachment 730959 [details] [diff] [review]
Add 'protocol' field, and support external negotiation

Review of attachment 730959 [details] [diff] [review]:
-----------------------------------------------------------------

* The rtcweb_datachannel_open_request structure doesn't match the
  definition in the ID.
* You can't rely on protocol being zero terminated.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 730960 [details] [diff] [review]
DataChannel DOM changes for adding 'protocol' and external negotiation

>@@ -129,14 +129,15 @@ interface IPeerConnection : nsISupports
>   readonly attribute string localDescription;
>   readonly attribute string remoteDescription;
> 
>   readonly attribute unsigned long iceState;
>   readonly attribute unsigned long readyState;
>   readonly attribute unsigned long sipccState;
> 
>   /* Data channels */
>-  nsIDOMDataChannel createDataChannel(in ACString label,
>+  nsIDOMDataChannel createDataChannel(in ACString label, in ACString protocol,
>     in unsigned short type, in boolean outOfOrderAllowed,
>-    in unsigned short maxTime, in unsigned short maxNum);
>+    in unsigned short maxTime, in unsigned short maxNum,
>+    in boolean externalNegotiated, in unsigned short stream);
What is stream? stream sounds like an object but it is just unsigned short here.
Perhaps some better name for the parameter.



>@@ -86,15 +86,15 @@ interface nsIDOMRTCPeerConnection : nsIS
>   attribute RTCPeerConnectionCallback onremovestream;
>   attribute RTCPeerConnectionCallback onicecandidate;
>   attribute RTCPeerConnectionCallback onstatechange;
>   attribute RTCPeerConnectionCallback ongatheringchange;
>   attribute RTCPeerConnectionCallback onicechange;
> 
>   /* Data channels */
>   nsIDOMDataChannel createDataChannel([optional] in ACString label,
>-    [optional] in jsval options);
>+    [optional] in ACString protocol, in jsval options);

So this breaks all the existing use of createDataChannel, and is not compatible with the spec.
Can we get the spec changed too, assuming it is agreed that createDataChannel will change.
Assuming the spec change will happen, r=me

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Update (interdiffs) in response to review

Update to resolve review issues.  Uses correct request structure and types.  Label/protocol are not/not assumed to be nul-terminated on the wire.  Fixed issue with deferred opens of non-prenegotiated channels

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Michael Tüxen from comment #9)
> Comment on attachment 730740 [details] [diff] [review]
> Update DataChannel protocol to be declarative vs 3-way handshake (per IETF)
> including queuing (WIP)
> 
> Review of attachment 730740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think your code doesn't handle the case where the number
> of incoming streams and outgoing streams doesn't match.
> 
> * Shouldn't take RequestMoreStreams() incoming and outgoing streams
>   into account?

We set mStreams length according to our outgoing stream size.  The recipient doesn't *have* to have a return stream for us to send an Open and use the channel in the outbound direction, as there is no handshake now.  (And in fact, while I wouldn't recommend it, an implementation *could* change their number of outbound streams in response to an Open in order to use the reverse stream).  Our implementation will always answer a request with a matches number of outbound channels.

> * SendOpenRequestMessage() still uses flags, which got removed in the
>   message format. The unordered flag has been "integrated" in the
>   channel_type.

Thanks, looked at that and totally forgot.

> * HandleOpenRequestMessage also doesn't use the new message format.
>   It should check if this there are not duplicate messages on this
>   stream and if the stream is odd/even such that reception of it is OK.
>   It also assumes that the number of incoming and outgoing streams is the
>   same.

External negotiation doesn't have to respect even/odd, even/odd is only enforced when sending an Open message to avoid glare - though external negotiation doesn't send Open messages.  I.e. the protocol should state that each side will allocate and send Open messages only on even or odd Streams.  At least it should be a LOG/warning, but I'm tempted to accept it (be liberal in what you accept) and mark the channel in use.

> * I don't think that req->label is a zero terminated string, at least
>   this is not written in the ID. Even if it were, the code should not
>   rely on it. You need to take the length of the request into account
>   and do input validation on it.

Thus far our implementation has assumed nul-terminated.  Since we're breaking wireline compatibility, this is a good point to fix that.  Also added incoming size validation, and ignore the Open request if the size is smaller than the computed size.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731054 [details] [diff] [review]
Queue createDataChannel() calls made before onconnection and process later

This enables us to call createDataChannel() before createOffer()/ConnectDataConnect() (which is going away), which is the "final" W3 API used to signal we should negotiate an m=application line for datachannels.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Set even/odd streams via DTLS role

Comment 16

[Michael Tüxen]

Comment on attachment 731157 [details] [diff] [review]
Update (interdiffs) in response to review

Review of attachment 731157 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the changes.

::: netwerk/sctp/datachannel/DataChannelProtocol.h
@@ +42,5 @@
>    uint16_t reliability_params;
>    int16_t  priority;
>    uint16_t label_length;
>    uint16_t protocol_length;
>    char     label[1]; // (and protocol) keep VC++ happy...  UTF8 null-terminated strings

Neither label nor protocol needs to be null-terminated. So remove the comment.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 730740 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) including queuing (WIP)

r+ = tuexen

Comment 18

[Ethan Hugg [:ehugg]]

Comment on attachment 730740 [details] [diff] [review]
Update DataChannel protocol to be declarative vs 3-way handshake (per IETF) including queuing (WIP)

Review of attachment 730740 [details] [diff] [review]:
-----------------------------------------------------------------

Just some nits.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +765,4 @@
>    if (limit > MAX_NUM_STREAMS)
>      limit = MAX_NUM_STREAMS;
> +
> +  for (i = mAllocateEven ? 0 : 1; i < limit; i += 2) {

assignment and ?: are very close in operator precedence, wouldn't hurt to use parens here.

@@ +1032,5 @@
>  
> +  channel->mState = DataChannel::WAITING_TO_OPEN;
> +
> +  LOG(("%s: sending ON_CHANNEL_CREATED for %s: %d", __FUNCTION__,
> +       channel->mLabel.get(), stream));

I believe stream is unsigned here.  %d for unsigned happens many times in this file.  You can decide whether that's important.

@@ +1167,5 @@
>    mLock.AssertCurrentThreadOwns();
>  
>    switch (ppid) {
>      case DATA_CHANNEL_PPID_CONTROL:
> +      NS_ENSURE_TRUE_VOID(length >= sizeof(*req)); // Ack is the smallest

I think this comment no longer matches this line

::: netwerk/sctp/datachannel/DataChannel.h
@@ +269,3 @@
>    // channels available from the stack must be negotiated!
> +  bool mAllocateEven;
> +  nsAutoTArray<nsRefPtr<DataChannel>,16> mStreams;

Should this 16 be coordinated with the default we send via SDP?  Currenty from fsm.h (well, after push of 837035)

#define WEBRTC_DATACHANNEL_STREAMS_DEFAULT 16

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add 'protocol' field, and support external negotiation

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731358 [details] [diff] [review]
Add 'protocol' field, and support external negotiation

This should look better

Comment 21

[Ethan Hugg [:ehugg]]

Comment on attachment 731054 [details] [diff] [review]
Queue createDataChannel() calls made before onconnection and process later

Review of attachment 731054 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +626,5 @@
> +    // and we increase the number of streams dynamically as needed.
> +    return NS_OK;
> +  }
> +  mDataConnection = new mozilla::DataChannelConnection(this);
> +  if (!mDataConnection->Init(5000, aNumstreams, true)) {

This number is currently stored in the config - signaling/src/sipcc/core/includes/config.h
and pulled out like this (from gsm.c)
config_get_value(CFGID_SCTP_PORT, &sctp_port, sizeof(sctp_port));
The idea being that later on we could hook it up to about:config so that the use could change their default.

@@ +683,5 @@
>    mozilla::DataChannelConnection::Type theType =
>      static_cast<mozilla::DataChannelConnection::Type>(aType);
>  
> +  // FIX!
> +  nsresult rv = EnsureDataConnection(16);

This could also use WEBRTC_DATACHANNEL_STREAMS_DEFAULT from fsm.h, or that def could be moved to a more centralized location.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: hook up createDataChannel-before-createOffer to SDP generation from bug 837035

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731479 [details] [diff] [review]
hook up createDataChannel-before-createOffer to SDP generation from bug 837035

moved to bug 856319

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Alternate non-compatibility-breaking JS API for createDataChannel

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731482 [details] [diff] [review]
Alternate non-compatibility-breaking JS API for createDataChannel

So we have both in-hand depending on the result of my question to the W3 list

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 731482 [details] [diff] [review]
Alternate non-compatibility-breaking JS API for createDataChannel

So this is not the full patch, since in the patch you added protocol, you changed
also .idl

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Alternate non-compatibility-breaking JS API for createDataChannel

Sorry, it was easier to put up a delta patch to the previous (and I was going to fold them before push), but that's confusing.  Here's the folded dom patch with no API change to createDataChannel

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 731521 [details] [diff] [review]
Alternate non-compatibility-breaking JS API for createDataChannel

Remove the change tonsIDOMRTCPeerConnection.

Comment 29

[Eric Rescorla (:ekr)]

Comment on attachment 731171 [details] [diff] [review]
Set even/odd streams via DTLS role

Review of attachment 731171 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +502,2 @@
>    mState = CONNECTING;
> +  LOG(("Connect DTLS local %d, remote %d, %s", localport, remoteport, mAllocateEven ? "Even" : "Odd"));

The ports are uint16_t. Shouldn't this be "%hu"?

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Even/odd can't be reliably set until the association is up

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Set even/odd streams via DTLS role

Unified patch to replace previous even/odd patch.  Moves even/odd definition to when the association comes up, since we can't reliably know it until then.  Defers setting stream values on queued DataChannels until the association is up and queued datachannels are fully open (and fire open).

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Set even/odd streams via DTLS role (updated)

This patch is a bit more cleaned up (source-wise), though the patch is a bit larger due to some re-arrangement in the "defer assigning streams" code.  It's green in Try.

From the message I plan to send to the mailing lists:

Even/odd roles for the underlying DataChannel protocol are tied to the DTLS roles on the DTLS connection.  These are only available after the DTLS connection is established, and so we will set the even/odd roles when the initial association is established (which is when onconnection fires, and then any queued DataChannels would have onopen fire).

Comment 33

[Eric Rescorla (:ekr)]

Comment on attachment 731613 [details] [diff] [review]
Set even/odd streams via DTLS role (updated)

Review of attachment 731613 [details] [diff] [review]:
-----------------------------------------------------------------

r- for threading issues. I also would like to go over OpenFinish in more detail,
since the logic seems confusing.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +672,5 @@
>    nsresult rv = EnsureDataConnection(aNumstreams);
>  
>    // XXX Fix! Get the correct flow for DataChannel. Also error handling.
>    for (int i = 2; i >= 0; i--) {
>      nsRefPtr<TransportFlow> flow = mMedia->GetTransportFlow(i,false).get();

Not the subject of this patch but this seems wrong. The
specification needs to clearly state which flow will
be used. Even if it doesn't, having this loop suggests
that the two sides might disagree, which clearly isn't
going to work. I don't think that can actually happen,
but then why do need a loop?

@@ +675,5 @@
>    for (int i = 2; i >= 0; i--) {
>      nsRefPtr<TransportFlow> flow = mMedia->GetTransportFlow(i,false).get();
>      CSFLogDebug(logTag, "Transportflow[%d] = %p", i, flow.get());
>      if (flow) {
> +      if (!mDataConnection->ConnectDTLS(flow, aLocalport, aRemoteport)) {

InitializeDataChannel returns silently OK if there is no flow?

At minimum a comment is needed here to explain this.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +486,5 @@
>  #ifdef MOZ_PEERCONNECTION
> +void
> +DataChannelConnection::SetEvenOdd()
> +{
> +  TransportLayerDtls *dtls = static_cast<TransportLayerDtls *>(

Needs to be on the STS thread. Please add an assert.

@@ +498,2 @@
>  {
> +  LOG(("Connect DTLS local %u, remote %u", localport, remoteport));

I would consider renaming this something like
ConnectViaTransportFlow.

@@ +501,5 @@
>    NS_PRECONDITION(mMasterSocket, "SCTP wasn't initialized before ConnectDTLS!");
>    NS_ENSURE_TRUE(aFlow, false);
>  
>    mTransportFlow = aFlow;
>    mTransportFlow->SignalPacketReceived.connect(this, &DataChannelConnection::SctpDtlsInput);

This is happening on the main thread? Remember that the signals are
not thread-safed currently. Unless this is guaranteed to happen
before TFlow has done anything, you need to do this on the STS
thread.

@@ +694,1 @@
>    struct linger l;

Can we remove Listen? It doesn't seem to ever be referenced.

@@ +772,4 @@
>    }
>  #endif
>  
>    mSocket = mMasterSocket;

Can we remove Connect? It doesn't seem to ever be referenced.

@@ +1820,5 @@
>  
>    mLock.AssertCurrentThreadOwns();
>  
>    if (stream == INVALID_STREAM || mState != OPEN) {
> +    if (mState == OPEN && stream == INVALID_STREAM) {

Doesn't this second test reduce to if (mState == OPEN)?

I.e.,
if mState == OPEN, then the second half of the first
test is false, thus the first half of the first test
(mSTream == INVALID_STREAM) must be true.

@@ +1826,1 @@
>        stream = FindFreeStream(); // may be INVALID_STREAM!

Reading https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=855623&attachment=730740,
it appears that this can only happen if we are out of streams? Shouldn't something
more exciting happen than just deferring the operation?

@@ +1839,1 @@
>          if (!RequestMoreStreams()) {

Same logic comment as above.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #33)
> Comment on attachment 731613 [details] [diff] [review]
> Set even/odd streams via DTLS role (updated)
> 
> Review of attachment 731613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for threading issues. I also would like to go over OpenFinish in more
> detail, since the logic seems confusing.

I have an update to the logic that makes it more readable.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +672,5 @@
> >    nsresult rv = EnsureDataConnection(aNumstreams);
> >  
> >    // XXX Fix! Get the correct flow for DataChannel. Also error handling.
> >    for (int i = 2; i >= 0; i--) {
> >      nsRefPtr<TransportFlow> flow = mMedia->GetTransportFlow(i,false).get();
> 
> Not the subject of this patch but this seems wrong. The
> specification needs to clearly state which flow will
> be used. Even if it doesn't, having this loop suggests
> that the two sides might disagree, which clearly isn't
> going to work. I don't think that can actually happen,
> but then why do need a loop?

This was a hack we had been using until SDP support landed; it's removed in a patch that was supposed to land after Ethan's bug 837035 and hasn't (since that just landed); I'll get that in the hopper as well.  It will use the assigned flow.

> ::: netwerk/sctp/datachannel/DataChannel.cpp
> @@ +486,5 @@
> >  #ifdef MOZ_PEERCONNECTION
> > +void
> > +DataChannelConnection::SetEvenOdd()
> > +{
> > +  TransportLayerDtls *dtls = static_cast<TransportLayerDtls *>(
> 
> Needs to be on the STS thread. Please add an assert.

ok

> @@ +498,2 @@
> >  {
> > +  LOG(("Connect DTLS local %u, remote %u", localport, remoteport));
> 
> I would consider renaming this something like
> ConnectViaTransportFlow.

Sure.  Old name.

> @@ +501,5 @@
> >    NS_PRECONDITION(mMasterSocket, "SCTP wasn't initialized before ConnectDTLS!");
> >    NS_ENSURE_TRUE(aFlow, false);
> >  
> >    mTransportFlow = aFlow;
> >    mTransportFlow->SignalPacketReceived.connect(this, &DataChannelConnection::SctpDtlsInput);
> 
> This is happening on the main thread? Remember that the signals are
> not thread-safed currently. Unless this is guaranteed to happen
> before TFlow has done anything, you need to do this on the STS
> thread.

Per IRC discussion, I'll file a bug on this existing issue and make it depend on the signal thread-locking bug.

> @@ +694,1 @@
> >    struct linger l;
> 
> Can we remove Listen? It doesn't seem to ever be referenced.

I'll #ifdef them, perhaps remove them in a cleanup pass later (needed anyways) if we decide direct usage won't ever be done (likely).

> @@ +1826,1 @@
> >        stream = FindFreeStream(); // may be INVALID_STREAM!
> 
> Reading
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=855623&attachment=730740,
> it appears that this can only happen if we are out of streams? Shouldn't
> something
> more exciting happen than just deferring the operation?

Sure, the exciting thing is we RequestMoreStreams().  The updated logic will make it clearer.

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Note that bug 856434 is a patch from bug 837035 that was supposed to land with it and was already r+'d, that resolves the "which transport" issue raised above.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Attachment: interdiffs for response to ekr's review of the role code

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Set even/odd streams via DTLS role (updated)

Note this assumes we'll also be landing the patch from bug 837035 (now in bug 856434).  Verified with Tuexen that we'll always get COMM_UP.

Comment 38

[Eric Rescorla (:ekr)]

Comment on attachment 731684 [details] [diff] [review]
Set even/odd streams via DTLS role (updated)

Review of attachment 731684 [details] [diff] [review]:
-----------------------------------------------------------------

I believe that the ALLOC_DIRECT_SCTP_LISTEN_CONNECT
arm is still broken. You should either fix it or
remove it or add a "this is broken" or something.

I would also consider refactoring the conditional
but that's your call.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +644,5 @@
>    return res;
>  }
>  #endif
>  
> +#ifdef ALLOW_DIRECT_SCTP_LISTEN_CONNECT

#ifdef the declaraions in the class too.

@@ +690,5 @@
>                           (const void *)&l, (socklen_t)sizeof(struct linger)) < 0) {
>      LOG(("Couldn't set SO_LINGER on SCTP socket"));
>    }
>  
> +  SetEvenOdd();

This is going to cause an assert b/c you are on the main thread (and also perhaps you're not doing DTLS).

@@ +1812,5 @@
>    uint16_t stream = channel->mStream;
>  
>    mLock.AssertCurrentThreadOwns();
>  
>    if (stream == INVALID_STREAM || mState != OPEN) {

I would reconsider the logic here to have a boolean like "ready" that you frob instead
of all these conditionals. It's a taste issue, but I find that easier to read.

@@ +1819,1 @@
>        stream = FindFreeStream(); // may be INVALID_STREAM!

Can you add a comment to the effect that this happens when you don't have enough streams left.

@@ +1820,2 @@
>        if (stream == INVALID_STREAM) {
>          if (!RequestMoreStreams()) {

A log message on this error seems like a good idea.

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Interdiffs for review

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Set even/odd streams via DTLS role (updated)

Now thunks to STS when connecting, and keys off the DTLS/TransportFlow state.

Comment 41

[Eric Rescorla (:ekr)]

Comment on attachment 731699 [details] [diff] [review]
Set even/odd streams via DTLS role (updated)

Review of attachment 731699 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments below. please annotate the code to indicate that these variables are
lock protected.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +505,5 @@
>    NS_ENSURE_TRUE(aFlow, false);
>  
>    mTransportFlow = aFlow;
> +  // FIX - this needs sigslot to be in threadsafe mode, or to thunk this to STS thread
> +  // See bug 856439

This comment is now unnecessary, right?

@@ +525,5 @@
> +  LOG(("Setting transport signals, state: %d", mTransportFlow->state()));
> +  mTransportFlow->SignalPacketReceived.connect(this, &DataChannelConnection::SctpDtlsInput);
> +  // SignalStateChange() doesn't call you with the initial state
> +  CompleteConnect(mTransportFlow, mTransportFlow->state());
> +  mTransportFlow->SignalStateChange.connect(this, &DataChannelConnection::CompleteConnect);

I would suggest moving the Signal connect up in case something causes a failure and now you get a state change failure.

@@ +534,5 @@
> +{
> +  LOG(("Data transport state: %d", state));
> +  MutexAutoLock lock(mLock);
> +  ASSERT_WEBRTC(IsSTSThread());
> +  if (state != TransportLayer::TS_OPEN || !mMasterSocket)

What about TS_ERRO?

::: netwerk/sctp/datachannel/DataChannel.h
@@ +287,5 @@
>    nsAutoTArray<uint16_t,4> mStreamsResetting;
>  
> +  struct socket *mMasterSocket; // accessed from STS thread
> +  struct socket *mSocket; // cloned from mMasterSocket on successful Connect on STS thread
> +  uint16_t mState; // modified on STS thread (to OPEN)

Is mState protected by the lock? If so, please add comment. If not, you need an STS-thread only assert.

Generally, I am having trouble convincing myself that all of these variable are treated safely.

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Green try at https://tbpl.mozilla.org/?tree=Try&rev=1b91504c14a7
Verified mState is protected with mLock and commented.  Will have cdiehl run tsan against it after we land.

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8d239474ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1220831b5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77a971c874a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96c68ef05a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/abaf53f17e94
https://hg.mozilla.org/integration/mozilla-inbound/rev/39543b8a5556
https://hg.mozilla.org/integration/mozilla-inbound/rev/196f50e2f906

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff8d239474ec
https://hg.mozilla.org/mozilla-central/rev/cd1220831b5b
https://hg.mozilla.org/mozilla-central/rev/c77a971c874a
https://hg.mozilla.org/mozilla-central/rev/a96c68ef05a9
https://hg.mozilla.org/mozilla-central/rev/abaf53f17e94
https://hg.mozilla.org/mozilla-central/rev/39543b8a5556
https://hg.mozilla.org/mozilla-central/rev/196f50e2f906