zed icon indicating copy to clipboard operation
zed copied to clipboard

gpui: Fix `update_window` to `borrow_mut` will crash on Windows

Open huacnlee opened this issue 11 months ago • 14 comments

Release Notes:

  • N/A

When we use window_handle to draw WebView on Windows, this will crash by:

This error caused by when used WebView2.

thread 'main' panicked at crates\gpui\src\app\async_context.rs:91:28:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at library\core\src\panicking.rs:221:5:
panic in a function that cannot unwind

Try this https://github.com/tauri-apps/wry/pull/1383 on Windows can replay the crash.

In fact, we had done a similar fix around August last year, but we used the unsafe method to avoid crashes in that version, we felt that it was not a good change, so we do not make PR.

Today @sunli829 thought about it again and changed the method. Now using try_borrow_mut is similar to the previous borrow_mut.

https://github.com/zed-industries/zed/blob/691de6b4b36d3ada91a1e238904b065eec454188/crates/gpui/src/app.rs#L70-L78

I have tested to start Zed by those changes, it is looks no problem.

huacnlee avatar Feb 10 '25 08:02 huacnlee

We require contributors to sign our Contributor License Agreement, and we don't have @sunli829 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

cla-bot[bot] avatar Feb 10 '25 08:02 cla-bot[bot]

@cla-bot check

huacnlee avatar Feb 10 '25 10:02 huacnlee

The cla-bot has been summoned, and re-checked this pull request!

cla-bot[bot] avatar Feb 10 '25 10:02 cla-bot[bot]

Hello. I would be very excited to see the WebView support in Zed. Also because it would expose a Canvas, allowing the port of some mroe sophisticated VSCode extensions. Is the work still onging?

mocenigo avatar Mar 08 '25 16:03 mocenigo

@JunkuiZhang @SomeoneToIgnore

This PR is important to provided us ability to use WebView or other depends raw_window_handle stuffs for works on Windows.

We are already useing this in our project and it works well in a long term.

Sorry, I have to push for this, because this PR is waited a bit long time.

huacnlee avatar Apr 08 '25 03:04 huacnlee

Really sorry about this, I nearly forgot about this PR

While your PR mentions fixing a Windows issue, I tested it on macOS and it's failing there too with this error:

thread 'main' panicked at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:3441:9:
null pointer dereference occurred

Looks like this bug might need some more investigation.

JunkuiZhang avatar Apr 08 '25 06:04 JunkuiZhang

How to do you start up? I have checked it again by start Zed in development cargo run, There is no error.

image

Please submit the full of error stack.

huacnlee avatar Apr 08 '25 08:04 huacnlee

I’m talking about the example you provided and attempted to fix https://github.com/tauri-apps/wry/pull/1383

Simply run that example and it reliably crashes on macOS

JunkuiZhang avatar Apr 08 '25 16:04 JunkuiZhang

I'll check it again tomorrow morning.

But we've been using this change to include WebView in our apps for a long time, and macOS is correct. It may be some other issue, such as what updates wry has recently made.

huacnlee avatar Apr 08 '25 16:04 huacnlee

How to reproduce the crash

  • Clone the repo: https://github.com/JunkuiZhang/wry
  • git switch test
  • cargo run --example gpui -- --nocapture

Full backtrace

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 11.90s
     Running `target/debug/examples/gpui --nocapture`
1
2

thread 'main' panicked at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:3441:9:
null pointer dereference occurred
stack backtrace:
   0: rust_begin_unwind
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/std/src/panicking.rs:695:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/core/src/panicking.rs:117:22
   2: core::panicking::panic_nounwind_fmt
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/core/src/intrinsics/mod.rs:3886:9
   3: core::panicking::panic_null_pointer_dereference
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/core/src/panicking.rs:304:5
   4: <*mut objc::runtime::Object as cocoa::appkit::NSScreen>::backingScaleFactor
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:3441:9
   5: gpui::platform::mac::window::get_scale_factor
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:1199:9
   6: gpui::platform::mac::window::MacWindowState::scale_factor
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:482:9
   7: gpui::platform::mac::window::set_frame_size
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:1702:24
   8: <unknown>
   9: <unknown>
  10: <(A,) as objc::message::MessageArguments>::invoke
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:128:17
  11: objc::message::platform::send_unverified
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/apple/mod.rs:27:9
  12: objc::message::send_message
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:178:5
  13: <*mut objc::runtime::Object as cocoa::appkit::NSWindow>::setContentView_
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:1445:9
  14: gpui::platform::mac::window::MacWindow::open
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:689:13
  15: <gpui::platform::mac::platform::MacPlatform as gpui::platform::Platform>::open_window
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/platform.rs:583:21
  16: gpui::window::Window::new
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/window.rs:724:35
  17: gpui::app::App::open_window::{{closure}}
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:608:19
  18: gpui::app::App::update
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:426:22
  19: gpui::app::App::open_window
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:605:9
  20: gpui::main::{{closure}}
             at ./examples/gpui.rs:226:18
  21: gpui::app::Application::run::{{closure}}
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:166:13
  22: core::ops::function::FnOnce::call_once{{vtable.shim}}
             at /Users/zhangjunkui/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
  23: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /Users/zhangjunkui/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1976:9
  24: gpui::platform::mac::platform::did_finish_launching
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/platform.rs:1334:13
  25: <unknown>
  26: <unknown>
  27: <unknown>
  28: <unknown>
  29: <unknown>
  30: <unknown>
  31: <unknown>
  32: <unknown>
  33: <unknown>
  34: <unknown>
  35: <unknown>
  36: <unknown>
  37: <unknown>
  38: <unknown>
  39: <unknown>
  40: <unknown>
  41: <unknown>
  42: <unknown>
  43: <() as objc::message::MessageArguments>::invoke
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:128:17
  44: objc::message::platform::send_unverified
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/apple/mod.rs:27:9
  45: objc::message::send_message
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:178:5
  46: <*mut objc::runtime::Object as cocoa::appkit::NSApplication>::run
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:628:9
  47: <gpui::platform::mac::platform::MacPlatform as gpui::platform::Platform>::run
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/platform.rs:454:13
  48: gpui::app::Application::run
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:164:9
  49: gpui::main
             at ./examples/gpui.rs:217:3
  50: core::ops::function::FnOnce::call_once
             at /Users/zhangjunkui/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.
[1]    95817 abort      cargo run --example gpui -- --nocapture    Finished `dev` profile [unoptimized + debuginfo] target(s) in 11.90s
     Running `target/debug/examples/gpui --nocapture`
1
2

thread 'main' panicked at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:3441:9:
null pointer dereference occurred
stack backtrace:
   0: rust_begin_unwind
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/std/src/panicking.rs:695:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/core/src/panicking.rs:117:22
   2: core::panicking::panic_nounwind_fmt
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/core/src/intrinsics/mod.rs:3886:9
   3: core::panicking::panic_null_pointer_dereference
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/core/src/panicking.rs:304:5
   4: <*mut objc::runtime::Object as cocoa::appkit::NSScreen>::backingScaleFactor
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:3441:9
   5: gpui::platform::mac::window::get_scale_factor
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:1199:9
   6: gpui::platform::mac::window::MacWindowState::scale_factor
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:482:9
   7: gpui::platform::mac::window::set_frame_size
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:1702:24
   8: <unknown>
   9: <unknown>
  10: <(A,) as objc::message::MessageArguments>::invoke
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:128:17
  11: objc::message::platform::send_unverified
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/apple/mod.rs:27:9
  12: objc::message::send_message
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:178:5
  13: <*mut objc::runtime::Object as cocoa::appkit::NSWindow>::setContentView_
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:1445:9
  14: gpui::platform::mac::window::MacWindow::open
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/window.rs:689:13
  15: <gpui::platform::mac::platform::MacPlatform as gpui::platform::Platform>::open_window
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/platform.rs:583:21
  16: gpui::window::Window::new
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/window.rs:724:35
  17: gpui::app::App::open_window::{{closure}}
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:608:19
  18: gpui::app::App::update
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:426:22
  19: gpui::app::App::open_window
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:605:9
  20: gpui::main::{{closure}}
             at ./examples/gpui.rs:226:18
  21: gpui::app::Application::run::{{closure}}
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:166:13
  22: core::ops::function::FnOnce::call_once{{vtable.shim}}
             at /Users/zhangjunkui/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
  23: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /Users/zhangjunkui/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1976:9
  24: gpui::platform::mac::platform::did_finish_launching
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/platform.rs:1334:13
  25: <unknown>
  26: <unknown>
  27: <unknown>
  28: <unknown>
  29: <unknown>
  30: <unknown>
  31: <unknown>
  32: <unknown>
  33: <unknown>
  34: <unknown>
  35: <unknown>
  36: <unknown>
  37: <unknown>
  38: <unknown>
  39: <unknown>
  40: <unknown>
  41: <unknown>
  42: <unknown>
  43: <() as objc::message::MessageArguments>::invoke
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:128:17
  44: objc::message::platform::send_unverified
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/apple/mod.rs:27:9
  45: objc::message::send_message
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/objc-0.2.7/src/message/mod.rs:178:5
  46: <*mut objc::runtime::Object as cocoa::appkit::NSApplication>::run
             at /Users/zhangjunkui/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cocoa-0.26.0/src/appkit.rs:628:9
  47: <gpui::platform::mac::platform::MacPlatform as gpui::platform::Platform>::run
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/platform/mac/platform.rs:454:13
  48: gpui::app::Application::run
             at /Users/zhangjunkui/projects/zed/crates/gpui/src/app.rs:164:9
  49: gpui::main
             at ./examples/gpui.rs:217:3
  50: core::ops::function::FnOnce::call_once
             at /Users/zhangjunkui/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.
[1]    95817 abort      cargo run --example gpui -- --nocapture

JunkuiZhang avatar Apr 08 '25 16:04 JunkuiZhang

BTW, I suspect this bug might be related to recent rust updates, it looks very similar to this one. While adding null checks would be a quick fix, what's puzzling to me is why we're hitting a null pointer during creation in the first place.

Previous null pointer bugs we fixed were all environment-specific (like minimized windows), but getting one during initialization feels abnormal. There might be a deeper issue here that needs investigation beyond just the null check.

JunkuiZhang avatar Apr 08 '25 16:04 JunkuiZhang

That should be caused by it, we used GPUI version that not include #28599, I remember that I had update in last week.

https://github.com/huacnlee/zed/pull/16#event-17104923619

huacnlee avatar Apr 08 '25 17:04 huacnlee

I have rebased main branch latest code.

It is strange, I'm running fine.

image

~~I have added webview example as a commit into this PR. When later we have confirmed, we can reset to avoid merge webview example commit.~~

huacnlee avatar Apr 09 '25 01:04 huacnlee

Today, I hit this crash as the comment, After I add this code,

index 26a62aeadf..c49219bff1 100644
--- a/crates/gpui/src/platform/mac/window.rs
+++ b/crates/gpui/src/platform/mac/window.rs
@@ -844,6 +844,9 @@ impl PlatformWindow for MacWindow {
     fn display(&self) -> Option<Rc<dyn PlatformDisplay>> {
         unsafe {
             let screen = self.0.lock().native_window.screen();
+            if screen.is_null() {
+                return None;
+            }
             let device_description: id = msg_send![screen, deviceDescription];
             let screen_number: id = NSDictionary::valueForKey_(
                 device_description,
@@ -1193,6 +1196,9 @@ impl rwh::HasDisplayHandle for MacWindow {
 fn get_scale_factor(native_window: id) -> f32 {
     let factor = unsafe {
         let screen: id = msg_send![native_window, screen];
+        if screen.is_null() {
+            return 1.0;
+        }
         NSScreen::backingScaleFactor(screen) as f32
     };

I thought I fixed this problem, But the interesting things, is that, when I reset this modify code, rebuild to get the crash report, everything work well. I waste one more hour.

So I think this crash report might not block this PR.

I don't known if I should send a PR for this code.

CharlesChen0823 avatar May 12 '25 14:05 CharlesChen0823

I think, we can merge this PR now, @CharlesChen0823 have fixed crash issue, it is not relative on this PR changes. @SomeoneToIgnore @JunkuiZhang

huacnlee avatar May 20 '25 13:05 huacnlee

Like I mentioned in my earlier comment, I don't think just checking for a null pointer is a solid fix. The bigger issue here is that, for some reason, other parts of the code are running while the window is still being created (on macOS and Windows). Since I don’t have the bandwidth to fully debug this right now, I’ve decided to go ahead and merge this PR as-is.

JunkuiZhang avatar May 20 '25 14:05 JunkuiZhang

Thanks BTW

JunkuiZhang avatar May 20 '25 14:05 JunkuiZhang