Skip to content

Slack is not recreating its web socket in case of network process crash #47063

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 25, 2025
Merged

Slack is not recreating its web socket in case of network process crash #47063

merged 1 commit into from
Jun 25, 2025

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Jun 23, 2025 •
edited by webkit-early-warning-system
Loading

5824211

Slack is not recreating its web socket in case of network process crash
rdar://153243346
https://bugs.webkit.org/show_bug.cgi?id=294842

Reviewed by Anne van Kesteren.

Slack is reacting to onclose but not onerror.
In case of network process crash, we were firing an error event but not a close event.
We now fire a close event, which gets us closer to spec.
We add a FAIL expectation to web-socket-tls test since, in debug, network process may crash, which will trigger a close event now.

* LayoutTests/http/tests/websocket/tests/hybi/network-process-crash-error-expected.txt:
* LayoutTests/http/tests/websocket/tests/hybi/network-process-crash-error.html:
* Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:
(WebKit::WebSocketChannel::networkProcessCrashed):
* Tools/TestWebKitAPI/glib/TestExpectations.json:

Canonical link: https://commits.webkit.org/296607@main

8d1d74a

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf requested a review from cdumez as a code owner June 23, 2025 15:12
@youennf youennf self-assigned this Jun 23, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 23, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 9435aef)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ❌ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf requested review from achristensen07 and annevk June 23, 2025 15:14
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 23, 2025
@youennf youennf force-pushed the eng/Slack-is-not-recreating-its-web-socket-in-case-of-network-process-crash branch from 9435aef to 8d1d74a Compare June 24, 2025 05:46
@youennf youennf requested a review from a team as a code owner June 24, 2025 05:46
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 24, 2025 •
edited
Loading

EWS run on current version of this PR (hash 8d1d74a)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jun 25, 2025
@youennf
Slack is not recreating its web socket in case of network process crash
5824211 
rdar://153243346
https://bugs.webkit.org/show_bug.cgi?id=294842

Reviewed by Anne van Kesteren.

Slack is reacting to onclose but not onerror.
In case of network process crash, we were firing an error event but not a close event.
We now fire a close event, which gets us closer to spec.
We add a FAIL expectation to web-socket-tls test since, in debug, network process may crash, which will trigger a close event now.

* LayoutTests/http/tests/websocket/tests/hybi/network-process-crash-error-expected.txt:
* LayoutTests/http/tests/websocket/tests/hybi/network-process-crash-error.html:
* Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:
(WebKit::WebSocketChannel::networkProcessCrashed):
* Tools/TestWebKitAPI/glib/TestExpectations.json:

Canonical link: https://commits.webkit.org/296607@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Slack-is-not-recreating-its-web-socket-in-case-of-network-process-crash branch from 8d1d74a to 5824211 Compare June 25, 2025 07:32
@webkit-commit-queue
Copy link
Collaborator

Committed 296607@main (5824211): https://commits.webkit.org/296607@main

Reviewed commits have been landed. Closing PR #47063 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 5824211 into WebKit:main Jun 25, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 25, 2025
@mcatanzaro
Copy link
Contributor

Nice, this will probably fix more than just Slack.

@aperezdc
Copy link
Contributor

Backported into webkitglib/2.48 as commit a7ddd04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@achristensen07 achristensen07 Awaiting requested review from achristensen07

Assignees

@youennf youennf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@youennf @webkit-early-warning-system @webkit-commit-queue @mcatanzaro @aperezdc @annevk @webkit-ews-buildbot