Implement EOR mode in WebRTC DataChannels

RESOLVED FIXED in Firefox 57

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: jesup, Assigned: lennart.grahl)

Tracking

({dev-doc-complete})

Trunk
mozilla57
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(URL)

Attachments

(1 attachment, 16 obsolete attachments)

59 bytes, text/x-review-board-request
jesup
: review+
Details
This allows sending and receiving arbitrary-sized messages.  without the ndata extension large messages will cause monopolization of the SCTP output queue.
Posted file datachannel:5,sctp:5 logs (obsolete) —
works when layered on top of the SCTP update now.  Transferred 8MB file as a single SCTP message.
Attachment #8385445 - Attachment is obsolete: true
Attachment #8385470 - Attachment is obsolete: true
Comment on attachment 8385531 [details] [diff] [review]
Support arbitrary-sized send and receive in DataChannels (WIP)

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

Still work-in-progress patch; for a real patch I'd have it be adaptive and use PPID when talking to older clients, and EOR when talking to newer (or ndata) clients (which is part of why I'm not ripping out the PPID code for a while).  Marking for feedback.
Attachment #8385531 - Flags: feedback?(tuexen)

Comment 5

5 years ago
The patch is WIP, but it looks good so far.

Updated

5 years ago
Attachment #8385531 - Flags: feedback?(tuexen) → feedback+
backlog: --- → parking-lot
Rank: 25
Priority: -- → P2
(Assignee)

Comment 6

2 years ago
Only EOR on receiving has been implemented and tested with RAWRTC.

Aborting reassembly of unreliable partial messages still needs to be implemented (listen for SCTP_PARTIAL_DELIVERY_EVENT).
Attachment #8865960 - Flags: feedback?(rjesup)
(Assignee)

Comment 7

2 years ago
Add support for arbitrarily sized data channel messages (including DCEP) when sending.

Due to the use of explicit EOR, this included some major refactoring of all send-related and deferred sending functions (which is now a lot less complex). There's now only one place where `usrsctp_sendv` is being used.
All data channel messages and DCEP messages will be sent without copying them first. Only in case this fails (e.g. usrsctp's buffer is full), the message will be copied and added to a buffer queue.
Queued data channel messages are now re-sent fairly (round-robin).
Attachment #8866843 - Flags: feedback?(rjesup)
(Assignee)

Comment 8

2 years ago
Make the maximum message size and the PPID-based fragmentation configurable using about:config.

Fix PPID-based fragmentation sending.
Add max-message-size to the SCTP stack (can be overriden using `media.peerconnection.sctp.force_maximum_mesage_size`).
Add flag to the data channel connection whether the deprecated PPID-based fragmentation shall be used or not (can be overriden using `media.peerconnection.sctp.force_ppid_fragmentation`).
Attachment #8866963 - Flags: feedback?(rjesup)
See Also: → bug 1335262
(Assignee)

Comment 9

2 years ago
Detect older Firefox versions by looking at the negotiated amount of incoming streams to turn on PPID-based fragmentation.
Enable interleaving of incoming messages for different streams.
Prevent outgoing message interleaving on different streams in case SCTP ndata has not been negotiated.
Attachment #8875945 - Flags: feedback?(rjesup)
(Assignee)

Comment 10

2 years ago
There's only one major issue left (listening for the SCTP_PARTIAL_DELIVERY_EVENT to abort receiving a message on non-reliable channels). Plus a little code cleanup and we're good to go.
(Assignee)

Comment 11

2 years ago
Handle partial delivery events (for cases where a partially delivered message is being aborted)
Attachment #8878062 - Flags: feedback?(rjesup)
(Assignee)

Comment 12

2 years ago
Some code cleanup is left.

We currently raise a nasty looking error message in case the send method is being called and len(message) > max-message-size. This still needs to be addressed. Related WebRTC spec issue: https://github.com/w3c/webrtc-pc/issues/1205
Depends on: 1373450
Depends on: 1374440
(Assignee)

Comment 13

2 years ago
Data channel code cleanup.
Throw TypeError in case a message is too large for the other peer to receive.
Attachment #8879342 - Flags: feedback?(rjesup)
(Assignee)

Comment 14

2 years ago
Posted patch Support EOR send/receive (obsolete) — Splinter Review
Done! Feedback welcome. I think it would be a good idea if Michael Tüxen would have a look as well.
Attachment #8865960 - Attachment is obsolete: true
Attachment #8866843 - Attachment is obsolete: true
Attachment #8866963 - Attachment is obsolete: true
Attachment #8875945 - Attachment is obsolete: true
Attachment #8878062 - Attachment is obsolete: true
Attachment #8879342 - Attachment is obsolete: true
Attachment #8865960 - Flags: feedback?(rjesup)
Attachment #8866843 - Flags: feedback?(rjesup)
Attachment #8866963 - Flags: feedback?(rjesup)
Attachment #8875945 - Flags: feedback?(rjesup)
Attachment #8878062 - Flags: feedback?(rjesup)
Attachment #8879342 - Flags: feedback?(rjesup)
Attachment #8880146 - Flags: feedback?(tuexen)
Attachment #8880146 - Flags: feedback?(rjesup)
(Assignee)

Updated

2 years ago
Attachment #8880146 - Flags: feedback?(rjesup) → review?(rjesup)
Comment on attachment 8880146 [details] [diff] [review]
Support EOR send/receive

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

Lennart you should also change this http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp#2374 as part of this patch here to re-enable sending the a=max-message-size.
Lennart - the patch as uploaded makes the diff/review viewer ('splinter') get very, very confused.  Perhaps partly because there are N separate diffs for the same small sets of files in the patch file.... 

I suspect this is due to a bad export from git.

I'll try applying the patch file with "patch -p1 <file" and then hg diff to see what changes it really made.  If you meant this to be a series of independent changes (and it likely was), you need to export them as N patches and add to the bug (and please number them to make the order-of-application obvious).   Thanks.
Assignee: rjesup → lennart.grahl
Flags: needinfo?(lennart.grahl)
The patch does not come close to applying cleanly, so we'll need a real appy-able patch.   Thanks!
> appy-able patch.   Thanks!

apply-able
Posted patch eor-support-central.patch (obsolete) — Splinter Review
This patch is between Lenart's code and mozilla-central
Attachment #8882424 - Flags: review?(rjesup)
(In reply to Nils Ohlmeier [:drno] from comment #19)
> Created attachment 8882424 [details] [diff] [review]

Hmm this does not compile right now:

5:06.64 In file included from /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.cpp:57:
 5:06.64 /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.h:507:3: error: call to deleted constructor of 'mozilla::Runnable'
 5:06.64   DataChannelOnMessageAvailable(int32_t     aType,
 5:06.64   ^
 5:06.64 /Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/dist/include/nsThreadUtils.h:427:3: note: 'Runnable' has been explicitly marked deleted here
 5:06.64   Runnable() = delete;
 5:06.64   ^
 5:06.64 In file included from /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.cpp:57:
 5:06.64 /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.h:516:3: error: call to deleted constructor of 'mozilla::Runnable'
 5:06.64   DataChannelOnMessageAvailable(int32_t     aType,
 5:06.64   ^
 5:06.64 /Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/dist/include/nsThreadUtils.h:427:3: note: 'Runnable' has been explicitly marked deleted here
 5:06.64   Runnable() = delete;
 5:06.64   ^
 5:06.64 In file included from /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.cpp:57:
 5:06.64 /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.h:524:3: error: call to deleted constructor of 'mozilla::Runnable'
 5:06.64   DataChannelOnMessageAvailable(int32_t     aType,
 5:06.64   ^
 5:06.65 /Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/dist/include/nsThreadUtils.h:427:3: note: 'Runnable' has been explicitly marked deleted here
 5:06.65   Runnable() = delete;
 5:06.65   ^
 5:06.65 In file included from /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.cpp:57:
 5:06.65 /Users/nohlmeier/src/mozilla-central/netwerk/sctp/datachannel/DataChannel.h:532:3: error: call to deleted constructor of 'mozilla::Runnable'
 5:06.65   DataChannelOnMessageAvailable(int32_t     aType,
 5:06.65   ^
Comment on attachment 8882424 [details] [diff] [review]
eor-support-central.patch

Does this look the changes you intended to do?
Attachment #8882424 - Flags: feedback?(lennart.grahl)
(Assignee)

Comment 22

2 years ago
I really have no clue what went wrong here... I think it's best if I pull and rebase against central, build it and then re-upload a patch you guys can review.
Flags: needinfo?(lennart.grahl)
(Assignee)

Comment 23

2 years ago
Comment on attachment 8882424 [details] [diff] [review]
eor-support-central.patch

In a long and painful process, I've rebased the patches against current central. However, this time I'll push it to review as this seems more reasonable for such a large patch-set.
Attachment #8882424 - Flags: feedback?(lennart.grahl) → feedback-
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8883123 - Flags: feedback?(tuexen)
(Assignee)

Updated

2 years ago
Attachment #8385531 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8880146 - Attachment is obsolete: true
Attachment #8880146 - Flags: review?(rjesup)
Attachment #8880146 - Flags: feedback?(tuexen)
(Assignee)

Updated

2 years ago
Attachment #8882424 - Attachment is obsolete: true
Attachment #8882424 - Flags: review?(rjesup)
Attachment #8882424 - Flags: feedback-
(Assignee)

Comment 25

2 years ago
We currently face the problem that DataChannelConnection::Init is called "too soon", so the remote maximum message size is always 65535 and remote maximum message set is always true, see https://dxr.mozilla.org/comm-central/source/mozilla/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#1321
It seems that https://dxr.mozilla.org/comm-central/source/mozilla/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#1265 is never executed, not for the offerer, not for the answerer. I've also tried using only a pre-negotiated channel and only a DCEP-negotiated channel which made no difference.

Looks like we need a setter for remote maximum message and remote maximum message. Furthermore, the above one could even be dead code.
Flags: needinfo?(drno)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

2 years ago
mozreview-review
Comment on attachment 8883732 [details]
Fix typo, don't hard-code SCTP port.

https://reviewboard.mozilla.org/r/154638/#review159742
Attachment #8883732 - Flags: review+
(Reporter)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8883406 [details]
Fix implicit conversion constructor.

https://reviewboard.mozilla.org/r/154304/#review161324
Attachment #8883406 - Flags: review?(rjesup) → review+
(Reporter)

Comment 30

2 years ago
mozreview-review
Comment on attachment 8883732 [details]
Fix typo, don't hard-code SCTP port.

https://reviewboard.mozilla.org/r/154638/#review161326

I presume this is undoing a change in patch 1.  When landing these patches, fold all three into one patch before requesting checkin-needed.
Attachment #8883732 - Flags: review?(rjesup) → review+
(Reporter)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review161328

Mostly nits. One or two minor issues.  Thanks!  one revision and we should be good to land

::: netwerk/sctp/datachannel/DataChannel.h:92
(Diff revision 1)
>  // for queuing incoming data messages before the Open or
>  // external negotiation is indicated to us
>  class QueuedDataMessage
>  {
>  public:
> -  QueuedDataMessage(uint16_t stream, uint32_t ppid,
> +  QueuedDataMessage(uint16_t stream, uint32_t ppid, int32_t flags,

odd that flags would be signed, but maybe that's needed by sctp?

::: netwerk/sctp/datachannel/DataChannel.h:110
(Diff revision 1)
>      free(mData);
>    }
>  
>    uint16_t mStream;
>    uint32_t mPpid;
> -  size_t   mLength;
> +  int32_t  mFlags;

ditto

::: netwerk/sctp/datachannel/DataChannel.h:196
(Diff revision 1)
> -  int32_t SendMsg(uint16_t stream, const nsACString &aMsg)
> +  // Returns a POSIX error code.
> +  int SendMsg(uint16_t stream, const nsACString &aMsg)
>      {
> -      return SendMsgCommon(stream, aMsg, false);
> +      return SendDataMsgCommon(stream, aMsg, false);
>      }
> -  int32_t SendBinaryMsg(uint16_t stream, const nsACString &aMsg)
> +    

delete trailing spaces before landing

::: netwerk/sctp/datachannel/DataChannel.h:202
(Diff revision 1)
> +  // Returns a POSIX error code.
> +  int SendBinaryMsg(uint16_t stream, const nsACString &aMsg)
>      {
> -      return SendMsgCommon(stream, aMsg, true);
> +      return SendDataMsgCommon(stream, aMsg, true);
>      }
> -  int32_t SendBlob(uint16_t stream, nsIInputStream *aBlob);
> +  

ditto

::: netwerk/sctp/datachannel/DataChannel.h:278
(Diff revision 1)
> +  void HandleDataMessage(const void *buffer, size_t dLength, uint32_t ppid, uint16_t stream,
> +                         int32_t flags);
> +  void HandleDCEPMessage(const void *buffer, size_t dLength, uint32_t ppid, uint16_t stream,
> +                         int32_t flags);

Probably "length" here unless we want to change the entire file to use aFoo for arguments.  dLength will  just confuse people

::: netwerk/sctp/datachannel/DataChannel.h:429
(Diff revision 1)
>        return 0;
>      }
>  
>      MutexAutoLock lock(mConnection->mLock);
> -    return GetBufferedAmountLocked();
> +    size_t buffered = GetBufferedAmountLocked();
> +    

trailing spaces

::: netwerk/sctp/datachannel/DataChannel.h:435
(Diff revision 1)
> +#if (SIZE_MAX > UINT32_MAX)
> +    if (buffered > UINT32_MAX) { // paranoia - >4GB buffered is very very unlikely
> +      buffered = UINT32_MAX;
> +    }
> +#endif
> +  

ditto

::: netwerk/sctp/datachannel/DataChannel.h:516
(Diff revision 1)
>  
>    DataChannelOnMessageAvailable(
>      int32_t aType,
>      DataChannelConnection* aConnection,
>      DataChannel* aChannel,
> -    nsCString& aData, // XXX this causes inefficiency
> +    nsCString& aData) // XXX this causes inefficiency)

remove added ')' at end of comment

::: netwerk/sctp/datachannel/DataChannel.cpp:371
(Diff revision 1)
> +            "media.peerconnection.sctp.force_ppid_fragmentation", &mPpidFragmentation))) {
> +          // Ensure that forced on/off PPID fragmentation does not get overridden when Firefox has
> +          // been detected.
> +          mMaxMessageSizeSet = true;
> +        }
> +        

spaces

::: netwerk/sctp/datachannel/DataChannel.cpp:406
(Diff revision 1)
> -      // ECN is currently not supported by the Firefox code
> +      
> +      // Disable the Explicit Congestion Notification extension (currently not supported by the
> +      // Firefox code)
>        usrsctp_sysctl_set_sctp_ecn_enable(0);
> +      
> +      // Enable interleaving messages for different streams (incoming)
> +      // See: https://tools.ietf.org/html/rfc6458#section-8.1.20
> +      usrsctp_sysctl_set_sctp_default_frag_interleave(2);
> +      

trailing spaces

::: netwerk/sctp/datachannel/DataChannel.cpp:480
(Diff revision 1)
> -      LOG(("*** failed encaps errno %d", errno));
>        goto error_cleanup;
>      }
> -    LOG(("SCTP encapsulation local port %d", aPort));
>    }
> +  

spaces - there are more, just get rid of all of them (M-x delete-trailing-whitespace in Emacs)

::: netwerk/sctp/datachannel/DataChannel.cpp:1152
(Diff revision 1)
> -      uint32_t buffered_amount = channel->GetBufferedAmountLocked();
> -      uint32_t threshold = channel->GetBufferedAmountLowThreshold();
> -      bool was_over_threshold = buffered_amount >= threshold;
> +    size_t bufferedAmount = channel->GetBufferedAmountLocked();
> +    size_t threshold = channel->mBufferedThreshold;
> +    bool wasOverThreshold = bufferedAmount >= threshold;

We've been generally using foo_bar for locals in this file, but fooBar is also obviously a local given our style.  I'm ok with fooBar, or we can maintain the current foo_bar.

::: netwerk/sctp/datachannel/DataChannel.cpp:1413
(Diff revision 1)
> +    nsAutoCString recvData(buffer, length); // copies (<64) or allocates
> +    recvBuffer += recvData;
> +    bufferFlags |= DATA_CHANNEL_BUFFER_MESSAGE_FLAGS_BUFFERED;

Don't bother with nsAutoCString here I think - rarely will a data message come in that needs to be buffered and is less than 64 bytes.  Also: there should be a simpler way to to add 'buffer' to recvBuffer without making an allocation in the middle.

I think recvBuffer.Append(buffer, length) *should* work correctly.  See nsTSubstring.h/cpp and nsCharTraits.h in xpcom/string (though it's less than obvious how all this plays out, but I'm pretty sure for a CString it ends up just calling copy() into (possibly newly allocated) space at the end of the buffer.

::: netwerk/sctp/datachannel/DataChannel.cpp:1421
(Diff revision 1)
> +  }
> +  return bufferFlags;
> +}
> +
>  void
> -DataChannelConnection::HandleDataMessage(uint32_t ppid,
> +DataChannelConnection::HandleDataMessage(const void *data, size_t dLength, uint32_t ppid,

see previous comment about dLength naming

::: netwerk/sctp/datachannel/DataChannel.cpp:1561
(Diff revision 1)
>    }
>  }
>  
> -// Called with mLock locked!
>  void
> -DataChannelConnection::HandleMessage(const void *buffer, size_t length, uint32_t ppid, uint16_t stream)
> +DataChannelConnection::HandleDCEPMessage(const void *buffer, size_t dLength, uint32_t ppid, 

dLength

::: netwerk/sctp/datachannel/DataChannel.cpp:1658
(Diff revision 1)
> +      // Check for older Firefox by looking at the amount of incoming streams
> +      LOG(("Negotiated number of incoming streams: %" PRIu16, sac->sac_inbound_streams));

Add a comment XXX Bug NNNNNN remove PPID fragmentation support once older firefoxes without EOR mode are no longer supported as target clients.  (and file the NNNNNN bug first so you can include the number in this patch)

::: netwerk/sctp/datachannel/DataChannel.cpp:1838
(Diff revision 1)
> +  // Note: Be aware that stream and sn being u32 instead of u16 is a bug in the SCTP API
> +  //       This may change in the future.

wording... perhaps "stream being a u32 instead of u16"?

::: netwerk/sctp/datachannel/DataChannel.cpp:1850
(Diff revision 1)
> +  LOG(("(flags = %x) ", spde->pdapi_flags));
> +  LOG(("stream = %" PRIu32 " ", spde->pdapi_stream));
> +  LOG(("sn = %" PRIu32 " ", spde->pdapi_seq));
> +  LOG(("\n"));

No \n's are needed in log messages.
Make this one log, or remove all of them (left-over debugging?)

::: netwerk/sctp/datachannel/DataChannel.cpp:2578
(Diff revision 1)
> +// Returns a POSIX error code.
> +int
> +DataChannelConnection::SendDataMsgInternalOrBuffer(DataChannel &channel, const uint8_t *data,
> +                                                   size_t len, uint32_t ppid)
> +{
> +  NS_ENSURE_TRUE(channel.mState == OPEN || channel.mState == CONNECTING, 0);

I'd probably prefer explicit code to NS_ENSURE_TRUE(), which hides that it can return.

::: netwerk/sctp/datachannel/DataChannel.cpp:2838
(Diff revision 1)
> -  DataChannel *channel;
> +#if (UINT32_MAX > SIZE_MAX)
> +  if (len > SIZE_MAX) {
> +    return EMSGSIZE;
> +  }
> +#endif
> +  DataChannel *channelp;

channel, or channel_ptr.  I'd prefer channel, but either will do

::: netwerk/sctp/datachannel/DataChannelProtocol.h:23
(Diff revision 1)
>  
> -#define WEBRTC_DATACHANNEL_STREAMS_DEFAULT          256
> -#define WEBRTC_DATACHANNEL_PORT_DEFAULT             5000
> -#define WEBRTC_DATACHANELL_MAX_MESSAGE_SIZE_DEFAULT 65536
> -
> -#define DATA_CHANNEL_PPID_CONTROL        50
> +#define WEBRTC_DATACHANNEL_STREAMS_DEFAULT                 256
> +// Do not change this value!
> +#define WEBRTC_DATACHANNEL_STREAMS_OLDER_FIREFOX           256
> +#define WEBRTC_DATACHANNEL_PORT_DEFAULT                    5000
> +// TODO: Change once we resolve the nsCString limitation

file a bug and note the number here
Attachment #8883123 - Flags: review?(rjesup) → review-
Posted patch bug979417_setmms.patch (obsolete) — Splinter Review
Lenart this here is a little patch which hopefully is doing what you are asking for.
For me the datachannel always got initialized with default values on the side where createDataChannel() got called (as we obviously did not have any signaling data from the other side available yet).

If this works for you you should add it as another commit to your patch set. I only wanted to avoid confusing mozreview by doing a commit from another person.
Flags: needinfo?(drno)
(Reporter)

Comment 33

2 years ago
mozreview-review-reply
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review161328

> wording... perhaps "stream being a u32 instead of u16"?

Ah, I thought 'sn' was a typo.  perhaps just add quotes around sn

> I'd probably prefer explicit code to NS_ENSURE_TRUE(), which hides that it can return.

I.e. instead of NS_ENSURE_TRUE, use something like (nsDebug.h)
 * if (NS_WARN_IF(NS_FAILED(rv)) {
 *   return rv;
 * }
or in this case:
 if (NS_WARN_IF(channel.mState == OPEN || channel.mState == CONNECTING)) {
   return 0;
 }
(Reporter)

Comment 34

2 years ago
mozreview-review-reply
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review161328

> Probably "length" here unless we want to change the entire file to use aFoo for arguments.  dLength will  just confuse people

Since you are using length in here somewhere as a local, I'd change the local to foo_length or foo_len or some such, and make the parameter length.  Or change the parameter to data_length (instead of dLength), since that's consistent with the style here.  Either.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 37

2 years ago
mozreview-review
Comment on attachment 8888532 [details]
Add SetMaxMessageSize and GetMaxMessageSize methods.

https://reviewboard.mozilla.org/r/159512/#review164992

::: netwerk/sctp/datachannel/DataChannel.h:155
(Diff revision 1)
>    // Finish Destroy on STS to avoid SCTP race condition with ABORT from far end
>    void DestroyOnSTS(struct socket *aMasterSocket,
>                      struct socket *aSocket);
>  
> +  void SetMaxMessageSize(bool aMaxMessageSizeSet, uint64_t aMaxMessageSize);
> +  uint64_t GetMaxMessageSize();

Probably should be const
Attachment #8888532 - Flags: review?(rjesup) → review+
(Reporter)

Comment 38

2 years ago
mozreview-review
Comment on attachment 8888531 [details]
Polishing.

https://reviewboard.mozilla.org/r/159510/#review164994

r+. 
Fold the patch in with the previous patch for landing; we don't want patch-development-history to become part of the master repo history.  
I.e. don't just set "checkin-needed"

Thanks!
Attachment #8888531 - Flags: review?(rjesup) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8883406 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8883732 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8888531 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8888532 - Attachment is obsolete: true
Attachment #8886586 - Attachment is obsolete: true
(Reporter)

Comment 40

2 years ago
mozreview-review
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review165122

r+ and also ping drno for review
Attachment #8883123 - Flags: review?(rjesup) → review+
Attachment #8883123 - Flags: review?(drno)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review165270

Looks good.
Just some minor questions/suggestions.

::: netwerk/sctp/datachannel/DataChannel.cpp:156
(Diff revision 2)
>    mData = tmp;
> +  mInfo = new sctp_sendv_spa;

Can we be sure that mData and mInfo are not containing data already?

To be safe I would prefer if we add some if() checks with delete to avoid mem leaks in case this gets called multiple times in the future for some reason.

::: netwerk/sctp/datachannel/DataChannel.cpp:545
(Diff revision 2)
> +
> +      int32_t temp;
> +      if (!NS_FAILED(branch->GetIntPref(
> +          "media.peerconnection.sctp.force_maximum_message_size", &temp))) {
> +        if (temp >= 0) {
> +          mMaxMessageSize = (uint64_t)temp;

Just casting this could lead to some unexpected behavior if negative values suddenly turn into positive here. It's probably not a big deal as this is only code for testing.

::: netwerk/sctp/datachannel/DataChannel.cpp:1062
(Diff revision 2)
> +  // Set pending type and stream index (if buffered)
> +  if (!error && buffered && !mPendingType) {
> +    mPendingType = PENDING_DCEP;
>    }

The comment mentions a stream index which does not appear in the code. Update the comment?

::: netwerk/sctp/datachannel/DataChannel.cpp:1431
(Diff revision 2)
> +  // Return directly if nothing has been buffered
> +  if ((bufferFlags & DATA_CHANNEL_BUFFER_MESSAGE_FLAGS_COMPLETE) && recvBuffer.IsEmpty()) {
> +    return bufferFlags;
> +  }

Can't you move this into the if block above with a simple 'if recvBuffer.IsEmpty()' check inside the existing if block?

::: netwerk/sctp/datachannel/DataChannel.cpp:2494
(Diff revision 2)
> -
> -  spa.sendv_sndinfo.snd_ppid = htonl(ppid);
> -  spa.sendv_sndinfo.snd_sid = channel->mStream;
> -  spa.sendv_sndinfo.snd_flags = flags;
> -  spa.sendv_sndinfo.snd_context = 0;
> -  spa.sendv_sndinfo.snd_assoc_id = 0;
> +      // Unset EOR flag
> +      info.snd_flags &= ~SCTP_EOR;
> +    } else {
> +      length = left;
> +
> +      // Reset EOR flag

Shouldn't the comment be "Set EOR flag"?
Attachment #8883123 - Flags: review?(drno) → review+
(Assignee)

Comment 42

2 years ago
mozreview-review-reply
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review165270

D'oh! Sorry for the delay on replying, I forgot to publish!

> Can we be sure that mData and mInfo are not containing data already?
> 
> To be safe I would prefer if we add some if() checks with delete to avoid mem leaks in case this gets called multiple times in the future for some reason.

I can't quite follow you. How can mData and mInfo contain previous data if the class instance is just being created?

> Just casting this could lead to some unexpected behavior if negative values suddenly turn into positive here. It's probably not a big deal as this is only code for testing.

Doesn't the conditional above the assignment test for exactly that or am I missing something?

Edit: Or did you mean that negative numbers are being ignored? Well, I don't really know what else to do in this case. Could log a warning but it should be fairly obvious that the maximum message size cannot be negative.

> Shouldn't the comment be "Set EOR flag"?

The flag is being *unset* in case the message cannot possibly fit into usrsctp's send buffer and is being *re-set* when it might fit. I believe the word fits in this context but I'm open for suggestions.
(Assignee)

Comment 43

2 years ago
mozreview-review-reply
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review161328

Forgot to publish...

> odd that flags would be signed, but maybe that's needed by sctp?

Yep, comes from the SCTP API when receiving: https://tools.ietf.org/html/rfc6458#section-9.13
But thanks for mentioning it as the signature should be int instead of int32_t. Will fix.

> We've been generally using foo_bar for locals in this file, but fooBar is also obviously a local given our style.  I'm ok with fooBar, or we can maintain the current foo_bar.

Seemed to me the code base is mostly camel case for local vars, so I went with that.

> I.e. instead of NS_ENSURE_TRUE, use something like (nsDebug.h)
>  * if (NS_WARN_IF(NS_FAILED(rv)) {
>  *   return rv;
>  * }
> or in this case:
>  if (NS_WARN_IF(channel.mState == OPEN || channel.mState == CONNECTING)) {
>    return 0;
>  }

Alright, I'll replace all the other `NS_ENSURE_*`'s as well (which is why I used it in the first place).

> channel, or channel_ptr.  I'd prefer channel, but either will do

Was to prevent yet another name clash but I renamed it to `channelPtr`.

Comment 44

2 years ago
mozreview-review-reply
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review165270

> I can't quite follow you. How can mData and mInfo contain previous data if the class instance is just being created?

Never mind. You are right.

> Doesn't the conditional above the assignment test for exactly that or am I missing something?
> 
> Edit: Or did you mean that negative numbers are being ignored? Well, I don't really know what else to do in this case. Could log a warning but it should be fairly obvious that the maximum message size cannot be negative.

Yes I meant that 'temp' is potentially a negative number. But I overlooked the if() check. So it's all good.

> The flag is being *unset* in case the message cannot possibly fit into usrsctp's send buffer and is being *re-set* when it might fit. I believe the word fits in this context but I'm open for suggestions.

To me "reset" translates into "setting it back to the default value (what ever that is/was)". As you used "unset" in the non-fitting case, to me it would make sense to just use "set" here. But maybe that is just me with a strange interpretation of words here.
(Assignee)

Comment 45

2 years ago
mozreview-review-reply
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review165270

> To me "reset" translates into "setting it back to the default value (what ever that is/was)". As you used "unset" in the non-fitting case, to me it would make sense to just use "set" here. But maybe that is just me with a strange interpretation of words here.

Simply *set* is fine by me as well. :)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 47

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9f0e7ba58159 -d 0d9cf59817bb: rebasing 412853:9f0e7ba58159 "Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP). r=drno,jesup" (tip)
merging media/webrtc/signaling/src/jsep/JsepCodecDescription.h
merging media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
merging media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
merging netwerk/sctp/datachannel/DataChannel.cpp
warning: conflicts while merging netwerk/sctp/datachannel/DataChannel.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
(Reporter)

Comment 49

2 years ago
mozreview-review
Comment on attachment 8883123 [details]
Bug 979417 - Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP).

https://reviewboard.mozilla.org/r/154070/#review172706

Comment 50

2 years ago
Pushed by rjesup@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8732792de20d
Implement EOR when receiving and explicit EOR when sending on data channels (including DCEP). r=drno,jesup
Keywords: checkin-needed

Comment 51

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8732792de20d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Keywords: dev-doc-needed
I've updated documentation:

https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/send now mentions that browsers may have different limitations, and links to a new documentation section (see below) that discusses this issue. I've also added a description of TypeError.

The overall discussion of message size limitations is here:

https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Using_data_channels#Understanding_message_size_limits

That entire section is new.

This is also mentioned on Firefox 57 for developers.

I'd appreciate it if someone could review the text mentioned above, since this is a highly technical change and I want to be sure that my summarization is accurate.
Keywords: dev-doc-needed → dev-doc-complete
Actually, let me needinfo Lennart in hopes that he can take a look.
Flags: needinfo?(lennart.grahl)
(Assignee)

Comment 54

2 years ago
I'll gladly take a look at it as soon as possible but can't give any guarantees before the end of this month (exam incoming). Hope that's ok for you. Should I post feedback here?
Flags: needinfo?(lennart.grahl)
(Assignee)

Comment 55

a year ago
Sorry, haven't forgotten about this. Maybe I can take a look at it tomorrow. But please let me know where I should place feedback.
Flags: needinfo?(eshepherd)
Wow, I missed the ni on this bug. My apologies! Just comment on the bug here with your feedback, and I'll see it. Thanks!
Flags: needinfo?(eshepherd)
You need to log in before you can comment on or make changes to this bug.