Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Crash on shutdown while calling OleUniitialize #557

Open
Psychlist1972 opened this issue Feb 24, 2025 · 5 comments
Open

[BUG] Crash on shutdown while calling OleUniitialize #557

Psychlist1972 opened this issue Feb 24, 2025 · 5 comments
Assignees
Labels
bug 🐞 Something isn't working needs-investigation 🔍 Needs to be investigated before considering or solving.

Comments

@Psychlist1972
Copy link
Contributor

Psychlist1972 commented Feb 24, 2025

Reported by Reuben Thomas (JUCE) on Discord

In JUCE, we call OleInitialize (nullptr) shortly after startup, and OleUninitialize() on shutdown after closing all COM resources.

For WinRT, I know we're supposed to call winrt::init_apartment() instead. I've read in a few places that it's actually fine (or even recommended) to skip calling winrt::uninit_apartment, but that advice seems to focus on applications rather than plugins. Given that we may be loaded and unloaded multiple times over the lifetime of a process, I'd prefer to clean up properly each time.

With the context out of the way, I'm seeing a crash on shutdown inside the final call to OleUninitialize() for JUCE apps that balance these COM setup/teardown calls. This happens after the message loop has exited and the message queue has been torn down. I'm omitting the stack trace for brevity, but it ends up crashing inside OleUninitialize while trying to call the destructor of IMidiSessionTracker, which seems to be owned by IMidiClientInitializer.

In the debugger, it appears that immediately after calling CoCreateInstance to make an IMidiClientInitializer, the returned object has a refcount of 2. This is also true at the point where we call Shutdown to close down our client instance. This means, even if we call release on our strong ref before reaching OleUninitialize, something else keeps the object alive with its own strong ref. This seems a bit weird - I wouldn't expect another object to hold a strong ref to an object we created, especially after we've called Shutdown to let the service know that we're about to go away.

This seems like a lifetime bug to me. I wonder, can anyone else can confirm?

@Psychlist1972 Psychlist1972 added the bug 🐞 Something isn't working label Feb 24, 2025
@Psychlist1972 Psychlist1972 self-assigned this Feb 24, 2025
@Psychlist1972 Psychlist1972 added the needs-investigation 🔍 Needs to be investigated before considering or solving. label Feb 24, 2025
@Psychlist1972
Copy link
Contributor Author

The SDK does need a multi-threaded apartment, and we do recommend calling winrt::init_apartment(), but uninit_apartment should not be needed (although I'll investigate your specific scenario).

I'll look to see what might be holding on to the IMidiSessionTracker object in this case as this sounds like a bug in the SDK or the RPC code.

@reuk
Copy link

reuk commented Feb 24, 2025

The SDK does need a multi-threaded apartment

I'm finding it a bit difficult to understand the implications of this. It seems like we want our main (UI) thread to be a STA, and we can't change the apartment mode without tearing down COM and setting it up again on the thread.

Are you saying that the thread that calls CoCreateInstance to create an IMidiClientInitializer needs to be in a MTA (i.e. we shouldn't call this from our UI thread), or that the process needs to have an (implicit) MTA before calling CoCreateInstance, or something else?

@Psychlist1972
Copy link
Contributor Author

The SDK does need a multi-threaded apartment

I'm finding it a bit difficult to understand the implications of this. It seems like we want our main (UI) thread to be a STA, and we can't change the apartment mode without tearing down COM and setting it up again on the thread.

If your UI thread is not the one initializing and using MIDI, it should be fine.

Are you saying that the thread that calls CoCreateInstance to create an IMidiClientInitializer needs to be in a MTA (i.e. we shouldn't call this from our UI thread), or that the process needs to have an (implicit) MTA before calling CoCreateInstance, or something else?

Here's what I'm saying. I should maybe amend it to be "prefer" and not "require".

  1. We try to limit use of coroutines in the SDK's interface, but there are some calls to internal WinRT components which must use them.

  2. We only test with MTA. The coroutines used in some calls are largely supposed to work with STA, but I have not personally tested them. (C++/WinRT does things like this behind the scenes: https://devblogs.microsoft.com/oldnewthing/20230124-00/?p=107746 ).

  3. Callbacks like the MidiMessageReceived event are typically going to come back on a different thread. If you initialize STA, there's additional marshaling/messaging overhead there. This should be tested in your app under STA if you want to go that route. I'd need to look into it, but C++/WinRT should be handling this for us, with the overhead built-in.

@Psychlist1972
Copy link
Contributor Author

Related issue on COM Lifetime is #558

@ReinholdH
Copy link

The SDK does need a multi-threaded apartment

I am having another/similar problem when using multi-threaded apartment. In our apps we use a solution for using 32-bit dlls with x64 apps. Please see https://blog.mattmags.com/2007/06/30/accessing-32-bit-dlls-from-64-bit-code/ (btw. a very old blog but still a very smart solution. See comments to the blog.)

This solution requires CoInitializeEx which is completely running while the app is open.

We found out that we need to use COINIT_APARTMENTTHREADED because with COINIT_MULTITHREADED the virtual printer "Microsoft Print to PDF" hungs up.

For Windows MIDI Services I have to change it back to COINIT_MULTITHREADED but now the virtual printer "Microsoft Print to PDF" won't work any longer.

I am not sure where the problem is but a solution needs to be found that "Microsoft Print to PDF" works, too. Probably a change in virtual printer "Microsoft Print to PDF" is required to be able to work with COINIT_MULTITHREADED. May be this is a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working needs-investigation 🔍 Needs to be investigated before considering or solving.
Projects
Status: No status
Development

No branches or pull requests

3 participants