Closed Bug 1426831 Opened 6 years ago Closed 6 years ago

Maximum message size reset to default when creating data channels at a later stage

Categories

(Core :: WebRTC: Signaling, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: lgrahl, Assigned: drno)

References

Details

Attachments

(1 file)

The first created (or all early ones, I'm not sure) data channel's maximum message size is updated from the signalling stack once the remote description has been set. But for data channels created at a later stage, the maximum message size is set to the default remote MMS and not updated.

This affects FF >= 57.

Can be tested here: https://lgrahl.de/examples/dc/dc-2nd-fail-ff57.html
I'm not sure I can follow what the demo page is suppose to show. I appear to get two data channels which both successfully send 65536 byte message, but refuse to send 65537. That seems perfectly normal to me given that 65536 got established as the MMS in offer and answer.

If the intention of the demo page is that the second data channel should only get established after the PeerConnection and the first data channel got established, then the code needs to get changed to wait for that to happen before creating a second data channel.

The console log shows the following for me:

pc0.mms open
dc-2nd-fail-ff57.html:89:9
pc0.mms outgoing message (65536 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:309:13
pc0.mms outgoing message (65537 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:318:17
Unable to send message: TypeError 
DOMException: The expression cannot be converted to return the specified type.
dc-2nd-fail-ff57.html:324:21
pc1.mms open
dc-2nd-fail-ff57.html:89:9
pc1.mms outgoing message (65536 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:309:13
pc1.mms outgoing message (65537 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:318:17
Unable to send message: TypeError 
DOMException: The expression cannot be converted to return the specified type.
dc-2nd-fail-ff57.html:324:21
pc1.mms incoming message (65536 bytes): 
message { target: DataChannel, isTrusted: true, data: Blob, origin: "https://lgrahl.de", lastEventId: "", ports: Restricted, eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, … }
dc-2nd-fail-ff57.html:102:9
pc0.mms incoming message (65536 bytes): 
message { target: DataChannel, isTrusted: true, data: Blob, origin: "https://lgrahl.de", lastEventId: "", ports: Restricted, eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, … }
dc-2nd-fail-ff57.html:102:9
pc0.failomat open
dc-2nd-fail-ff57.html:89:9
pc0.failomat outgoing message (65536 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:309:13
pc0.failomat outgoing message (65537 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:318:17
Unable to send message: TypeError 
DOMException: The expression cannot be converted to return the specified type.
dc-2nd-fail-ff57.html:324:21
pc1.failomat open
dc-2nd-fail-ff57.html:89:9
pc1.failomat outgoing message (65536 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:309:13
pc1.failomat outgoing message (65537 bytes): 
Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, … ]
dc-2nd-fail-ff57.html:318:17
Unable to send message: TypeError 
DOMException: The expression cannot be converted to return the specified type.
dc-2nd-fail-ff57.html:324:21
pc1.failomat incoming message (65536 bytes): 
message { target: DataChannel, isTrusted: true, data: Blob, origin: "https://lgrahl.de", lastEventId: "", ports: Restricted, eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, … }
dc-2nd-fail-ff57.html:102:9
pc0.failomat incoming message (65536 bytes): 
message { target: DataChannel, isTrusted: true, data: Blob, origin: "https://lgrahl.de", lastEventId: "", ports: Restricted, eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, … }
Ah, sorry, you could only see the effect in FF 57. I've updated the demo, so it works with FF >= 58, too. But here's a short description of what's happening in the demo script:

1. Patching the remote MMS to 2^18.
2. Sending a message of size 2^18 bytes via the initially created data channel - works.
3. Sending a message of size 2^18 bytes via a data channel that's created at a later stage - fails because for this channel the MMS is not updated to 2^18 but remains at 2^16 (which is the default remote MMS).

I thought about consequences for adapter (https://github.com/webrtc/adapter/pull/702)... and I actually believe the safest option would be to limit sending to 2^16 for FF > 57 and 2^16 - 1 for FF 57 until this is fixed. This would prevent unexpected errors potentially leading to a lot of confusion (because it only applies to some data channels). Please let me know what you think as this is quite a hefty constraint. However, I currently don't see an alternative.
Flags: needinfo?(drno)
I need to correct myself, this also applies to the data channels created earlier but only as soon as the subsequent data channel has been created.

So, I think this is what happens:
1. DataChannelConnection::SetMaxMessageSize is called with the 2^18 value when the remote description is being set.
2. When the second data channel is created at a later stage, DataChannelConnection::SetMaxMessageSize is called again and resets the connection to the default values (MMS=2^16, MMSSet=false).

You can verify that this is the case for the initially created data channel as well by applying the following code in the console when you run the demo:
pcs['pc0'].dcs['mms'].send(new Uint8Array(262144))
Summary: Maximum message size not applied to all data channels correctly → Maximum message size reset to default when creating data channels at a later stage
I believe this is the cause:
https://dxr.mozilla.org/mozilla-beta/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#1313

We need to make sure that DataChannelConnection::SetMaxMessageSize is only called iff the remote MMS changed due to an update by the remote description.
Yes now it makes sense to me. But it not only affects the max-message-size, but also the port numbers (although I'm not sure if they actually matter).
Assignee: nobody → drno
Rank: 15
Flags: needinfo?(drno)
Priority: -- → P2
Summary: Maximum message size reset to default when creating data channels at a later stage → Datachannel values reset to default when creating data channels at a later stage
(In reply to Lennart Grahl from comment #6)
> Doesn't affect the port:
> https://dxr.mozilla.org/mozilla-beta/source/media/webrtc/signaling/src/
> peerconnection/PeerConnectionImpl.cpp#1088

Right. 

But I think this actually raises the question: is changing any of these values allowed for renegotiations?
And if so: which values can be changed and what should implementations do about it?
Mh, have you looked at JSEP and the W3C spec? I mean, to be honest, changing just the MMS will be a super-rare corner case if anything, so this is more of an academic question.
(In reply to Lennart Grahl from comment #8)
> Mh, have you looked at JSEP and the W3C spec? I mean, to be honest, changing
> just the MMS will be a super-rare corner case if anything, so this is more
> of an academic question.

I meant this more on a higher level then just the MMS: what is an implementation suppose to do if a re-offer or answer contains different port numbers or MMS value then before? Ignore or update (if possible)?
Attachment #8941307 - Flags: review?(lennart.grahl)
(In reply to Nils Ohlmeier [:drno] from comment #9)
> (In reply to Lennart Grahl from comment #8)
> > Mh, have you looked at JSEP and the W3C spec? I mean, to be honest, changing
> > just the MMS will be a super-rare corner case if anything, so this is more
> > of an academic question.
> 
> I meant this more on a higher level then just the MMS: what is an
> implementation suppose to do if a re-offer or answer contains different port
> numbers or MMS value then before? Ignore or update (if possible)?

For 'sctp-port', it's complicated: https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-9.3

Regarding 'max-message-size', I don't really think it's clearly specified: https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-6.1
Common sense tells me to just apply this to any existing SCTP association for that peer connection. However, if you think this isn't really well-defined too, one of us should probably ask on the IETF mailing list.
(In reply to Lennart Grahl from comment #11)
> For 'sctp-port', it's complicated:
> https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-9.3

This read to me as if a different sctp port number means we would have to close the SCTP association and establish a new one. Which we clearly don't support right now. But I doubt anyone else does. And so it's probably not worth spending too much time worrying about that right now.
 
> Regarding 'max-message-size', I don't really think it's clearly specified:
> https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-6.1
> Common sense tells me to just apply this to any existing SCTP association
> for that peer connection. However, if you think this isn't really
> well-defined too, one of us should probably ask on the IETF mailing list.

Okay this read to me as if it is legit for an endpoint to change this value. And if I'm not mistaken the patch I attached here accomplishes exactly that: don't overwrite existing MMS values with default values, but any new MMS value from signaling will update the existing value.
Summary: Datachannel values reset to default when creating data channels at a later stage → Maximum message size reset to default when creating data channels at a later stage
See Also: → 1429611
(In reply to Nils Ohlmeier [:drno] from comment #12)
> (In reply to Lennart Grahl from comment #11)
> > For 'sctp-port', it's complicated:
> > https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-9.3
> 
> This read to me as if a different sctp port number means we would have to
> close the SCTP association and establish a new one. Which we clearly don't
> support right now. But I doubt anyone else does. And so it's probably not
> worth spending too much time worrying about that right now.

This is how I understand it as well. Like I said, this is probably something no one has a use case for anyway (at least not in WebRTC). But we should consider at least filing a bug for this.

> > Regarding 'max-message-size', I don't really think it's clearly specified:
> > https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-6.1
> > Common sense tells me to just apply this to any existing SCTP association
> > for that peer connection. However, if you think this isn't really
> > well-defined too, one of us should probably ask on the IETF mailing list.
> 
> Okay this read to me as if it is legit for an endpoint to change this value.
> And if I'm not mistaken the patch I attached here accomplishes exactly that:
> don't overwrite existing MMS values with default values, but any new MMS
> value from signaling will update the existing value.

Yes and I'll take a look at that asap.
Comment on attachment 8941307 [details]
Bug 1426831: don't overwrite MaxMessageSize with default values.

https://reviewboard.mozilla.org/r/211604/#review218846

::: netwerk/sctp/datachannel/DataChannel.cpp:436
(Diff revision 1)
>                              SCTP_STREAM_RESET_EVENT,
>                              SCTP_STREAM_CHANGE_EVENT};
>    {
>      ASSERT_WEBRTC(NS_IsMainThread());
>      // MutexAutoLock lock(mLock); Not needed since we're on mainthread always
>  

We can initialise `mMaxMessageSize = false` here iff it's crystal clear that ::Init is called before anything else.

::: netwerk/sctp/datachannel/DataChannel.cpp:604
(Diff revision 1)
>  void
>  DataChannelConnection::SetMaxMessageSize(bool aMaxMessageSizeSet, uint64_t aMaxMessageSize)
>  {
>    MutexAutoLock lock(mLock); // TODO: Needed?
>  
> +  if (mMaxMessageSizeSet && !aMaxMessageSizeSet) {

IIRC mMaxMessageSizeSet is not initialised at that point. But it would work if mMaxMessageSizeSet would be preinitialised with 'false'.
Attachment #8941307 - Flags: review?(lennart.grahl) → review-
Comment on attachment 8941307 [details]
Bug 1426831: don't overwrite MaxMessageSize with default values.

https://reviewboard.mozilla.org/r/211604/#review218846

> We can initialise `mMaxMessageSize = false` here iff it's crystal clear that ::Init is called before anything else.

I think you meant mMaxMessageSizeSet. And yes ::Init gets called through EnsureDataConnection(), which gets called no matter how you create a data channel.

> IIRC mMaxMessageSizeSet is not initialised at that point. But it would work if mMaxMessageSizeSet would be preinitialised with 'false'.

Good catch. Thanks. Fixed.
Comment on attachment 8941307 [details]
Bug 1426831: don't overwrite MaxMessageSize with default values.

https://reviewboard.mozilla.org/r/211604/#review220110
Attachment #8941307 - Flags: review?(lennart.grahl) → review+
Looks good. I'll need to remind myself to update adapter once this lands...
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/5058e3e4798e
don't overwrite MaxMessageSize with default values. r=lennart.grahl+594092
https://hg.mozilla.org/mozilla-central/rev/5058e3e4798e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: