Make context current when dropping glow::Context to allow for debug callback cleanup

[Imberflur]

Description

Previously when running tests via cargo xtask test, I noticed some of the tests randomly segfaulting on opengl. This was due to the debug message callback running after its state was freed when glow::Context is dropped (i.e. an instance of grovesNL/glow#270).

The latest version of glow fixes this by clearing the debug message callback in its Drop impl. This requires the context to be current when dropping glow::Context (which is now documented in https://docs.rs/glow/0.14.0/glow/trait.HasContext.html#safety).

This PR adds some logic to make the context current when the glow::Context will be dropped and utilizes ManuallyDrop to control specifically when the drop occurs as well as preventing accidental drops from unwinding.

Presumably the context is always current for gles/web.rs?

Testing

Running cargo xtask test on a Linux machine.

Unfortunately in the latest version of wgpu, I'm not able to reproduce this segfault by running the tests, probably due to timing changes. So it is hard to confirm that this is fixing things. But it should theoretically be correct and my previous experimentation showed this fixing the bug.

Checklist

• Run cargo fmt.
• Run cargo clippy. If applicable, add:
  • --target wasm32-unknown-unknown
  • --target wasm32-unknown-emscripten
• Run cargo xtask test to run tests.
• Add change to CHANGELOG.md. See simple instructions inside file.

[cwfitzgerald]

Looks good to me!

[cwfitzgerald]

Presumably the context is always current for gles/web.rs?

Yeah there's no concept of non-current contexts

[MarijnS95]

Is this a stale comment? You're not avoiding a drop when the context is not current, instead you're forcing it to be current Before dropping.

[Imberflur]

This comment is noting the reason to wrap in ManuallyDrop before calling unmake_current on the line below. This prevents the glow::Context from being dropped if there is some panic between when we call unmake_current and when gl is placed into the AdapterContext (where the drop impl will handle making the context current before dropping glow::Context).

FWIW the comment could probably be clarified.

[MarijnS95]

Clarification is happening in #6152 where I ran into a few conflicts with this PR. Panics (except inside the drop handler) should still cause Drop to be called while unwinding. I'll add a mention that the only skipped case is between let gl and when Self {} is finally constructed.

Fwiw when a debug callback is specified by the user (in this case wgpu), it's common to skip messages when panicking:

wgpu/wgpu-hal/src/vulkan/instance.rs

Lines 21 to 23 in 5deaef3

	if thread::panicking() {
	return vk::FALSE;
	}

Not sure if that can be done in EGL/WGL as well? Otherwise it seems this solution is a litle bit overengineered if it's just about making the context current in drop()?

[Imberflur]

Panics (except inside the drop handler) should still cause Drop to be called while unwinding

Yes, this is why it needs to be wrapped in ManuallyDrop in the intermittent time before AdapterContext is constructed since panics would drop the glow::Context which could lead to UB if it isn't current.

Fwiw when a debug callback is specified by the user (in this case wgpu), it's common to skip messages when panicking:

Something like this won't help here with the way the Drop impl for glow::Context works, by the time the callback is invoked there is already UB.