DOMWebSocket::bufferedAmount() should not return an "unsigned long"

Source/modules/websockets/DOMWebSocket.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 187 matching lines @@
 {
     StringBuilder builder;
     for (size_t i = 0; i < strings.size(); ++i) {
         if (i)
             builder.append(separator);
         builder.append(strings[i]);
     }
     return builder.toString();
 }
 
-static unsigned long saturateAdd(unsigned long a, unsigned long b)

[tyoshino (SeeGerritForStatus), 2014/11/17 14:15:32]

please leave the second argument type "unsigned long".

[Paritosh Kumar, 2014/11/18 09:38:44]

Thanks

Done.

+static unsigned saturateAdd(unsigned a, unsigned b)
 {
-    if (std::numeric_limits<unsigned long>::max() - a < b)
-        return std::numeric_limits<unsigned long>::max();
+    if (std::numeric_limits<unsigned>::max() - a < b)
+        return std::numeric_limits<unsigned>::max();
     return a + b;
 }
 
 static void setInvalidStateErrorForSendMethod(ExceptionState& exceptionState)
 {
     exceptionState.throwDOMException(InvalidStateError, "Still in CONNECTING state.");
 }
 
 const char* DOMWebSocket::subprotocolSeperator()
 {
@@ skipping 120 matching lines @@
     m_channel = createChannel(executionContext(), this);
 
     if (!m_channel->connect(m_url, protocolString)) {
         m_state = CLOSED;
         exceptionState.throwSecurityError("An insecure WebSocket connection may not be initiated from a page loaded over HTTPS.");
         releaseChannel();
         return;
     }
 }
 
-void DOMWebSocket::updateBufferedAmountAfterClose(unsigned long payloadSize)

[tyoshino (SeeGerritForStatus), 2014/11/17 14:15:32]

please leave this unchanged

[Paritosh Kumar, 2014/11/18 09:38:44]

Done.

+void DOMWebSocket::updateBufferedAmountAfterClose(unsigned payloadSize)
 {
     m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, payloadSize);
     m_bufferedAmountAfterClose = saturateAdd(m_bufferedAmountAfterClose, getFramingOverhead(payloadSize));
 
     logError("WebSocket is already in CLOSING or CLOSED state.");
 }
 
 void DOMWebSocket::reflectBufferedAmountConsumption(Timer<DOMWebSocket>*)
 {
     ASSERT(m_bufferedAmount >= m_consumedBufferedAmount);
     WTF_LOG(Network, "WebSocket %p reflectBufferedAmountConsumption() %lu => %lu", this, m_bufferedAmount, m_bufferedAmount - m_consumedBufferedAmount);

[eroman, 2014/11/18 03:02:10]

This should be %u now

[Paritosh Kumar, 2014/11/18 09:38:44]

Done.

 
     m_bufferedAmount -= m_consumedBufferedAmount;
     m_consumedBufferedAmount = 0;
 }
 
 void DOMWebSocket::releaseChannel()
 {
     ASSERT(m_channel);
     m_channel->disconnect();
     m_channel = nullptr;
 }
 
 void DOMWebSocket::send(const String& message, ExceptionState& exceptionState)
 {
     WTF_LOG(Network, "WebSocket %p send() Sending String '%s'", this, message.utf8().data());
     if (m_state == CONNECTING) {
         setInvalidStateErrorForSendMethod(exceptionState);
         return;
     }
     // No exception is raised if the connection was once established but has subsequently been closed.
     if (m_state == CLOSING || m_state == CLOSED) {
         updateBufferedAmountAfterClose(message.utf8().length());

[eroman, 2014/11/18 03:02:10]

This (and other callsites) are potentially an integer truncation (going from
size_t to unsigned). toyoshino is this a concern?

[tyoshino (SeeGerritForStatus), 2014/11/18 03:38:03]

Yes. They should be fixed. It's ok to either take care of it in this CL or leave
it to us.

[Paritosh Kumar, 2014/11/18 09:38:44]

Thanks, I'm leaving this one now.

         return;
     }
     Platform::current()->histogramEnumeration("WebCore.WebSocket.SendType", WebSocketSendTypeString, WebSocketSendTypeMax);
     ASSERT(m_channel);
     m_bufferedAmount += message.utf8().length();
     m_channel->send(message);
 }
 
 void DOMWebSocket::send(DOMArrayBuffer* binaryData, ExceptionState& exceptionState)
 {
@@ skipping 97 matching lines @@
 const KURL& DOMWebSocket::url() const
 {
     return m_url;
 }
 
 DOMWebSocket::State DOMWebSocket::readyState() const
 {
     return m_state;
 }
 
-unsigned long DOMWebSocket::bufferedAmount() const
+unsigned DOMWebSocket::bufferedAmount() const
 {
     return saturateAdd(m_bufferedAmount, m_bufferedAmountAfterClose);
 }
 
 String DOMWebSocket::protocol() const
 {
     return m_subprotocol;
 }
 
 String DOMWebSocket::extensions() const
@@ skipping 120 matching lines @@
     }
 }
 
 void DOMWebSocket::didError()
 {
     WTF_LOG(Network, "WebSocket %p didError()", this);
     m_state = CLOSED;
     m_eventQueue->dispatch(Event::create(EventTypeNames::error));
 }
 
 void DOMWebSocket::didConsumeBufferedAmount(unsigned long consumed)

[eroman, 2014/11/18 03:02:10]

Should this be changed to an unsigned too?

[Paritosh Kumar, 2014/11/18 09:38:44]

Thanks
I think this should be changed to unsigned, since m_consumedBufferedAmount is
unsigned only.

 {
     ASSERT(m_bufferedAmount >= consumed);
     WTF_LOG(Network, "WebSocket %p didConsumeBufferedAmount(%lu)", this, consumed);
     if (m_state == CLOSED)
         return;
     m_consumedBufferedAmount += consumed;
     if (!m_bufferedAmountConsumeTimer.isActive())
         m_bufferedAmountConsumeTimer.startOneShot(0, FROM_HERE);
 }
 
@@ skipping 32 matching lines @@
 
 void DOMWebSocket::trace(Visitor* visitor)
 {
     visitor->trace(m_channel);
     visitor->trace(m_eventQueue);
     WebSocketChannelClient::trace(visitor);
     EventTargetWithInlineData::trace(visitor);
 }
 
 } // namespace blink