coroutine_pause.rs test broken on head

[mandroll]

When running cargo test in my fork, I see the following error:

cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests src\lib.rs (target\debug\deps\macroquad-39e857bc1218d84e.exe)

running 2 tests
test color::color_from_bytes ... ok
test material::shaders::preprocessor_test ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\back_to_the_future.rs (target\debug\deps\back_to_the_future-29cb4bf128cc5937.exe)

running 1 test
test back_to_the_future ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.28s

     Running tests\back_to_the_future_coroutines.rs (target\debug\deps\back_to_the_future_coroutines-b2e3d9943e110d30.exe)

running 1 test
test back_to_the_future_coroutine ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.41s

     Running tests\coroutine_pause.rs (target\debug\deps\coroutine_pause-8289b31e25dc5fee.exe)

running 3 tests
test coroutine_execution_order ... ok
error: test failed, to rerun pass `--test coroutine_pause`

Caused by:
  process didn't exit successfully: `C:\Users\mandroll\Documents\GitHub\macroquad\target\debug\deps\coroutine_pause-8289b31e25dc5fee.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION)
note: test exited abnormally; to see the full output pass --nocapture to the harness.

[cyrgani]

The error comes from this Drop implementation:

impl Drop for GlPipelineGuarded {
    fn drop(&mut self) {
        get_context().gl.delete_pipeline(self.0);
    }
}

The mutable static CONTEXT contains a Option<Context> object. get_context returns a &'static mut Context.

A Context contains a UiContext, which contains a Option<Material>. A Material finally contains an Arc<GlPipelineGuarded>.

Since CONTEXT is a static variable, its value is not dropped at the end of the program.

When starting the first test, CONTEXT is None. While creating the window, CONTEXT will become Some(Context {...}).

At the end of the first test, this object will not be dropped and remain the same when the second test begins.

The second test will then try to reassign a new Some(Context {...}) to CONTEXT. This overwriting causes the old Context to be dropped. When its fields are being dropped, eventually a GlPipelineGuarded will be dropped as well. When dropping the GlPipelineGuarded, it will try to get a mutable reference to CONTEXT and modify it. Since the old Context is already partially dropped, this causes Undefined Behavior.

Now, I see these options:

1. Remove this implementation and hope nothing depends on it (removing it causes the test to succeed again).
2. Replace static mut with a type providing interior mutability (seems like the best way, given that references to mutable statics will be illegal in the 2024 edition).
3. Use a crate such as ctor to drop the static value at the end.

[cyrgani]

The error changed with the release of the latest miniquad version: it panics now instead of causing visible UB:

thread 'coroutine_execution_order' panicked at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/miniquad-0.4.5/src/lib.rs:90:29:
NATIVE_DISPLAY already set

[cyrgani]

This needs to be fixed within miniquad as even this minimized example of two macroquad tests without content panics:

pub mod test {
    pub static mut MUTEX: Option<std::sync::Mutex<()>> = None;
    pub static ONCE: std::sync::Once = std::sync::Once::new();
}

struct Stage {}

impl miniquad::EventHandler for Stage {
    fn update(&mut self) {}

    fn draw(&mut self) {
        miniquad::window::quit();
    }
}

pub fn new_window() {
    miniquad::start(miniquad::conf::Conf::default(), || Box::new(Stage {}));
}

#[test]
fn x() {
    let _lock = unsafe {
        test::ONCE.call_once(|| {
            test::MUTEX = Some(std::sync::Mutex::new(()));
        });
        test::MUTEX.as_mut().unwrap().lock()
    };
    new_window();
}

#[test]
fn y() {
    let _lock = unsafe {
        test::ONCE.call_once(|| {
            test::MUTEX = Some(std::sync::Mutex::new(()));
        });
        test::MUTEX.as_mut().unwrap().lock()
    };
    new_window();
}

[cyrgani]

This seems hard to fix: This is the relevant section from miniquads lib.rs:

static NATIVE_DISPLAY: OnceLock<Mutex<native::NativeDisplayData>> = OnceLock::new();

fn set_display(display: native::NativeDisplayData) {
    NATIVE_DISPLAY
        .set(Mutex::new(display))
        .unwrap_or_else(|_| panic!("NATIVE_DISPLAY already set"));
}
fn native_display() -> &'static Mutex<native::NativeDisplayData> {
    NATIVE_DISPLAY
        .get()
        .expect("Backend has not initialized NATIVE_DISPLAY yet.") //|| Mutex::new(Default::default()))
}

It would be necessary to drop this value at the end of each test, which would require replacing OnceLock with some other type. So far, I was not able to come up with a design to replace OnceLock that would avoid static mut, allow dropping the native::NativeDisplayData and allow returning &'static Mutex<native::NativeDisplayData>.

[Bentebent]

Any progress on this? It's causing issues downstream for implementing my own unit tests that use macroquad.

[cyrgani]

I've opened not-fl3/miniquad#505 and #866, which should fix the issue once both PRs are merged,