`raw_debug_message_callback` seems to be called after `Context` is dropped

[Imberflur]

Dropping Context will attempt to clear the debug callback via gl.DebugMessageCallback(None, std::ptr::null()); since it drops the state that the callback refers to.

But I was getting segfaults, so I added in some println's and it looks like the callback can be invoked with the same pointer after the drop method for Context runs.

Maybe this is due to some asynchronous operations occurring?

[Imberflur]

Or maybe the gl.DebugMessageCallback(None, std::ptr::null()); call isn't succeeding

[Imberflur]

I think the context isn't current when it is dropped? Is this necessary? Hacking in a make_current in the drop of the owner seems to avoid the issue. Specifically for this type https://github.com/gfx-rs/wgpu/blob/fb3b33d09233140533a9e431256a8690bf4c5d42/wgpu-hal/src/gles/egl.rs#L307

Although, this could just be changing the timing of things.

edit: I put in an unmake_current call and the issue comes back, so it probably isn't the timing being changed that fixes it, making it current might be preventing operations in other threads that would generate the debug messages though?

[grovesNL]

Ah yeah that could make sense although it's a bit unfortunate because we can't do this automatically during drop (in glow).

I guess we could document that glow's context must be current when it's being dropped, and it would be the owner's responsibility to make the context current immediately before it drops. Does that make sense?

[Imberflur]

Yeah since everything is pretty unsafe that seems reasonable. Although, it might be more friendly in terms of avoiding accidental UB to panic and/or leak (edit: or just log an error and leak) if some other cleanup method isn't called. Especially, if the user already needs to arrange for the context to be current when this is dropped.

It's a bit unfortunate, especially since at least in wgpu the callback is stateless so the userdata could just be a function pointer of the callback provided by the glow user. But I can see the value of having some state here.

[Imberflur]

@grovesNL would you be interested in a PR for this that also addresses https://github.com/grovesNL/glow/issues/289?

[Imberflur]

actually there probably isn't any reason to combine these to one PR

[grovesNL]

Although, it might be more friendly in terms of avoiding accidental UB to panic and/or leak (edit: or just log an error and leak) if some other cleanup method isn't called.

I'm basically hoping to avoid extra function calls that aren't necessary when using GL, and usually it's not a problem unless multiple threads are using the context.

[Imberflur]

I'm basically hoping to avoid extra function calls that aren't necessary when using GL, and usually it's not a problem unless multiple threads are using the context.

Ah, that makes sense! Hmmm, the situation is complicated by the additional allocation to cleanup, that the user would probably handle themselves if using GL directly.

FWIW I'm happy with the documentation solution (and really just that the source of the issue was identified). And it seems like my proposed solution could easily be implemented on top of this if someone wants that.

That said, I did have fun trying to come up with a (somewhat complicated) way for users to opt into needing state. And then only requiring those users to free their state by calling debug_message_callback again with None:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d34d2eb676258a525bbf5767152e4434