Bug 43902 – flaky websocket/tests/frame-length-overflow.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fumitoshi Ukai]

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

[Fumitoshi Ukai]

Created attachment 64197 [details]
Patch

[Early Warning System Bot]

Attachment 64197 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3770091

[Fumitoshi Ukai]

Created attachment 64200 [details]
Patch

[WebKit Review Bot]

Attachment 64197 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3740070

[Alexey Proskuryakov]

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.

[Fumitoshi Ukai]

(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.

[Fumitoshi Ukai]

Created attachment 64300 [details]
Patch

[Alexey Proskuryakov]

+    , 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?

[Fumitoshi Ukai]

(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.

[Alexey Proskuryakov]

m_shouldDiscardReceivedData sounds like the best option so far.

[Fumitoshi Ukai]

Created attachment 64310 [details]
Patch

[Fumitoshi Ukai]

Committed r65313: <http://trac.webkit.org/changeset/65313>