Bug 73404 – [WebSocket] bufferedAmount calculation is wrong in CLOSING and CLOSED state

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Takashi Toyoshima]

In WebSocket::close(), m_bufferedAmountAfterClose is set to m_channel->bufferedAmount() and state is changed to CLOSING.
On the other hand, WebSocket:bufferedAmount() returns m_channel->bufferedAmount() + m_bufferedAmountAfterClose in CLOSING state.
That means bufferedAmount is double counted in CLOSING state.
Also, in didClose(), we just added unhandledBufferedAmount to m_bufferedAmountAfterClose.
In CLOSE state, WebSocket::bufferedAmount() just returns m_bufferedAmountAfterClose. But still it contains previously added bufferedAmount() in addition to unhandledBufferedAmount.

[Takashi Toyoshima]

Created attachment 117138 [details]
Patch

Has dependency on https://bugs.webkit.org/show_bug.cgi?id=73290
So not ready for review yet.

[Takashi Toyoshima]

FYI, on going chromium side's bufferedAmount implementation for Pepper API.
http://codereview.chromium.org/8748002/

[Kent Tamura]

Comment on attachment 117138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117138&action=review

> Source/WebCore/websockets/WebSocket.cpp:571
> +    unsigned long c = a + b;
> +    if (c < a || c < b)
> +        return std::numeric_limits<unsigned long>::max();

I think this code is correct according to the C99 standard, but I guess some people might be afraid that this code is not correct on some platforms.
I recommend changing this to
  if (numeric_limits<unsigned long>::max() - a < b) ...

[Takashi Toyoshima]

Created attachment 117331 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 117138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117138&action=review

>> Source/WebCore/websockets/WebSocket.cpp:571
>> +        return std::numeric_limits<unsigned long>::max();
> 
> I think this code is correct according to the C99 standard, but I guess some people might be afraid that this code is not correct on some platforms.
> I recommend changing this to
>   if (numeric_limits<unsigned long>::max() - a < b) ...

Thank you.
I fix this code and Chromium side code, too.

Then, I change this method as a static function out of class.

[Kent Tamura]

Comment on attachment 117331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117331&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests because this change contains no functional update.

Correcting bufferedAmount calculation must be a functional change.

> Source/WebCore/ChangeLog:10
> +        * websockets/WebSocket.cpp: Change bufferedAmount calculation logic.

Please explain what is changed concretely.

> Source/WebCore/websockets/WebSocket.cpp:132
> +    if (std::numeric_limits<unsigned long>::max() - a < b)
> +        return std::numeric_limits<unsigned long>::max();

We usually declare "using namespace std;" and don't prepend "std::".

[Takashi Toyoshima]

Comment on attachment 117331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117331&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests because this change contains no functional update.
> 
> Correcting bufferedAmount calculation must be a functional change.

I see.
Actually, layout tests cover failure situation, but seems not to catch any errors until now.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2FbufferedAmount-after-close.html

So I'll try to add new test which will reproduce the error cases.
The test must close Websocket with over 32K buffered data, then query the bufferedAmount value as soon as possible.

Thank you.

>> Source/WebCore/ChangeLog:10
>> +        * websockets/WebSocket.cpp: Change bufferedAmount calculation logic.
> 
> Please explain what is changed concretely.

OK. I'll do in the next patch.

>> Source/WebCore/websockets/WebSocket.cpp:132
>> +        return std::numeric_limits<unsigned long>::max();
> 
> We usually declare "using namespace std;" and don't prepend "std::".

Fixed.

[Takashi Toyoshima]

Created attachment 117396 [details]
Patch

[Kent Tamura]

Comment on attachment 117396 [details]
Patch

Looks ok.

[WebKit Review Bot]

Comment on attachment 117396 [details]
Patch

Clearing flags on attachment: 117396

Committed r101730: <http://trac.webkit.org/changeset/101730>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.