Jason Duell Assignee
	Description • 13 years ago

We're not currently implementing any limit on how much traffic clients can queue for outgoing websocket messages.  This patch implements a configurable limit for bufferedAmount, and (as the W3C WebSockets spec requires) fails the WebSocket if the buffer would become full.

I've picked 16 MB as the limit for desktop, matching the existing max incoming message size, for now at least: one could make the case that the outgoing buffer should be bigger than the max incoming msg size, since we could handle several incoming msgs just under the max).  I'm open to discussing a different limit, but this seems safe for now. (Chrome on desktop appears to support > 16 MB: they pass the largest Autobahn test, which is 16 MB, while we don't, partly because we were treating 16 MB

I've added mobile prefs that override both the incoming max msg size and the outgoing buffer size to 4 MB.  That's an arbitrary number I pulled out of a hat, but it does seem likely that mobile is going to want a smaller size.  Hopefully someone on the mobile team can give me a reasonable limit to use if this isn't right.

Future work:  as far as resource use goes, there's actually a big difference between messages which we need to reserve RAM for (text msgs, binary arrays, blobs backed by RAM) and blobs backed by a file.  The latter we could support up to arbitrary sizes (it's really the disk that's the limit).  The spec allows us to be arbitrary about the max buffer length--we don't report it to the user, so we could conceivably only count RAM-backed bytes towards the limit.

	Jason Duell Assignee
	Comment 1 • 13 years ago

Smaug: let me know if you're not comfortable with the necko changes (I just changed "16MB" to mean power-of-two-based MB, and made the config option use KB for parity with other necko size options).

Do you have any idea how expensive looking up a pref is?  (I assume it's a hashtable lookup).  If it's expensive, in a followup we could change the logic to use some sort of global protocol-wide service (like nsHttpHandler) that hands out the values w/o doign a string-based lookup, and listens for pref change events.

	Jason Duell Assignee
	Comment 2 • 13 years ago

Mark, see comment 0.

	Jason Duell Assignee
	Comment 3 • 13 years ago

BTW I'm open to either changing the name of "network.websocket.max-message-size" to "network.websocket.incoming.max-message-size", or enforcing the limit for outgoing messages too.   Or I suppose we could change the pref to be for an incoming total buffer size.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 13 years ago

Comment on attachment 581902 [details] [diff] [review]
v1


>@@ -1223,16 +1224,22 @@ nsWebSocket::Send(nsIVariant *aData)
> 
>   nsCString msgString;
>   nsCOMPtr<nsIInputStream> msgStream;
>   bool isBinary;
>   PRUint32 msgLen;
>   nsresult rv = GetSendParams(aData, msgString, msgStream, isBinary, msgLen);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (mOutgoingBufferedAmount + msgLen > mMaxOutgoingBufferedAmount) {
>+    // Spec requires us to terminate websocket if buffer becomes full
>+    FailConnection();
>+    return NS_ERROR_FILE_TOO_BIG;
>+  }
>+
The spec doesn't say that any exception should be thrown.
The web socket connection should be just closed "with prejudice".

But I think the spec is strange. One could accidentally close the whole connection if some processing happens a bit slowly.


>@@ -1521,16 +1528,28 @@ nsWebSocket::Init(nsIPrincipal* aPrincip
>                                  nsContentUtils::GetContentPolicy(),
>                                  nsContentUtils::GetSecurityManager());
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (NS_CP_REJECTED(shouldLoad)) {
>     // Disallowed by content policy.
>     return NS_ERROR_CONTENT_BLOCKED;
>   }
> 
>+  // Get prefs
>+  nsCOMPtr<nsIPrefBranch> prefService;
>+  prefService = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  if (prefService) {
>+    PRInt32 intpref;
>+    rv = prefService->GetIntPref("network.websocket.max-bufferedamount",
>+                                 &intpref);
>+    if (NS_SUCCEEDED(rv)) {
>+      mMaxOutgoingBufferedAmount = clamped(intpref * 1024, 1024, 1 << 30);
>+    }
>+  }
There are helpers for this. Preferences::GetInt for example.
And mMaxOutgoingBufferedAmount could be probably a static member variable.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 13 years ago

Do we really want to prevent messages > 16MB?


Also, I filed
https://www.w3.org/Bugs/Public/show_bug.cgi?id=15209
and 
https://www.w3.org/Bugs/Public/show_bug.cgi?id=15210

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 6 • 13 years ago

4MB for mobile seems reasonable, but I'd like to find some web apps that use web sockets and see what amount of buffering is typical.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 13 years ago

sending 16MB doesn't sound a lot, and we don't have such artificial limit in XHR.
how can web page know about it?

We do have a similar limit for incoming message? Can server know the limit?
(I haven't reviewed the protocol spec for ages)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 13 years ago

Use one of these functions if you want quick access to a pref:

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/Preferences.h#273


I agree with Smaug that 16MB sounds like a very small limit. Pages generally are *really* bad at doing error handling and so are unlikely to deal well with running out of buffer space. And racking up 16MB of buffered data if you are careless doesn't sound hard on a slow-ish connection.

	Jason Duell Assignee
	Comment 9 • 13 years ago

I can test larger amount and see if they work.  Do we have a goal in mind?  64MB?  128M?  The sky's the limit?  We could conceivably support anything that malloc can handle, but I was worried about putting memory pressure on the rest of the application.  How do we limit (if we do) XHR/HTTP msg size?  I'll look into it if no one knows offhand...

	Jason Duell Assignee
	Comment 10 • 13 years ago

Thoughts on a sensible size limit for mobile also appreciated...

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 13 years ago

> How do we limit (if we do) XHR/HTTP msg size? 

We don't.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 12 • 13 years ago

The main thing I'd like to see is to not have a limit on the total amount of buffer that websocket uses. If the page sends 500 1MB messages we don't fail anything unless malloc fails to allocate any of the 1MB buffers. In other words, do we have to allocate a single buffer for the outgoing data rather than one buffer per message?

Other than that it seems reasonable to have a limit on a single message. Say something in the order of 128MB.

	Patrick McManus [:mcmanus]
	Comment 13 • 13 years ago

I'm pretty certain the inbound limit is per message. It sounds like the limit jason imposed outbound is in aggregate. Both could theoretically be removed without my objection.

	Jason Duell Assignee
	Comment 14 • 13 years ago

> we don't fail anything unless malloc fails

Yes, I'm doing that in bug 711205.  This bug is WONTFIX.

	Andrzej Grzegorz Borkowski
	Comment 15 • 13 years ago

http://www.lenholgate.com/blog/2011/07/websockets-is-a-stream-not-a-message-based-protocol.html