859179 - onMessage event on a data channel will fail to fire in which the message being sent is a very long string

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

STR:

1. Load the attached test case
2. Select "Setup Handshake"
3. Select "Local DC Send String Message"

Expected:

Even with long valid, string messages, the messages should go through when the channels are deemed reliable. So I would expect to get an onMessage event eventually.

Actual:

After waiting a few minutes, I have yet to see the onMessage event fire. Seems like maybe the message failed to send maybe? Although the question I think here is if it did fail to send, why didn't we throw an exception?

Comment 1

[Jason Smith [:jsmith]]

I recently tested a similar case that involved a 1 GB blob object and this worked fine. So why isn't this working with a very long string that takes up a lot of bytes?

Comment 2

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

Large binary messages are explicitly fragmented currently; strings aren't.  Also, strings may be more expensive to convert.  I'll look into it.

Comment 3

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

Right now, there's an application-defined limit of 256K (bytes) for strings (but no limit for binary messages).  I'll look to relax that in FF23 via a protocol change.

Comment 4

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

Removing uplift tag for now; will reconsider once we've landed in m-c

Comment 5

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

Also, not blocking for release since a 256K limit on strings for initial release is probably ok (much as WebSockets initially had a 16MB limit onbinary  transfers).

Comment 6

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #5)
> Also, not blocking for release since a 256K limit on strings for initial
> release is probably ok (much as WebSockets initially had a 16MB limit
> onbinary  transfers).

Yeah, that's fine. Probably a good case to dev doc here for Fx22.

Comment 7

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

Attachment: Support large strings by fragmentation

Comment 8

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

Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

Note: PPID 51 used to be DATA_CHANNEL_PPID_DOMSTRING, now it's DATA_CHANNEL_PPID_DOMSTRING_LAST (which does what DOMSTRING used to).  This keeps the naming consistent, and the binary protocol forward-compatible (send from 22 to 23 would be max 256K bytes as is 22 generally), and backward-compatible for strings less than 16K bytes (when sending from 23 to 22).

Note this issue only affects sending large DOMStrings; binary messages already work this way.

It may be worthwhile due to the back-compat issue to uplift this to Aurora, though not strictly required; if we don't we should relnote that strings >16K will not work when sending from FF23 to FF22 (and consider uplifting a patch to 22 to throw some type of warning, though that might be as much code as this patch).

Also note this assumes the fragmentation size remains at 16K; changing that would change the size at which back-compatibility comes into play.

r? to tuexen
f? to mcmanus on the uplift issue

(Michael: I'll incorporate this into the protocol document)

Comment 9

[Michael Tüxen]

Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

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

Looks good to me.

Comment 10

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

Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

r+ = tuexen by email

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b023bbaffecf

Comment 12

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

Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

Chatted with mcmanus on IRC; asking for uplift now to minimize incompatibility period (though the incompatibility isn't major and will only affect a few (if any) current apps.)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: DataChannels between FF22 and FF23 will be limited (in the 23->22 direction) to 16K maximum for DOMString Send()'s

Testing completed (on m-c, etc.): on inbound; locally tested using testcase.  We'll land a test to back this up.

Risk to taking this patch (and alternatives if risky): very low; this patch leverages the existing fragmentation code for binary messages.

String or IDL/UUID changes made by this patch: none

Comment 13

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

Note: I wouldn't land this on Aurora until it's in m-c and I've tested it there.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b023bbaffecf

Comment 15

[bhavana bajaj [:bajaj]]

[Relman comment]
Holding off on aurora approval till, status-firefox23 is marked verified.

Comment 16

[Jason Smith [:jsmith]]

lgtm on trunk. I'm seeing my original test case working - onMessage fires and buffering is happening to handle long strings.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

approving now that this is verified on trunk.

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/02ea518aec8b