Bug 73404 - [WebSocket] bufferedAmount calculation is wrong in CLOSING and CLOSED state
Summary: [WebSocket] bufferedAmount calculation is wrong in CLOSING and CLOSED state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Takashi Toyoshima
URL:
Keywords:
Depends on: 73290
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-29 23:36 PST by Takashi Toyoshima
Modified: 2011-12-01 17:59 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.15 KB, patch)
2011-11-30 00:43 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2011-11-30 21:41 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (14.90 KB, patch)
2011-12-01 04:23 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Toyoshima 2011-11-29 23:36:41 PST
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.
Comment 1 Takashi Toyoshima 2011-11-30 00:43:20 PST
Created attachment 117138 [details]
Patch

Has dependency on https://bugs.webkit.org/show_bug.cgi?id=73290
So not ready for review yet.
Comment 2 Takashi Toyoshima 2011-11-30 00:45:34 PST
FYI, on going chromium side's bufferedAmount implementation for Pepper API.
http://codereview.chromium.org/8748002/
Comment 3 Kent Tamura 2011-11-30 01:47:50 PST
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) ...
Comment 4 Takashi Toyoshima 2011-11-30 21:41:31 PST
Created attachment 117331 [details]
Patch
Comment 5 Takashi Toyoshima 2011-11-30 21:43:35 PST
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.
Comment 6 Kent Tamura 2011-12-01 00:19:50 PST
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::".
Comment 7 Takashi Toyoshima 2011-12-01 01:03:45 PST
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.
Comment 8 Takashi Toyoshima 2011-12-01 04:23:53 PST
Created attachment 117396 [details]
Patch
Comment 9 Kent Tamura 2011-12-01 17:23:51 PST
Comment on attachment 117396 [details]
Patch

Looks ok.
Comment 10 WebKit Review Bot 2011-12-01 17:59:08 PST
Comment on attachment 117396 [details]
Patch

Clearing flags on attachment: 117396

Committed r101730: <http://trac.webkit.org/changeset/101730>
Comment 11 WebKit Review Bot 2011-12-01 17:59:18 PST
All reviewed patches have been landed.  Closing bug.