Bug 43902 - flaky websocket/tests/frame-length-overflow.html
Summary: flaky websocket/tests/frame-length-overflow.html
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-12 02:30 PDT by Fumitoshi Ukai
Modified: 2010-08-13 05:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2010-08-12 02:37 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (3.70 KB, patch)
2010-08-12 03:02 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2010-08-12 22:22 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2010-08-13 00:59 PDT, Fumitoshi Ukai
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-08-12 02:30:58 PDT
chrome buildbot shows flaky failures of websocket/tests/frame-length-overflow.html
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=websocket%2Ftests%2Fframe-length-overflow.html

Actual test looks like:

Make sure WebSocket does not crash and report error when it sees length overflow

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

WebSocket is open
WebSocket received error frame
WebSocket received error frame
WebSocket is closed
PASS successfullyParsed is true

TEST COMPLETE


So, it received 2 error event before close.
I guess it would happen as follows:

 processBuffer post error event and leave buffer unchanged.
 before didClose is called, message loops runs again (more data comes from network or so) and calls processBuffer again (via didReceivedData or resumeTimerFired)
 it sees the same buffer again and post error event
Comment 1 Fumitoshi Ukai 2010-08-12 02:37:38 PDT
Created attachment 64197 [details]
Patch
Comment 2 Early Warning System Bot 2010-08-12 02:44:17 PDT
Attachment 64197 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3770091
Comment 3 Fumitoshi Ukai 2010-08-12 03:02:37 PDT
Created attachment 64200 [details]
Patch
Comment 4 WebKit Review Bot 2010-08-12 03:23:06 PDT
Attachment 64197 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3740070
Comment 5 Alexey Proskuryakov 2010-08-12 07:38:43 PDT
Comment on attachment 64200 [details]
Patch

> +    , m_errorClosed(false)

I don't like this name, it sounds as if the error itself were somehow closed. When is this variable set to true? If you can explain it in a full English sentence, that may help come up with a good variable name.

> +            skipBuffer(m_bufferSize); // Release buffer. We don't read any more data.

A comment saying why we're doing this would be more helpful than one saying what we're doing. Is this so save memory?

+    if (m_errorClosed)
+      return false;

Indentation is incorrect here.
Comment 6 Fumitoshi Ukai 2010-08-12 22:22:05 PDT
(In reply to comment #5)
> (From update of attachment 64200 [details])
> > +    , m_errorClosed(false)
> 
> I don't like this name, it sounds as if the error itself were somehow closed. When is this variable set to true? If you can explain it in a full English sentence, that may help come up with a good variable name.

how about m_readShutdowned.  It means the channel won't receive any more data, like SHUT_RD.
 
> > +            skipBuffer(m_bufferSize); // Release buffer. We don't read any more data.
> 
> A comment saying why we're doing this would be more helpful than one saying what we're doing. Is this so save memory?

Yes. and make sure it doesn't read the same buffer again.
 
> +    if (m_errorClosed)
> +      return false;
> 
> Indentation is incorrect here.
Comment 7 Fumitoshi Ukai 2010-08-12 22:22:23 PDT
Created attachment 64300 [details]
Patch
Comment 8 Alexey Proskuryakov 2010-08-12 23:10:50 PDT
+    , m_readShutdowned(false)

Shutdown is not a verb, so this should be "m_readShutDown" for better grammar. 

> It means the channel won't receive any more data, like SHUT_RD.

This sounds like something we should be asking the channel about then, not something to keep in WebSocket object. If it's better to keep this in WebSocket, then the name needs to be different yet, indicating what it means from this code point of view.

> Yes. and make sure it doesn't read the same buffer again.

Doesn't setting m_errorClosed/m_readShutdowned already prevent that?
Comment 9 Fumitoshi Ukai 2010-08-12 23:55:19 PDT
(In reply to comment #8)
> +    , m_readShutdowned(false)
> 
> Shutdown is not a verb, so this should be "m_readShutDown" for better grammar. 
> 
> > It means the channel won't receive any more data, like SHUT_RD.
> 
> This sounds like something we should be asking the channel about then, not something to keep in WebSocket object. If it's better to keep this in WebSocket, then the name needs to be different yet, indicating what it means from this code point of view.

Hmm, how about m_readClosed, or m_shouldDiscardReceivedData ?
 
> > Yes. and make sure it doesn't read the same buffer again.
> 
> Doesn't setting m_errorClosed/m_readShutdowned already prevent that?

Ah, yes.
Comment 10 Alexey Proskuryakov 2010-08-13 00:07:13 PDT
m_shouldDiscardReceivedData sounds like the best option so far.
Comment 11 Fumitoshi Ukai 2010-08-13 00:59:03 PDT
Created attachment 64310 [details]
Patch
Comment 12 Fumitoshi Ukai 2010-08-13 05:02:22 PDT
Committed r65313: <http://trac.webkit.org/changeset/65313>