fix: Panic if the subclassing adapter is double-instantiated (#596)

Author: mwcampbell

Cargo.lock

@@ -146,6 +146,13 @@ impl SubclassingAdapter {
         action_handler: impl 'static + ActionHandler,
     ) -> Self {
         let view = Id::as_ptr(&retained_view) as *mut NSView;
+        if !unsafe {
+            objc_getAssociatedObject(view as *const NSView as *const _, associated_object_key())
+        }
+        .is_null()
+        {
+            panic!("subclassing adapter already instantiated on view {view:?}");
+        }
         let adapter = unsafe { Adapter::new(view as *mut c_void, false, action_handler) };
         // Cast to a pointer and back to force the lifetime to 'static
         // SAFETY: We know the class will live as long as the instance,

platforms/macos/src/subclass.rs

@@ -38,5 +38,6 @@ features = [
 
 [dev-dependencies]
 once_cell = "1.13.0"
+parking_lot = "0.12.4"
 scopeguard = "1.1.0"
 winit = "0.30"

platforms/windows/Cargo.toml

@@ -93,6 +93,12 @@ impl SubclassImpl {
     }
 
     fn install(&mut self) {
+        if !unsafe { GetPropW(self.hwnd, PROP_NAME) }.0.is_null() {
+            panic!(
+                "subclassing adapter already instantiated on window {:?}",
+                self.hwnd.0
+            );
+        }
         unsafe {
             SetPropW(
                 self.hwnd,

platforms/windows/src/subclass.rs

@@ -180,7 +180,9 @@ impl Scope {
 }
 
 // It's not safe to run these UI-related tests concurrently.
-pub(crate) static MUTEX: Mutex<()> = Mutex::new(());
+// We need a non-poisoning mutex here because the subclassing adapter's
+// double-instantiation test intentionally panics.
+pub(crate) static MUTEX: parking_lot::Mutex<()> = parking_lot::const_mutex(());
 
 pub(crate) fn scope<F>(
     window_title: &str,
@@ -191,7 +193,7 @@ pub(crate) fn scope<F>(
 where
     F: FnOnce(&Scope) -> Result<()>,
 {
-    let _lock_guard = MUTEX.lock().unwrap();
+    let _lock_guard = MUTEX.lock();
 
     let window_mutex: Mutex<Option<WindowHandle>> = Mutex::new(None);
     let window_cv = Condvar::new();

platforms/windows/src/tests/mod.rs

@@ -6,7 +6,15 @@
 use accesskit::{
     Action, ActionHandler, ActionRequest, ActivationHandler, Node, NodeId, Role, Tree, TreeUpdate,
 };
-use windows::Win32::{Foundation::*, UI::Accessibility::*};
+use once_cell::sync::Lazy;
+use windows::{
+    core::*,
+    Win32::{
+        Foundation::*,
+        System::LibraryLoader::GetModuleHandleW,
+        UI::{Accessibility::*, WindowsAndMessaging::*},
+    },
+};
 use winit::{
     application::ApplicationHandler,
     event::WindowEvent,
@@ -63,7 +71,14 @@ impl ActivationHandler for SimpleActivationHandler {
 }
 
 // This module uses winit for the purpose of testing with a real third-party
-// window implementation that we don't control.
+// window implementation that we don't control. However, only one test
+// can use winit, because winit only allows an event loop to be created
+// once per process. So we end up creating our own window anyway for the
+// double-instantiation test.
+//
+// Also, while these tests don't use the main test harness or show the window,
+// they still need to run with the main harness's mutex, to avoid disturbing
+// other tests, particularly the focus test.
 
 struct TestApplication;
 
@@ -92,12 +107,69 @@ impl ApplicationHandler<()> for TestApplication {
 
 #[test]
 fn has_native_uia() {
-    // This test is simple enough that we know it's fine to run entirely
-    // on one thread, so we don't need a full multithreaded test harness.
-    // Still, we must prevent this test from running concurrently with other
-    // tests, especially the focus test.
-    let _lock_guard = MUTEX.lock().unwrap();
+    let _lock_guard = MUTEX.lock();
     let event_loop = EventLoop::builder().with_any_thread(true).build().unwrap();
     let mut state = TestApplication {};
     event_loop.run_app(&mut state).unwrap();
 }
+
+extern "system" fn wndproc(window: HWND, message: u32, wparam: WPARAM, lparam: LPARAM) -> LRESULT {
+    unsafe { DefWindowProcW(window, message, wparam, lparam) }
+}
+
+static WINDOW_CLASS_ATOM: Lazy<u16> = Lazy::new(|| {
+    let class_name = w!("AccessKitSubclassTest");
+
+    let wc = WNDCLASSW {
+        hCursor: unsafe { LoadCursorW(None, IDC_ARROW) }.unwrap(),
+        hInstance: unsafe { GetModuleHandleW(None) }.unwrap().into(),
+        lpszClassName: class_name,
+        style: CS_HREDRAW | CS_VREDRAW,
+        lpfnWndProc: Some(wndproc),
+        ..Default::default()
+    };
+
+    let atom = unsafe { RegisterClassW(&wc) };
+    if atom == 0 {
+        panic!("{}", Error::from_win32());
+    }
+    atom
+});
+
+fn create_window(title: &str) -> HWND {
+    let module = HINSTANCE::from(unsafe { GetModuleHandleW(None).unwrap() });
+
+    let window = unsafe {
+        CreateWindowExW(
+            Default::default(),
+            PCWSTR(*WINDOW_CLASS_ATOM as usize as _),
+            &HSTRING::from(title),
+            WS_OVERLAPPEDWINDOW,
+            CW_USEDEFAULT,
+            CW_USEDEFAULT,
+            CW_USEDEFAULT,
+            CW_USEDEFAULT,
+            None,
+            None,
+            Some(module),
+            None,
+        )
+    }
+    .unwrap();
+    if window.is_invalid() {
+        panic!("{}", Error::from_win32());
+    }
+
+    window
+}
+
+#[test]
+#[should_panic(expected = "already instantiated")]
+fn double_instantiate() {
+    let _lock_guard = MUTEX.lock();
+    let window = create_window(WINDOW_TITLE);
+    let _adapter1 =
+        SubclassingAdapter::new(window, SimpleActivationHandler {}, NullActionHandler {});
+    let _adapter2 =
+        SubclassingAdapter::new(window, SimpleActivationHandler {}, NullActionHandler {});
+}