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

Safari websocket compression is broken #731

Closed
mohamedmansour opened this issue Oct 21, 2021 · 8 comments
Closed

Safari websocket compression is broken #731

mohamedmansour opened this issue Oct 21, 2021 · 8 comments
Labels

Comments

@mohamedmansour
Copy link

Using go v1.17
Using gorilla websocket v1.4.2

iOS Safari 15 enabled by default for all users "NSURLSession WebSocket", by enabling this, it broke my application at (https://watchtheburn.com) if you have a Mac, you can inspect and see it not working (on an iOS device).

Going into Experimental WebKit Features and disabling "NSURLSession WebSocket" it works again.

@ejain opened this up and closed it for #696 Any ideas how I can resolve it?

@ghost
Copy link

ghost commented Oct 21, 2021

Is the problem that iOS 15 does not support compressed fragmented messages?

If that is the problem, there are ways for the application to work around the bug in iOS.

@mohamedmansour
Copy link
Author

@mamjsn is there a way to test it?

@ghost
Copy link

ghost commented Oct 21, 2021

There are a couple of options.

Disable compression

Delete this line.

Prevent fragmentation

EDIT: The code below does not prevent fragmentation when compression is used.

Replace this section of code with:

 w, err := c.conn.WriteMessage(websocket.TextMessage, message)
 if err != nil {
     return
 }

WriteMessage does not fragment message. This change removes the optimization to send multiple application messages in a single websocket message. You should check to see if that optimization actually kicks in.

Here's how to retain the optimization at the cost of allocating memory:

  m := message[0:len(message):len(message)] // force new allocation on append
  n := len(c.send)
  for i := 0; i < n; i++ {
      m = append(m, '\n')
      m = append(m, <-c.send)
 }
 w, err := c.conn.WriteMessage(websocket.TextMessage, m)
 if err != nil {
       return
 }

@mohamedmansour
Copy link
Author

mohamedmansour commented Oct 27, 2021

@mamjsn how did you figure this out! thank you for taking your time and looking at the source code of my app!

I verified that removing compression works on iOS 15.
mohamedmansour/ethereum-burn-stats@d282823

I tried re-enabling compression, but it doesn't work, so disabling it instead. Unless I am doing it wrong.
mohamedmansour/ethereum-burn-stats@ceb5e2e

Thank you!

@nicks
Copy link

nicks commented Oct 27, 2021

we're hitting this too in MacOS Monterey 😭 tilt-dev/tilt#4746

thanks @mamjsn for the workarounds above.

how hard would it be to have gorilla/websocket detect this scenario and workaround it? Like maybe we could check for Sec-WebSocket-Extensions: permessage-deflate and User-Agent: Safari? would be happy to try to put together a PR if no one is opposed to browser-specific workarounds.

@ghost
Copy link

ghost commented Oct 27, 2021

@mohamedmansour: Based on Safari's past use of x-webkit-deflate-frame extension, I am guessing that Safari compresses frames instead of messages. The permessage-deflate extension compresses entire messages.

Thank you for trying the code to prevent message fragmentation. It turns out that my suggestion does not prevent fragmentation when compression is used. I apologize for the wasted effort there.

There's not easy path to disabling fragmentation: the code changes are not trivial, and the package must allocate memory to hold the entire compressed message in memory.

Here's another possible workaround for applications: Compressed messages do not fragment when the compressed message size plus websocket overhead is smaller than the write buffer size. Set the write buffer size to a value larger than any expected message. Use buffer pools to mitigate memory allocation costs.

@garyburd
Copy link
Contributor

garyburd commented Jan 19, 2022

Some versions of Safari do not support compressed fragmented messages. Work around the Safari bug by disabling compression.

Other discussions on the topic:

@garyburd
Copy link
Contributor

Applications should disable compression when working with buggy Safari clients.

There are reports that the problem is fixed in recent versions of Safari.

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

3 participants