Repository navigation
Conversation
The Linux response writers stream the body to WebKit through a pipe created with Pipe2(p, 0). Without O_CLOEXEC, any child process forked while a response is in flight (os/exec, a helper the app spawns) inherits the write end. The handler returns and closes its copy, but WebKit only sees EOF once every copy is closed, so the body (and the fetch() behind a binding call) stalls until that child exits, even though headers arrived on time. Create the pipe with O_CLOEXEC on both ends. The read end is only consumed in-process by the GUnixInputStream, so nothing relies on inheriting it. Applies to both the GTK4 and GTK3 writers.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughBoth Linux response pipe implementations now create descriptors with close-on-exec enabled. A Linux test checks that both descriptors have the ChangesLinux response pipe descriptors
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change stops child processes from inheriting the response pipe, which removes the reported stalls in response bodies. No merge-blocking risk was found. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change prevents unintended response-pipe access after child processes execute another program, while preserving the existing response delivery and cleanup behavior. No introduced or worsened security concern was identified in the reviewed change. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. A rabbit checks each pipe end bright, Comment |
1 participant
Description
On Linux, the asset server's response writers (
responsewriter_linux.gofor GTK4,responsewriter_linux_gtk3.gofor GTK3) stream the response body to WebKit through a pipe created withsyscall.Pipe2(p, 0). Because the pipe has noO_CLOEXEC, any child process forked while a response is in flight inherits the write end. This includesos/exec, a helper the app restarts, and so on. The handler returns and closes its own copy, but WebKit only sees EOF on the body once every copy of the write end is closed. So the body, and thefetch()behind a binding call, stalls until that unrelated child exits, even though the response headers arrived on time.In a real app this showed up as a binding call (
GetMessages) whose result never reached JS. The Go method had returned in ~60 ms, but the promise resolved 9–17 s later, exactly when the app killed a helper process it had (re)started at the same moment. Instrumenting/procconfirmed it: during every stall, the helper held the write end of the response pipe, and the body completed the instant the helper died.This PR creates the pipe with
O_CLOEXECon both ends. The read end is only consumed in-process by theGUnixInputStream, so nothing relies on it being inherited. The original comment ("we especially don't want to have the FD_CLOEXEC") gave no reason, and I could not find one. It is replaced by an explanation of why the flag is required.No existing issue; happy to open one if preferred.
Type of change
How Has This Been Tested?
Unit test:
TestPipeIsCloseOnExecchecksFD_CLOEXECon both ends ofpipe(). It fails onmasterand passes with this change, on both the default (GTK4) and-tags gtk3builds.go test -race ./internal/assetserver/...passes on both builds.End-to-end repro (minimal app, no frontend framework): a bound method returns an N KiB string. While it returns, a goroutine starts a few short-lived children, simulating an app that spawns helpers concurrently:
The page calls
Call.ByName("main.Repro.Big", kb)with an idle gap between calls and measures the time until the promise resolves. 6 calls per size (1, 16, 64, 256, 512, 1024 KiB), 36 calls per run:master-tags gtk3)Without the concurrent
exec, both builds show 0 stalls. That is expected, since the bug needs a fork while the pipe is open.Test Configuration
Checklist:
Summary by CodeRabbit