[WebSocket] bufferedAmount should not decrease inside a task.

Source/modules/websockets/WebSocket.cpp

 /*
  * Copyright (C) 2011 Google Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 209 matching lines @@
 
 const char* WebSocket::subprotocolSeperator()
 {
     return ", ";
 }
 
 WebSocket::WebSocket(ExecutionContext* context)
     : ActiveDOMObject(context)
     , m_state(CONNECTING)
     , m_bufferedAmount(0)
+    , m_bufferedAmountDecrease(0)
     , m_bufferedAmountAfterClose(0)
     , m_binaryType(BinaryTypeBlob)
     , m_subprotocol("")
     , m_extensions("")
     , m_eventQueue(EventQueue::create(this))
+    , m_bufferedAmountDecreaseTimer(this, &WebSocket::decreaseBufferedAmount)
 {
     ScriptWrappable::init(this);
 }
 
 WebSocket::~WebSocket()
 {
     ASSERT(!m_channel);
 }
 
 void WebSocket::logError(const String& message)
@@ skipping 120 matching lines @@
 }
 
 void WebSocket::updateBufferedAmountAfterClose(unsigned long payloadSize)
 {
     m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, payloadSize);
     m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, getFramingOverhead(payloadSize));
 
     logError("WebSocket is already in CLOSING or CLOSED state.");
 }
 
+void WebSocket::decreaseBufferedAmount(Timer<WebSocket>*)
+{
+    ASSERT(m_bufferedAmount >= m_bufferedAmountDecrease);
+    WTF_LOG(Network, "WebSocket %p decreaseBufferedAmount() %lu => %lu", this, m_bufferedAmount, m_bufferedAmount - m_bufferedAmountDecrease);
+
+    m_bufferedAmount -= m_bufferedAmountDecrease;
+    m_bufferedAmountDecrease = 0;
+}
+
 void WebSocket::releaseChannel()
 {
     ASSERT(m_channel);
     m_channel->disconnect();
     m_channel = nullptr;
 }
 
 void WebSocket::send(const String& message, ExceptionState& exceptionState)
 {
     WTF_LOG(Network, "WebSocket %p send() Sending String '%s'", this, message.utf8().data());
@@ skipping 247 matching lines @@
     }
 }
 
 void WebSocket::didReceiveMessageError()
 {
     WTF_LOG(Network, "WebSocket %p didReceiveMessageError()", this);
     m_state = CLOSED;
     m_eventQueue->dispatch(Event::create(EventTypeNames::error));
 }
 
-void WebSocket::didUpdateBufferedAmount(unsigned long bufferedAmount)
+void WebSocket::didIncreaseBufferedAmount(unsigned long amount)
 {
-    WTF_LOG(Network, "WebSocket %p didUpdateBufferedAmount() New bufferedAmount is %lu", this, bufferedAmount);
+    WTF_LOG(Network, "WebSocket %p didIncreaseBufferedAmount() %lu => %lu", this, m_bufferedAmount, m_bufferedAmount + amount);
     if (m_state == CLOSED)
         return;
-    m_bufferedAmount = bufferedAmount;
+    m_bufferedAmount += amount;
+}
+
+void WebSocket::didDecreaseBufferedAmount(unsigned long amount)
+{
+    ASSERT(m_bufferedAmount >= amount);
+    WTF_LOG(Network, "WebSocket %p didDecreaseBufferedAmount(%lu)", this, amount);
+    if (m_state == CLOSED)
+        return;
+    m_bufferedAmountDecrease += amount;
+    if (!m_bufferedAmountDecreaseTimer.isActive())
+        m_bufferedAmountDecreaseTimer.startOneShot(0, FROM_HERE);
 }
 
 void WebSocket::didStartClosingHandshake()
 {
     WTF_LOG(Network, "WebSocket %p didStartClosingHandshake()", this);
     m_state = CLOSING;
 }
 
 void WebSocket::didClose(unsigned long unhandledBufferedAmount, ClosingHandshakeCompletionStatus closingHandshakeCompletion, unsigned short code, const String& reason)
 {
     WTF_LOG(Network, "WebSocket %p didClose()", this);
     if (!m_channel)
         return;
     bool wasClean = m_state == CLOSING && !unhandledBufferedAmount && closingHandshakeCompletion == ClosingHandshakeComplete && code != WebSocketChannel::CloseEventCodeAbnormalClosure;
-
     m_state = CLOSED;

[tyoshino (SeeGerritForStatus), 2014/06/09 09:24:26]

is this change accidental?

anyway i'm fine with either keeping this or reverting this

[yhirano, 2014/06/09 11:59:51]

Reverted

     m_bufferedAmount = unhandledBufferedAmount;

[tyoshino (SeeGerritForStatus), 2014/06/09 09:24:26]

is it possible unhandledBufferedAmount and m_bufferedAmount become inconsistent?

[yhirano, 2014/06/09 11:59:51]

I noticed I didn't understand why unhandledBufferedAmount is needed.
I remove unhandledBufferedAmount parameter from this function and some
duplicated bufferedAmount members from WebSocketChannel classes. What do you
think?

[tyoshino (SeeGerritForStatus), 2014/06/10 07:24:56]

Took a look at the code and history of it. I agree that it's unnecessary at
least now. All the changes on m_buffer is reported and the last report by
didClose() is just redundant. Removal looks good.

     m_eventQueue->dispatch(CloseEvent::create(wasClean, code, reason));
     releaseChannel();
 }
 
 size_t WebSocket::getFramingOverhead(size_t payloadSize)
 {
     static const size_t hybiBaseFramingOverhead = 2; // Every frame has at least two-byte header.
     static const size_t hybiMaskingKeyLength = 4; // Every frame from client must have masking key.
     static const size_t minimumPayloadSizeWithTwoByteExtendedPayloadLength = 126;
     static const size_t minimumPayloadSizeWithEightByteExtendedPayloadLength = 0x10000;
     size_t overhead = hybiBaseFramingOverhead + hybiMaskingKeyLength;
     if (payloadSize >= minimumPayloadSizeWithEightByteExtendedPayloadLength)
         overhead += 8;
     else if (payloadSize >= minimumPayloadSizeWithTwoByteExtendedPayloadLength)
         overhead += 2;
     return overhead;
 }
 
 void WebSocket::trace(Visitor* visitor)
 {
     visitor->trace(m_channel);
     visitor->trace(m_eventQueue);
     EventTargetWithInlineData::trace(visitor);
 }
 
 } // namespace WebCore