fix(linux): prevent browser teardown from disabling WebGL - #166
Conversation
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughLinux CEF close handling now allows native closing to proceed in specified cases. Native-view release detaches the embed host, and the close callback notifies Linux after unregistering the browser. ChangesLinux CEF close lifecycle
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CEF
participant do_close
participant CefBrowser_drop
participant linux_x11_release_native_view
participant X11_server
participant on_before_close
participant browser_native_close_finished
CEF->>do_close: Request close
do_close-->>CEF: Return 0 for applicable Linux closes
CefBrowser_drop->>linux_x11_release_native_view: Release native view
linux_x11_release_native_view->>X11_server: Unmap and reparent embed host
linux_x11_release_native_view->>X11_server: Wait for server round-trip
CEF->>CEF: Close X11 child
CEF->>on_before_close: Finish native close
on_before_close->>browser_native_close_finished: Notify native close finished
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No confirmed issue remains that should block merging. The native handle’s value during the close callback has not been verified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change addresses a browser-close race that could disable WebGL across tabs. The normal close path preserves per-browser window identity, but cleanup now depends on completion of an asynchronous callback. No new attacker-controlled access path or verified security finding was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.98.1)Clippy execution failed 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. Comment |
Closing an embedded browser could destroy its X11 parent while Chromium was still creating a GPU surface. On the affected Linux machine, ANGLE crashed in
WindowSurfaceGLX::initializeafterXGetWindowAttributesreturned no visual; Chromium recorded three GPU crashes and disabled WebGL for every tab.Keep the embed host alive but unmapped and reparented away from its GPUI owner during close. Allow CEF to perform its native X11 child close, then release the embed host from
on_before_close. This fixes the window lifetime in the desktop CEF adapter without changing GPU flags or introducing software rendering.Validation:
cargo check --bin ghostex-gpui --offlinepassed on Linux (existing warnings).git diff --checkpassed.Note
Fix Linux browser teardown destroying WebGL embed host too early
release_native_viewnow only unmaps and reparents the embed host to the X11 root window instead of destroying it (linux_x11.rs)browser_native_close_finished, which destroys and flushes the host after CEF finishes closing the browserLifeSpanHandler::do_closenow marks app-initiated closes as handled on Linux, andon_before_closecalls the new cleanup after unregistering the native view (browser_handlers.rs)on_before_closefiring; checkbrowser_native_close_finishedand thedo_closehandled-close branch if browser windows lingerMacroscope summarized 8e97109.
Summary by CodeRabbit