bufferedAmount calculation is wrong in CLOSING and CLOSED state.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Dec 2011 01:58:49 +0000 (01:58 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Dec 2011 01:58:49 +0000 (01:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=73404

Patch by Takashi Toyoshima <toyoshim@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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@101730 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/websockets/WebSocket.cpp
Source/WebCore/websockets/WebSocket.h

index 678a6a3..9014a43 100644 (file)
@@ -1,3 +1,18 @@
+2011-12-01  Takashi Toyoshima  <toyoshim@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  <scheib@chromium.org>
 
         [Chromium] Marking Flakey tests in test_expectations:
diff --git a/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt b/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy-expected.txt
new file mode 100644 (file)
index 0000000..c54780c
--- /dev/null
@@ -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
+
diff --git a/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html b/LayoutTests/http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html
new file mode 100644 (file)
index 0000000..637528b
--- /dev/null
@@ -0,0 +1,112 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../../../js-test-resources/js-test-pre.js"></script>
+</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 = createStringWithLength(8193);
+    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="../../../../js-test-resources/js-test-post.js"></script>
+</body>
+</html>
index f98fae4..381cb53 100644 (file)
@@ -1,3 +1,36 @@
+2011-12-01  Takashi Toyoshima  <toyoshim@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  <haraken@chromium.org>
 
         Replace a custom constructor of window.Option with the [NamedConstructor] IDL
index f9fd2fe..682fab1 100644 (file)
@@ -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 @@ static String joinStrings(const Vector<String>& strings, const char* separator)
     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 @@ bool WebSocket::isAvailable()
 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 @@ bool WebSocket::send(const String& message, ExceptionCode& ec)
     // 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 @@ bool WebSocket::send(ArrayBuffer* binaryData, ExceptionCode& ec)
         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 @@ bool WebSocket::send(Blob* binaryData, ExceptionCode& ec)
     }
     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 @@ void WebSocket::close(int code, const String& reason, ExceptionCode& ec)
         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 @@ WebSocket::State WebSocket::readyState() const
 
 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::didReceiveMessageError()
 
 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 @@ void WebSocket::didClose(unsigned long unhandledBufferedAmount, ClosingHandshake
         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);
index c1363e9..b860b75 100644 (file)
@@ -131,6 +131,7 @@ private:
     State m_state;
     KURL m_url;
     EventTargetData m_eventTargetData;
+    unsigned long m_bufferedAmount;
     unsigned long m_bufferedAmountAfterClose;
     BinaryType m_binaryType;
     bool m_useHixie76Protocol;