x/net/websocket: Receive returns ErrNotImplemented for unsolicited pong frames

[gopherbot]

by aicommander:

What steps will reproduce the problem?
0. Open a WebSocket which will be connected to Internet Explorer 11
1. Call WebSocket.Receive() without providing any data to receive
2. Wait 30 seconds or so for the Receive() call to fail

What is the expected output?
WebSocket.Receive() should continue to wait after receiving a pong.

What do you see instead?
WebSocket.Receive() returns ErrNotImplemented when it gets an unsolicited pong from IE

Which operating system are you using?
Windows 8.1 with IE 11

Which version are you using?  (run 'go version')
Go 1.1.2 Windows/AMD64

Please provide any additional information below.

This behavior does not adhere to RFC 6455 Section 5.5.3 which explicitly mentions that
browsers are allowed to send unsolicited pongs.

"A Pong frame MAY be sent unsolicited.  This serves as a unidirectional heartbeat. 
A response to an unsolicited Pong frame is not expected."

[gopherbot]

Comment 1 by aicommander:

Here is a potential patch for this issue. I've changed the code handling PongFrame in
Hybi's HandleFrame() function to return nil data and nil error (similar to how the
PingFrame is handled). Returning nil nil here makes WebSocket.Receive() wait for another
frame which is the intended behavior.
diff -r bc411e2ac33f websocket/hybi.go
--- a/websocket/hybi.go Thu Aug 15 13:52:04 2013 +1000
+++ b/websocket/hybi.go Thu Sep 12 18:20:12 2013 -0400
@@ -301,7 +301,7 @@
        }
        return nil, nil
    case PongFrame:
-       return nil, ErrNotImplemented
+       return nil, nil
    }
    return frame, nil
 }

[rsc]

Comment 2:

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

[rsc]

Comment 3:

Labels changed: added go1.3maybe.

[rsc]

Comment 4:

Labels changed: added release-none, removed go1.3maybe.

[rsc]

Comment 5:

Labels changed: added repo-net.

[gopherbot]

Comment 6 by ledangster:

I noticed that while it is returning ErrNotImplemented, can it be further made useful by
discarding the entire frame itself, similar to what the PingFrame handler does? The
connection becomes useless after receiving a Pong frame because of the frame not being
fully junked away.
I did a test by sending a PingFrame from the server and just waited until a Pong was
received from the browser:
  conn.PayloadTyhpe = websocket.PingFrame
  _, err = conn.Write([]byte("ping")

[gopherbot]

Comment 7 by ledangster:

aicomman...@gmail.com ... returning nil, nil will not be sufficient as there's seems to
remain the frame payload which hasn't been skipped, so per my previous message, the
PongFrame needs to be discarded the similar to PingFrame:
diff -r 93f5aef8cb25 websocket/hybi.go
--- a/websocket/hybi.go Wed Jan 29 10:23:52 2014 +0900
+++ b/websocket/hybi.go Wed Feb 26 10:00:05 2014 -0800
@@ -288,20 +288,21 @@
        handler.payloadType = frame.PayloadType()
    case CloseFrame:
        return nil, io.EOF
-   case PingFrame:
+   case PingFrame, PongFrame:
        pingMsg := make([]byte, maxControlFramePayloadLength)
        n, err := io.ReadFull(frame, pingMsg)
        if err != nil && err != io.ErrUnexpectedEOF {
            return nil, err
        }
        io.Copy(ioutil.Discard, frame)
+       if frame.PayloadType() == PongFrame {
+           return nil, ErrNotImplemented
+       }
        n, err = handler.WritePong(pingMsg[:n])
        if err != nil {
            return nil, err
        }
        return nil, nil
-   case PongFrame:
-       return nil, ErrNotImplemented
    }
    return frame, nil
 }

[gopherbot]

Comment 8 by matusis:

ledangs...@gmail.com : your patch seems to return EOF err for an unexpected Pong frame,
which an application cannot distinguish from a normal client disconnect. I think
PongFrame needs to be handled separately from PingFrame:
--- hybi.go     2014-09-04 11:03:07.000000000 -0700
+++ hybi.go.patched     2014-09-04 11:02:58.000000000 -0700
@@ -301,7 +301,14 @@
                }
                return nil, nil
        case PongFrame:
-               return nil, ErrNotImplemented
+               //discard unexpected Pong frame
+               pongMsg := make([]byte, maxControlFramePayloadLength)
+               _, err := io.ReadFull(frame, pongMsg)
+               if err != nil && err != io.EOF {
+                       return nil, err
+               }
+               io.Copy(ioutil.Discard, frame)
+               return nil,  nil
        }
        return frame, nil
 }
It would be nice if the code was actually fixed in the repo, since it causes real world
disconnects with IE11.

[gopherbot]

Comment 9 by ledangster:

matu...@gmail.com: that's a weird misbehavior of io.EOF (golang spec says/recommends the
use of EOF to indicate an end of stream - due to connection being disconnected). If
io.ReadFull() is getting an EOF, then there's a bug somewhere in the hybi frame Read
handler.
btw, have you tested this against all the other browsers? I don't have ie11 on hand to
play with.
Like you, I don't have control of the repo.

[gopherbot]

Comment 10 by matusis:

ledangs...@gmail.com : I tested in Firefox, IE11, Chrome and Safari on iOS, and it does
not disconnect.

[mattbaird]

i have tested this patch with IE 11, Chrome, Safari and firefox and it fixes the issue (websockets unusable on ie, and does not effect other browsers). Any chance this gets integrated soon? It's very hard/impossible to patch this through forking when releasing a product.

[fredlebel]

This is also an issue with the WebSocket implementation in WinJS. Is there any chance we can get an estimate on when this issue will be resolved?

[ianlancetaylor]

It looks like people have an idea as to what to fix. Can somebody send in a CL?

[fredlebel]

We're going to create a PR for this. We'll take the patch from comment 8 and test it with the AppRTC server and WinJS which is where we're having issues. Is there anything else we should know before starting this effort? Thanks.

[ianlancetaylor]

If by PR you mean pull request, please be aware that Go does not accept Github pull requests. In to contribute, please see https://golang.org/doc/contribute.html . Thanks.

[gopherbot]

CL https://golang.org/cl/13054 mentions this issue.