Bug 240311

Summary: REGRESSION (STP): WebSocket.bufferedAmount does not get updated in some cases
Product: WebKit Reporter: Roberto D'Auria <roberto>
Component: WebCore Misc.Assignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ews-watchlist, toyoshim, webkit-bug-importer, youennf, yutak
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: All   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Roberto D'Auria
Reported 2022-05-11 10:12:18 PDT
This is probably related to the fix implemented for https://bugs.webkit.org/show_bug.cgi?id=235707. Context: The M-Lab team maintains a WebSocket-based network measurement client, https://speed.measurementlab.net/ (and elsewhere) which measures the maximum TCP throughput of the user’s Internet connection. The upload throughput measurement sends repeated messages over a WebSocket connection for 10 seconds. It uses bufferedAmount to monitor the backlog of untransmitted data. The goal is to always have some data ready and waiting to be transmitted but not so much data that the buffer overflows or takes too long to drain at the end of the test. Overview: We are testing the patch above, which makes the update of bufferedAmount synchronous, on the latest (version 144) Safari Technology Preview, and it looks like it might have introduced a significant regression. On some machines (more details below) bufferedAmount stops being updated after just a few writes, so our client stops sending data. Is it possible that bufferedAmount is not being decremented correctly in all the possible places in the code, or the data isn't actually sent over the network anymore for some reason? Additional details: The testing was done with the the code at https://github.com/m-lab/ndt7-js/blob/main/src/ndt7-upload-worker.js#L78 with added debugging logs around sock.send(): console.log("desired buffer: " + desiredBuffer + " - bufferedAmount: " + sock.bufferedAmount); if (sock.bufferedAmount < desiredBuffer) { console.log("Sending " + data.length + " bytes"); sock.send(data); console.log("Sent"); total += data.length; } The very first calls to send() work normally. After flushing the buffer at least once (I can see bufferedAmount going down to zero), bufferedAmount reaches the desiredBuffer and then it’s never decremented again for the whole duration of the measurement -- 10 seconds. From the logs attached you can also see that NumBytes (total number of bytes sent) stays at 64KB and does not increase anymore. This code works as intended on Safari stable (except for the sporadic crashes that caused us to report the original bug linked above) and every other major browser. This new issue affects the speedtest at https://speed.measurementlab.net and Google's own speedtest client in search pages (OneBox), both based on websockets and the ndt7 protocol. Both of them report extremely low upload rates due to no new data being sent after the initial few milliseconds, matching what we observed in our testing. We believe that including that change as-is in the next Safari release will cause problems to all the integrations of the ndt7 speedtest protocol and everyone else relying on bufferedAmount to measure the backlog. We have noticed differences in behavior depending on the hardware this was tested on: 2019 iMac 8-Core Intel i9, 64GB ram, MacOS Monterey: Works as intended 2019 MacBook Pro 8-core Intel i9 16GB ram, MacOS Monterey: Works as intended 2018 MacBook Pro 4-core Intel i7 16GB ram, MacOS Big Sur: Never works, stops sending after a few milliseconds as described above 2020 M1 MacBook Air, 16GB ram, MacOS Monterey: Works as intended when the tab running the measurement is in the foreground, stops sending when the tab is in the background. This is the most puzzling finding, but it could be replicated very consistently Each test was run on a different network connection. Link speed does not seem to make a difference. Running the sender loop on the main thread vs a dedicated Web Worker does not seem to make any difference either. Console logs, with comments added, collected on the 2018 MacBook Pro running Safari Technology Preview 144: [Log] Upload started [Log] desired buffer: 57344 - bufferedAmount: 0 First calls to send(): [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 8192 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 16384 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 24576 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 32768 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 40960 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 49152 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 57344 bufferedAmount is decremented for the first time here: [Log] desired buffer: 57344 - bufferedAmount: 24600 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 0 After this point, bufferedAmount is not decremented anymore, as if no data is being sent over the network: [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 8192 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 16384 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 24576 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 32768 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 40960 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 49152 [Log] Sending 8192 bytes [Log] Sent [Log] desired buffer: 57344 - bufferedAmount: 57344 From here on the client stops sending, since bufferedAmount == desiredBuffer. As a result, no new send() happens until the end of the 10-second measurement interval. What follows are just periodic measurement results which continue to decrease since no new data is sent: [Log] Server-side measurement: \'96 \{ConnectionInfo: \{Client: "<redacted>:52361", Server: "149.20.5.88:443", UUID: "<redacted>"\}, BBRInfo: Object, TCPInfo: Object\} \{ConnectionInfo: \{Client: "<redacted>:52361", Server: "149.20.5.88:443", UUID: "<redacted>"\}, BBRInfo: Object, TCPInfo: Object\}Object\ [Log] desired buffer: 57344 - bufferedAmount: 57344 [Log] Client-side measurement: \'96 \{ElapsedTime: 0.253, NumBytes: 65536, MeanClientMbps: 2.072284584980237\} [Log] desired buffer: 57344 - bufferedAmount: 57344 [Log] Client-side measurement: \'96 \{ElapsedTime: 0.507, NumBytes: 65536, MeanClientMbps: 1.0340986193293884\} [Log] desired buffer: 57344 - bufferedAmount: 57344 [Log] Server-side measurement: \'96 \{ConnectionInfo: \{Client: "<redacted>:52361", Server: "149.20.5.88:443", UUID: "<redacted>"\}, BBRInfo: Object, TCPInfo: Object\} \{ConnectionInfo: \{Client: "<redacted>:52361", Server: "149.20.5.88:443", UUID: "<redacted>"\}, BBRInfo: Object, TCPInfo: Object\}Object\ [Log] desired buffer: 57344 - bufferedAmount: 57344 [Log] Client-side measurement: \'96 \{ElapsedTime: 0.759, NumBytes: 65536, MeanClientMbps: 0.6907615283267456\} [Log] desired buffer: 57344 - bufferedAmount: 57344 [Log] Server-side measurement: \'96 \{ConnectionInfo: \{Client: "<redacted>:52361", Server: "149.20.5.88:443", UUID: "<redacted>"\}, BBRInfo: Object, TCPInfo: Object\} (... snip ...)
Attachments
Patch (4.14 KB, patch)
2022-05-13 16:34 PDT, Alex Christensen
no flags
Patch (4.31 KB, patch)
2022-05-16 09:59 PDT, Alex Christensen
no flags
Patch (5.24 KB, patch)
2022-05-16 11:21 PDT, Alex Christensen
no flags
Patch (17.47 KB, patch)
2022-05-17 12:55 PDT, Alex Christensen
no flags
Radar WebKit Bug Importer
Comment 1 2022-05-11 10:16:12 PDT
Alex Christensen
Comment 2 2022-05-13 16:34:23 PDT
Alex Christensen
Comment 3 2022-05-13 16:35:41 PDT
Thanks for the report, Roberto. Unfortunately, this means I need to restore the status quo on Big Sur, but fortunately newer OSes have been fixed.
Roberto D'Auria
Comment 4 2022-05-16 05:00:39 PDT
Thank you for looking into this! Have you had a chance to verify the background vs foreground behavior on MacOS Monterey, too? It behaves the same as on Big Sur, but only when the tab running the send loop does not have focus.
Alex Christensen
Comment 5 2022-05-16 09:59:09 PDT
Alex Christensen
Comment 6 2022-05-16 10:56:54 PDT
Roberto, I can't reproduce the background vs foreground issue on Monterey on my m1 MacBook Air with 16GB of ram. Maybe I'm misunderstanding something, but I'm going to https://speed.measurementlab.net/#/ then starting the speed test and switching to another tab or minimizing Safari, and I see no difference. It's definitely a different issue than the one this bug is fixing, though.
Alex Christensen
Comment 7 2022-05-16 11:21:00 PDT
Roberto D'Auria
Comment 8 2022-05-16 12:41:50 PDT
The procedure is the correct one, but we're having some trouble replicating it ourselves right now. I'll open a separate issue if it happens again and I can figure out how to replicate it more consistently. It's also possible that it can happen on Safari stable too, but rarely enough that we never noticed. Thanks!
EWS
Comment 9 2022-05-16 13:50:37 PDT
Found 16 new test failures: imported/w3c/web-platform-tests/websockets/Send-65K-data.any.html, imported/w3c/web-platform-tests/websockets/Send-binary-65K-arraybuffer.any.html, imported/w3c/web-platform-tests/websockets/Send-binary-65K-arraybuffer.any.worker.html, imported/w3c/web-platform-tests/websockets/Send-binary-arraybuffer.any.html, imported/w3c/web-platform-tests/websockets/Send-binary-arraybuffer.any.worker.html, imported/w3c/web-platform-tests/websockets/Send-data.any.html, imported/w3c/web-platform-tests/websockets/Send-data.any.worker.html, imported/w3c/web-platform-tests/websockets/Send-paired-surrogates.any.html, imported/w3c/web-platform-tests/websockets/Send-paired-surrogates.any.worker.html, imported/w3c/web-platform-tests/websockets/Send-unicode-data.any.worker.html ...
Alex Christensen
Comment 10 2022-05-17 12:55:49 PDT
EWS
Comment 11 2022-05-18 11:40:18 PDT
Committed r294413 (250705@main): <https://commits.webkit.org/250705@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459512 [details].
Note You need to log in before you can comment on or make changes to this bug.