Closed Bug 859179 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- disabled
firefox22 --- fixed
firefox23 --- fixed

People

(Reporter: jsmith, Assigned: jesup)

Details

(Whiteboard: [webrtc][blocking-webrtc-][spec-issue])

Attachments

(2 files)

Attached file 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?
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?
Large binary messages are explicitly fragmented currently; strings aren't.  Also, strings may be more expensive to convert.  I'll look into it.
Whiteboard: [webrtc][blocking-webrtc+]
Assignee: nobody → rjesup
Priority: -- → P2
Flags: in-testsuite?
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][webrtc-uplift]
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.
Removing uplift tag for now; will reconsider once we've landed in m-c
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+]
Target Milestone: --- → mozilla23
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).
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc-][spec-issue]
(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.
Keywords: dev-doc-needed
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)
Attachment #735186 - Flags: review?(tuexen)
Attachment #735186 - Flags: feedback?(mcmanus)
Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

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

Looks good to me.
Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

r+ = tuexen by email
Attachment #735186 - Flags: review?(tuexen) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b023bbaffecf
Whiteboard: [webrtc][blocking-webrtc-][spec-issue] → [webrtc][blocking-webrtc-][spec-issue][webrtc-uplift]
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
Attachment #735186 - Flags: feedback?(mcmanus) → approval-mozilla-aurora?
Note: I wouldn't land this on Aurora until it's in m-c and I've tested it there.
https://hg.mozilla.org/mozilla-central/rev/b023bbaffecf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Keywords: dev-doc-neededverifyme
[Relman comment]
Holding off on aurora approval till, status-firefox23 is marked verified.
lgtm on trunk. I'm seeing my original test case working - onMessage fires and buffering is happening to handle long strings.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment on attachment 735186 [details] [diff] [review]
Support large strings by fragmentation

approving now that this is verified on trunk.
Attachment #735186 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/02ea518aec8b
Whiteboard: [webrtc][blocking-webrtc-][spec-issue][webrtc-uplift] → [webrtc][blocking-webrtc-][spec-issue]
You need to log in before you can comment on or make changes to this bug.