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.
Created attachment 117138 [details] Patch Has dependency on https://bugs.webkit.org/show_bug.cgi?id=73290 So not ready for review yet.
FYI, on going chromium side's bufferedAmount implementation for Pepper API. http://codereview.chromium.org/8748002/
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) ...
Created attachment 117331 [details] Patch
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 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 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.
Created attachment 117396 [details] Patch
Comment on attachment 117396 [details] Patch Looks ok.
Comment on attachment 117396 [details] Patch Clearing flags on attachment: 117396 Committed r101730: <http://trac.webkit.org/changeset/101730>
All reviewed patches have been landed. Closing bug.