- Revision
- 101730
- Author
- commit-qu...@webkit.org
- Date
- 2011-12-01 17:58:49 -0800 (Thu, 01 Dec 2011)
Log Message
bufferedAmount calculation is wrong in CLOSING and CLOSED state.
https://bugs.webkit.org/show_bug.cgi?id=73404
Patch by Takashi Toyoshima <toyos...@chromium.org> on 2011-12-01
Reviewed by Kent Tamura.
Source/WebCore:
WebSocket::bufferedAmount() must return buffered frame size including
disposed frames which are passed via send() calls after close().
Old implementation had a problem at CLOSING state. Buffered frame size
was added to m_bufferedAmountAfterClose at close(). But the function
returns the sum of m_bufferedAmountAfterClose and internally buffered
frame size, or m_channel->bufferedAmount(). So, buffered frames was
double counted.
In new implementation, m_bufferedAmount always represents buffered
frame size and m_bufferedAmountAfterClose does disposed frame size.
As a result, bufferedAmount() implementation become just to return the
sum of m_bufferedAmount and m_bufferedAmountAfterClose.
Test: http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html
* websockets/WebSocket.cpp: Implement new bufferedAmount handling.
(WebCore::saturateAdd):
(WebCore::WebSocket::WebSocket):
(WebCore::WebSocket::send):
(WebCore::WebSocket::close):
(WebCore::WebSocket::bufferedAmount):
(WebCore::WebSocket::didUpdateBufferedAmount):
(WebCore::WebSocket::didClose):
* websockets/WebSocket.h:
LayoutTests:
Add a layout test to check the WebSocket bufferedAmount property.
This test close the socket channel when it is busy and buffer some
message frames, then check the property's value in CLOSING and CLOSED
state.
* http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt: Added.
* http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (101729 => 101730)
--- trunk/LayoutTests/ChangeLog 2011-12-02 01:56:53 UTC (rev 101729)
+++ trunk/LayoutTests/ChangeLog 2011-12-02 01:58:49 UTC (rev 101730)
@@ -1,3 +1,18 @@
+2011-12-01 Takashi Toyoshima <toyos...@chromium.org>
+
+ bufferedAmount calculation is wrong in CLOSING and CLOSED state.
+ https://bugs.webkit.org/show_bug.cgi?id=73404
+
+ Reviewed by Kent Tamura.
+
+ Add a layout test to check the WebSocket bufferedAmount property.
+ This test close the socket channel when it is busy and buffer some
+ message frames, then check the property's value in CLOSING and CLOSED
+ state.
+
+ * http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt: Added.
+ * http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html: Added.
+
2011-12-01 Vincent Scheib <sch...@chromium.org>
[Chromium] Marking Flakey tests in test_expectations:
Added: trunk/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt (0 => 101730)
--- trunk/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt 2011-12-02 01:58:49 UTC (rev 101730)
@@ -0,0 +1,54 @@
+Web Socket bufferedAmount after closed in busy
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+Connected.
+PASS bufferedAmountBeforeClose + closeFrameSize >= bufferedAmountAfterClose is true
+Closed.
+PASS ws.readyState is 3
+PASS ws.bufferedAmount <= bufferedAmountAfterClose is true
+Testing send(string)...
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 27
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 6
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 7
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 131
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 134
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 65543
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 65550
+Testing send(ArrayBuffer)...
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 6
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 7
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 131
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 134
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 65543
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 65550
+Testing send(Blob)...
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 6
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 7
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 131
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 134
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 65543
+PASS ws.send(messageToSend) is false
+PASS bufferedAmountDifference is 65550
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html (0 => 101730)
--- trunk/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html (rev 0)
+++ trunk/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html 2011-12-02 01:58:49 UTC (rev 101730)
@@ -0,0 +1,112 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<div id="description"></div>
+<div id="console"></div>
+<script type="text/_javascript_">
+description("Web Socket bufferedAmount after closed in busy");
+
+window.jsTestIsAsync = true;
+if (window.layoutTestController)
+ layoutTestController.overridePreference("WebKitHixie76WebSocketProtocolEnabled", 0);
+
+function createStringWithLength(length)
+{
+ var string = 'x';
+ while (string.length < length)
+ string = string + string;
+ return string.substring(0, length);
+}
+
+function createBlobWithLength(length)
+{
+ var builder = new WebKitBlobBuilder();
+ builder.append(new ArrayBuffer(length));
+ return builder.getBlob();
+}
+
+var ws = new WebSocket("ws://localhost:8880/websocket/tests/hybi/simple");
+
+var bufferedAmountBeforeClose = 0;
+var bufferedAmountAfterClose = 0;
+var bufferedAmountAfterClosed = 0;
+var closeFrameSize = 6;
+
+ws._onopen_ = function()
+{
+ debug("Connected.");
+
+ var data = ""
+ for (var i = 0; i < 100; ++i) {
+ ws.send(data);
+ if (ws.bufferedAmount > 262144)
+ break;
+ }
+ bufferedAmountBeforeClose = ws.bufferedAmount;
+ ws.close();
+ bufferedAmountAfterClose = ws.bufferedAmount;
+ shouldBeTrue("bufferedAmountBeforeClose + closeFrameSize >= bufferedAmountAfterClose");
+};
+
+ws._onclose_ = function()
+{
+ debug("Closed.");
+ shouldBe("ws.readyState", "3");
+ shouldBeTrue("ws.bufferedAmount <= bufferedAmountAfterClose");
+
+ var baseOverhead = 2 + 4; // Base header size and masking key size.
+ debug("Testing send(string)...");
+ testBufferedAmount('send to closed socket', 21 + baseOverhead);
+ testBufferedAmount('', baseOverhead);
+ testBufferedAmount('a', 1 + baseOverhead);
+ testBufferedAmount(createStringWithLength(125), 125 + baseOverhead);
+ testBufferedAmount(createStringWithLength(126), 126 + baseOverhead + 2); // With 16-bit extended payload length field.
+ testBufferedAmount(createStringWithLength(0xFFFF), 0xFFFF + baseOverhead + 2);
+ testBufferedAmount(createStringWithLength(0x10000), 0x10000 + baseOverhead + 8); // With 64-bit extended payload length field.
+
+ debug("Testing send(ArrayBuffer)...");
+ testBufferedAmount(new ArrayBuffer(0), baseOverhead);
+ testBufferedAmount(new ArrayBuffer(1), 1 + baseOverhead);
+ testBufferedAmount(new ArrayBuffer(125), 125 + baseOverhead);
+ testBufferedAmount(new ArrayBuffer(126), 126 + baseOverhead + 2);
+ testBufferedAmount(new ArrayBuffer(0xFFFF), 0xFFFF + baseOverhead + 2);
+ testBufferedAmount(new ArrayBuffer(0x10000), 0x10000 + baseOverhead + 8);
+
+ debug("Testing send(Blob)...");
+ testBufferedAmount(createBlobWithLength(0), baseOverhead);
+ testBufferedAmount(createBlobWithLength(1), 1 + baseOverhead);
+ testBufferedAmount(createBlobWithLength(125), 125 + baseOverhead);
+ testBufferedAmount(createBlobWithLength(126), 126 + baseOverhead + 2);
+ testBufferedAmount(createBlobWithLength(0xFFFF), 0xFFFF + baseOverhead + 2);
+ testBufferedAmount(createBlobWithLength(0x10000), 0x10000 + baseOverhead + 8);
+ finishJSTest();
+};
+
+ws._onerror_ = function()
+{
+ debug("Error.");
+}
+
+var messageToSend;
+var bufferedAmountDifference;
+
+function testBufferedAmount(message, expectedBufferedAmountDifference)
+{
+ // If the connection is closed, bufferedAmount attribute's value will only
+ // increase with each call to the send() method.
+ // (the number does not reset to zero once the connection closes).
+ messageToSend = message;
+ var bufferedAmountBeforeSend = ws.bufferedAmount;
+ shouldBeFalse("ws.send(messageToSend)");
+ var bufferedAmountAfterSend = ws.bufferedAmount;
+ bufferedAmountDifference = bufferedAmountAfterSend - bufferedAmountBeforeSend;
+ shouldEvaluateTo("bufferedAmountDifference", expectedBufferedAmountDifference);
+}
+
+</script>
+<script src=""
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (101729 => 101730)
--- trunk/Source/WebCore/ChangeLog 2011-12-02 01:56:53 UTC (rev 101729)
+++ trunk/Source/WebCore/ChangeLog 2011-12-02 01:58:49 UTC (rev 101730)
@@ -1,3 +1,36 @@
+2011-12-01 Takashi Toyoshima <toyos...@chromium.org>
+
+ bufferedAmount calculation is wrong in CLOSING and CLOSED state.
+ https://bugs.webkit.org/show_bug.cgi?id=73404
+
+ Reviewed by Kent Tamura.
+
+ WebSocket::bufferedAmount() must return buffered frame size including
+ disposed frames which are passed via send() calls after close().
+
+ Old implementation had a problem at CLOSING state. Buffered frame size
+ was added to m_bufferedAmountAfterClose at close(). But the function
+ returns the sum of m_bufferedAmountAfterClose and internally buffered
+ frame size, or m_channel->bufferedAmount(). So, buffered frames was
+ double counted.
+
+ In new implementation, m_bufferedAmount always represents buffered
+ frame size and m_bufferedAmountAfterClose does disposed frame size.
+ As a result, bufferedAmount() implementation become just to return the
+ sum of m_bufferedAmount and m_bufferedAmountAfterClose.
+
+ Test: http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html
+
+ * websockets/WebSocket.cpp: Implement new bufferedAmount handling.
+ (WebCore::saturateAdd):
+ (WebCore::WebSocket::WebSocket):
+ (WebCore::WebSocket::send):
+ (WebCore::WebSocket::close):
+ (WebCore::WebSocket::bufferedAmount):
+ (WebCore::WebSocket::didUpdateBufferedAmount):
+ (WebCore::WebSocket::didClose):
+ * websockets/WebSocket.h:
+
2011-12-01 Kentaro Hara <hara...@chromium.org>
Replace a custom constructor of window.Option with the [NamedConstructor] IDL
Modified: trunk/Source/WebCore/websockets/WebSocket.cpp (101729 => 101730)
--- trunk/Source/WebCore/websockets/WebSocket.cpp 2011-12-02 01:56:53 UTC (rev 101729)
+++ trunk/Source/WebCore/websockets/WebSocket.cpp 2011-12-02 01:58:49 UTC (rev 101730)
@@ -59,6 +59,8 @@
#include <wtf/text/StringBuilder.h>
#include <wtf/text/WTFString.h>
+using namespace std;
+
namespace WebCore {
const size_t maxReasonSizeInBytes = 123;
@@ -126,6 +128,13 @@
return builder.toString();
}
+static unsigned long saturateAdd(unsigned long a, unsigned long b)
+{
+ if (numeric_limits<unsigned long>::max() - a < b)
+ return numeric_limits<unsigned long>::max();
+ return a + b;
+}
+
static bool webSocketsAvailable = false;
void WebSocket::setIsAvailable(bool available)
@@ -141,6 +150,7 @@
WebSocket::WebSocket(ScriptExecutionContext* context)
: ActiveDOMObject(context, this)
, m_state(CONNECTING)
+ , m_bufferedAmount(0)
, m_bufferedAmountAfterClose(0)
, m_binaryType(BinaryTypeBlob)
, m_useHixie76Protocol(true)
@@ -266,7 +276,8 @@
// No exception is raised if the connection was once established but has subsequently been closed.
if (m_state == CLOSING || m_state == CLOSED) {
size_t payloadSize = message.utf8().length();
- m_bufferedAmountAfterClose += payloadSize + getFramingOverhead(payloadSize);
+ m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, payloadSize);
+ m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, getFramingOverhead(payloadSize));
return false;
}
// FIXME: check message is valid utf8.
@@ -285,7 +296,9 @@
return false;
}
if (m_state == CLOSING || m_state == CLOSED) {
- m_bufferedAmountAfterClose += binaryData->byteLength() + getFramingOverhead(binaryData->byteLength());
+ unsigned payloadSize = binaryData->byteLength();
+ m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, payloadSize);
+ m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, getFramingOverhead(payloadSize));
return false;
}
ASSERT(m_channel);
@@ -304,7 +317,8 @@
}
if (m_state == CLOSING || m_state == CLOSED) {
unsigned long payloadSize = static_cast<unsigned long>(binaryData->size());
- m_bufferedAmountAfterClose += payloadSize + getFramingOverhead(payloadSize);
+ m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, payloadSize);
+ m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, getFramingOverhead(payloadSize));
return false;
}
ASSERT(m_channel);
@@ -336,9 +350,6 @@
return;
}
m_state = CLOSING;
- m_bufferedAmountAfterClose = m_channel->bufferedAmount();
- // didClose notification may be already queued, which we will inadvertently process while waiting for bufferedAmount() to return.
- // In this case m_channel will be set to null during didClose() call, thus we need to test validness of m_channel here.
if (m_channel)
m_channel->close(code, reason);
}
@@ -355,11 +366,7 @@
unsigned long WebSocket::bufferedAmount() const
{
- if (m_state == OPEN)
- return m_channel->bufferedAmount();
- else if (m_state == CLOSING)
- return m_channel->bufferedAmount() + m_bufferedAmountAfterClose;
- return m_bufferedAmountAfterClose;
+ return saturateAdd(m_bufferedAmount, m_bufferedAmountAfterClose);
}
String WebSocket::protocol() const
@@ -507,8 +514,10 @@
void WebSocket::didUpdateBufferedAmount(unsigned long bufferedAmount)
{
- UNUSED_PARAM(bufferedAmount);
LOG(Network, "WebSocket %p didUpdateBufferedAmount %lu", this, bufferedAmount);
+ if (m_state == CLOSED)
+ return;
+ m_bufferedAmount = bufferedAmount;
}
void WebSocket::didStartClosingHandshake()
@@ -524,7 +533,7 @@
return;
bool wasClean = m_state == CLOSING && !unhandledBufferedAmount && closingHandshakeCompletion == ClosingHandshakeComplete;
m_state = CLOSED;
- m_bufferedAmountAfterClose += unhandledBufferedAmount;
+ m_bufferedAmount = unhandledBufferedAmount;
ASSERT(scriptExecutionContext());
RefPtr<CloseEvent> event = CloseEvent::create(wasClean, code, reason);
dispatchEvent(event);
Modified: trunk/Source/WebCore/websockets/WebSocket.h (101729 => 101730)
--- trunk/Source/WebCore/websockets/WebSocket.h 2011-12-02 01:56:53 UTC (rev 101729)
+++ trunk/Source/WebCore/websockets/WebSocket.h 2011-12-02 01:58:49 UTC (rev 101730)
@@ -131,6 +131,7 @@
State m_state;
KURL m_url;
EventTargetData m_eventTargetData;
+ unsigned long m_bufferedAmount;
unsigned long m_bufferedAmountAfterClose;
BinaryType m_binaryType;
bool m_useHixie76Protocol;
