Deal with safari fragmented pong data

[DeqingSun]

I have a kiosk iPad application that has to use Safari. The iPad app works with this library in 2019, but not with the new iOS and the new library. The fragmented data, especially pong, causes data corruption.

Pong data in Chrome
8A 80 8B 9B 7E 64
Pong data in Safari
8A 80
46 DC 9F 25

This commit addresses the pointer corruption that occurs when Safari sends a Pong. Without this change, the library will use the mask as the beginning of a new data frame and misbehave.

I used _pinfo.masked as a counter to minimize change of the code.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mathieucarbou]

@DeqingSun thank you for the PR.

Indeed since the new iOS and macOS updates safari does not work anymore with a few projects using websocket but not all. It depends highly on the UI used.

I am wondering if this fix will solve this issue:

hoylabs/OpenDTU-OnBattery#2331 (reply in thread)

i will test it.

[Copilot]

Pull request overview

This PR addresses a WebSocket fragmentation issue that occurs when Safari sends pong frames (and potentially other frames) split across multiple packets. The fix modifies the mask reading logic to handle cases where the 4-byte WebSocket mask arrives in fragments rather than all at once.

Key Changes:

• Repurposed _pinfo.masked from a boolean flag to a counter that tracks mask reading progress (0=unmasked, 1-4=partial, 5=complete)
• Moved mask reading logic outside the initial frame parsing block to support resuming mask reads across multiple data packets
• Added state machine logic to read mask bytes incrementally and wait for more data when needed

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mathieucarbou]

@DeqingSun : I tested your PR as-is with our WebSocket example (that I updated for that). We can correctly see the ping/pong mechanism. Also, refreshing the page in Safari works (closes the previous ws connection and opens a new one) and closing the browser also disconnects.

⚠️ But 2 issues:

• _pinfo.masked is exposed to the client code, so this PR does not keep backward compatibility anymore if someone was using _pinfo.masked == 1.

• I tested your PR with an app using ESP-DASH on Safari and the WS connection does not work while it works with 3.9.2

I tested the copilot suggestion to restore the boolean state after defragmentation - the code seemed OK to me - but without luck - it leads to a MCU crash when closing the browser window.

There is also this safari bug to handle where it is sending some data when ws.close() is called on client side (this was a user report some time ago).

I will mark this PR as draft for now - it still require some work.

[mathieucarbou]

@DeqingSun @willmmiles @me-no-dev @vortigont : I added a second commit to fix.

• ✅ Code is now backward compatible in rgards to the mask boolean (1 or 0)
• ✅ The code supports fragmented pong replies
• ✅ Tested with ESP-DASH and binary websocket
• ✅ Tested with our examples

Connected clients: 1 / 1 total
Free heap: 228548
[ 97181][W][AsyncWebSocket.cpp:521] _onData(): WS[7]: _onData: 11
[ 97187][W][AsyncWebSocket.cpp:522] _onData(): WS[7]: _status = 1
[ 97193][W][AsyncWebSocket.cpp:523] _onData(): WS[7]: _pinfo: index: 0, final: 1, opcode: 1, masked: 1, len: 5
[ 97203][W][AsyncWebSocket.cpp:576] _onData(): WS[7]: mask complete
[ 97209][W][AsyncWebSocket.cpp:586] _onData(): WS[7]: _processing data: datalen=5, plen=5
index: 0, len: 5, final: 1, opcode: 1
ws text: fdfdf
[ 97225][W][AsyncWebSocket.cpp:521] _onData(): WS[7]: _onData: 2
[ 97231][W][AsyncWebSocket.cpp:522] _onData(): WS[7]: _status = 1
[ 97237][W][AsyncWebSocket.cpp:523] _onData(): WS[7]: _pinfo: index: 0, final: 1, opcode: 10, masked: 1, len: 0
[ 97247][W][AsyncWebSocket.cpp:569] _onData(): WS[7]: waiting for more mask data: read=0/4 <=== FRAGMENTED
[ 97255][W][AsyncWebSocket.cpp:576] _onData(): WS[7]: mask complete
[ 97261][W][AsyncWebSocket.cpp:586] _onData(): WS[7]: _processing data: datalen=0, plen=0
ws pong

[mathieucarbou]

@DeqingSun : please test again with your setup and let us know! I tested on my side and it works.

[DeqingSun]

Yes, your improvement is confirmed, working on my setup.

[willmmiles]

Seems like it will work for the specific case. I cannot personally test this as I do not own any Apple hardware.

I'm happy to accept this as a quick fix; but broadly I'd prefer a general solution for arbitrary fragmentation over the whole function. (At no point is there any guarantee of how websocket traffic is divided into TCP packets - that's very much implementation dependent, right up to fragmentation at the physical interface.) It's somewhat ironic that we've ended up with a synchronous protocol decode function in a library named "AsyncWebServer"... ;) Helpfully we've got a _pstate variable already declared that could be expanded to a full state machine; alternately, if the current synchronous implementation is preferred, we could implement either a deferred-processing buffer or a continuation-passing style.

[mathieucarbou]

Seems like it will work for the specific case. I cannot personally test this as I do not own any Apple hardware.

I'm happy to accept this as a quick fix; but broadly I'd prefer a general solution for arbitrary fragmentation over the whole function. (At no point is there any guarantee of how websocket traffic is divided into TCP packets - that's very much implementation dependent, right up to fragmentation at the physical interface.) It's somewhat ironic that we've ended up with a synchronous protocol decode function in a library named "AsyncWebServer"... ;) Helpfully we've got a _pstate variable already declared that could be expanded to a full state machine; alternately, if the current synchronous implementation is preferred, we could implement either a deferred-processing buffer or a continuation-passing style.

Yes this PR adds fragmentation support for the control frames like pong (mask) which was not supported before.

But more generally defragmenting to support any kind of incoming buffer len and be able to progressively reconstruct the object is out of this scope and a lot more work.

That's funny that this global defragmentation was never needed since the lib exist... Maybe because we can be sure that the websocket info object will always fit into a minimal tcp frame 🤷 ?

[vortigont]

Sorry, missed this topic.
Just wanted to add that this problem was reworked from scratch in #312. Newer implementation is able to reassemble any kind of messages spitted into multiple TCP packets.
But this mask split this something very strange. I did not get from the description what is split there - the header bytes and the whole mask itself or the the mask bytes arrives in different packets?
Can anybody provide me a TCP capture dump of that example?
I'll sync PR with up to date main code for anyone willing to test.

P.S. Sorry, I do not have any apple stuff in possession (lucky me :) )
Apple is on his own vibes as usual... :(

[mathieucarbou]

Sorry, missed this topic. Just wanted to add that this problem was reworked from scratch in #312. Newer implementation is able to reassemble any kind of messages spitted into multiple TCP packets. But this mask split this something very strange. I did not get from the description what is split there - the header bytes and the whole mask itself or the the mask bytes arrives in different packets? Can anybody provide me a TCP capture dump of that example? I'll sync PR with up to date main code for anyone willing to test.

P.S. Sorry, I do not have any apple stuff in possession (lucky me :) ) Apple is on his own vibes as usual... :(

There is no way to have a backward compatible api with your changes, even merly ? If this is just about adapting a little that could be ok.

[vortigont]

No backward compat, sorry. The new API is more feature reach and implies a bit different approaches to use.
But I've implemented it in other class names, so applications could be adopted when whenever it it feasible for the user. We could just have both implementations available side by side.

[mathieucarbou]

No backward compat, sorry. The new API is more feature reach and implies a bit different approaches to use. But I've implemented it in other class names, so applications could be adopted when whenever it it feasible for the user. We could just have both implementations available side by side.

OK! Maybe that could be an option then.... We need to discuss it.