Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compressed messages fail in Safari #218

Closed
corny opened this issue Mar 26, 2020 · 16 comments
Closed

Compressed messages fail in Safari #218

corny opened this issue Mar 26, 2020 · 16 comments
Labels
Milestone

Comments

@corny
Copy link

corny commented Mar 26, 2020

When Safari 13.1 for MacOS receives a compressed text message, it prints WebSocket connection to 'ws://localhost:8080/ws' failed: Could not decode a text frame as UTF-8 on the Javascript console and closes the WebSocket connection.

This is probably a bug in Safari. But this issue might help others to disable compression as a workaround. They have to pass AcceptOptions to Accept():

websocket.AcceptOptions{
	CompressionMode: websocket.CompressionDisabled,
}
@nhooyr
Copy link
Contributor

nhooyr commented Mar 27, 2020

Received an email report about this as well, will look into it.

For now you can disable compression.

@nhooyr nhooyr added the bug label Mar 27, 2020
@nhooyr
Copy link
Contributor

nhooyr commented Mar 27, 2020

Might be https://bugs.webkit.org/show_bug.cgi?id=202401 as reported in the email I got.

@nhooyr nhooyr added needs-info and removed bug labels Mar 27, 2020
@nhooyr
Copy link
Contributor

nhooyr commented Mar 27, 2020

Please let me know if this occurs on messages smaller than 4092 bytes.

@corny
Copy link
Author

corny commented Mar 27, 2020

Yes, this also occurs with much smaller messages. A message with 186 bytes uncompressed size already causes the error. Messages with 30-40 bytes work well - maybe because they won't be compressed?

@nhooyr
Copy link
Contributor

nhooyr commented Mar 30, 2020

Hmm, weird. Could you try the technology preview and see if that fixes it as the patches landed in Safari several months ago.

@nhooyr
Copy link
Contributor

nhooyr commented Mar 30, 2020

Hmm actually fixes should be in 13.1 as it released just a week ago.

@nhooyr nhooyr added docs and removed needs-info labels Mar 30, 2020
jeremija added a commit to peer-calls/peer-calls that referenced this issue Mar 30, 2020
Subscription error: client.subscribeRead - error reading data: failed to get reader: failed to read frame header: EOF

coder/websocket#218
@nhooyr
Copy link
Contributor

nhooyr commented Apr 6, 2020

Best to just disable compression for now until I figure out what's going on here.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 6, 2020

To clarify I mean I'm going to turn off compression by default.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 14, 2020

Confirmed reproduction btw with the new chat example. It's proven handy already.

@nhooyr nhooyr closed this as completed in 4edcada Apr 14, 2020
@gharia
Copy link

gharia commented Nov 29, 2021

@nhooyr we are facing a similar issue with Safari version 15.1 (17612.2.9.1.20) in Monterey OS if compression is enabled

wsc, err := websocket.Accept(c.Response(), c.Request(), &websocket.AcceptOptions{
		CompressionMode:    websocket.CompressionContextTakeover,	
		InsecureSkipVerify: true,
})

We get below error is mentioned Safari,

The operation couldn’t be completed. (kNWErrorDomainPOSIX error 100 - Protocol error)

If we disable compression, it works fine. Is there any suggestion for using it with compression enabled?

@Plasmatium
Copy link

Now safari are using x-webkit-deflate-frame instead of permessage-deflate, so just comment case: x-webkit-deflate-frame is not working now.

I'm suffering this issue only under k8s + istio + gin. While running the same code on my macbook, this problem disappeared.

The log shows the error comes from deeply in frame.go -> readFrameHeader at line 51, as EOF.
Is there a way to mirror the c.Request.Body reader to a buffer, so that I can log more about what exactly data have been read

@Plasmatium
Copy link

/reopen

@nhooyr nhooyr reopened this Feb 18, 2022
@nhooyr
Copy link
Contributor

nhooyr commented Feb 18, 2022

Sorry I don't currently have the time to help debug, in the middle of closing a home, working a new job and moving.

But I've reopened this issue to investigate at some future date.

mihaip added a commit to tailscale/tailscale that referenced this issue Sep 15, 2022
The data that we send over WebSockets is encrypted and thus not
compressible. Additionally, Safari has a broken implementation of compression
(see coder/websocket#218) that makes enabling it actively harmful.

Fixes tailscale/corp#6943

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
mihaip added a commit to tailscale/tailscale that referenced this issue Sep 15, 2022
The data that we send over WebSockets is encrypted and thus not
compressible. Additionally, Safari has a broken implementation of compression
(see coder/websocket#218) that makes enabling it actively harmful.

Fixes tailscale/corp#6943

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
showurl added a commit to cherish-chat/xxim-server that referenced this issue Jan 13, 2023
@erikdubbelboer
Copy link

You can disable this conditionally only for Safari:

acceptOptions := &websocket.AcceptOptions{
	// ...
}

userAgentLower := strings.ToLower(r.Header.Get("User-Agent"))
isSafari := strings.Contains(userAgentLower, "safari") && !strings.Contains(userAgentLower, "chrome") && !strings.Contains(userAgentLower, "android")

if isSafari {
	acceptOptions.CompressionMode = websocket.CompressionDisabled
}

conn, err := websocket.Accept(w, r, acceptOptions)

See: poki/netlib#28

@nhooyr
Copy link
Contributor

nhooyr commented Sep 28, 2023

I would suggest not using compression at all. It's proven too unreliable even without Safari. See #391

Will be disabled by default in #256

koenbollen added a commit to poki/netlib that referenced this issue Oct 4, 2023
(also snuck in the disabling of websocket compression, see coder/websocket#218 (comment))
koenbollen added a commit to poki/netlib that referenced this issue Oct 11, 2023
Currently all lobbies stay in the database even if they are empty. This
change introduces a simple timer that deletes day old empty lobbies
every 30 minutes.

This is needed for better lobby listing (filters, private etc) in the
future.

Also snuck in the disabling of websocket compression, see
coder/websocket#218 (comment)
@nhooyr
Copy link
Contributor

nhooyr commented Oct 13, 2023

Going to close as the commit in dev exists to disable it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants