fix(windows): Crash on app startup when Media Feature Pack is not installed on Windows N#2019
Conversation
Fixed this issue: #1985
…ware on VirusTotal (False positive))
Sync with original repo
…ware on VirusTotal (False positive))
Sync latest changes
I'm not sure what I'm supposed to test. This fix does not fix anything, app crash happens in different place in different file, audio_player.cpp Problem occurs not when you execute the code, it happens even before the app executes flutters main function, upon register somewhere in flutter engine, before app window is created That's why you need to wrap full AudioPlayer::AudioPlayer( Into try catch and add library presence check there , that's the base minimum |
As for the crash you mentioned in other pr, I'm not sure why it happens for you, I spent a few days testing previous version and it worked okay, try building in release mode and launching app through visual studio debugger, so that we can see where it happens |
|
Apparently the CI is not agreeing with me. I did test on usual Windows and Windows 11 N though...
@krll-kov The code part "AudioPlayer" isn't even initialized in my code implementation, so therefore cannot crash. It doesn't make sense to initialize a player, which cannot be used. |
AudioPlayer is never dynamically instantiated because of the if (isMediaFoundationSupported) check. But you are thinking about Runtime, while the crash is happening at Load-time (before the app's main() even starts). Because audio_player.cpp uses Media Foundation functions (like MFStartup), the compiler adds mfplat.dll and mfreadwrite.dll to the plugin's Import Address Table (IAT). The OS kills the app before AudioplayersWindowsPlugin gets to run its CheckMediaFoundationSupport() logic. That's why C++ if checks don't prevent the crash. |
By the way since fix was created, flutter introduced merged thread that changed a lot of things in plugin behavior, so that also could change what we see now with crash you mentioned |
But that's exactly what this change is for, so it happens at runtime, not startup / load time, what you did exactly the same in your implementation Otherwise your check always would have been unnecessary
In your implementation you call try catch after the dynamic library loading, so it wouldn't have an effect. Apparently the check works upfront without any try catch: And I tested my implementation: it did not initialize the player on Windows N system with the desired error messages, and it did initialize on a normal System and played the audio.
Thread merging apparently nothing has to do with plugin handling, they were always supposed to be on the same thread: But the thread switch helper function was added recently to Flutter: |
I remember having this crash on my system even with delayload that happened before initialization of my app in flutters main function, so somehow that code was executed during internal initialization inside engine Unhandled exception at 0x00007FFC310F804A (KernelBase.dll) in test.exe: 0xC06D007E: Module not found (parameters: 0x00000094F791F380). I have no idea how, maybe some static initialization or complier optimizations but I'm 100 % sure I didn't even create a player and crash was already present |
If you have reproducible steps, which crash despite the newly provided implementation, I am more than happy to let me convince. But, I see no reason to add a try catch, which isn't guaranteed to work. |
It's impossible to provide any steps since app just crashed upon launch before I even used any flutter code (after adding delayload that fixed initial dll absense error). Maybe something in package has changed within this year, or your windows n version is newer and behavior has changed or there's one more issue related to WinRT apartment initialization/some static initialization/something similar, I have no idea, but I'm sure that just checking for dll absense was not enough for some reason and somehow that code did trigger on launch. One more thing I notice is that right now you use newer version of WIL |
That's why I asked to test the new implementation (of this PR). We are discussing something, which was a year ago on assumptions, not on actual testing and reproducible steps.
But that's a different issue. That error could stem from anything. I don't know what code you use on your side. We have to agree on a common test base, which in our case is the example. You need to understand, that we cannot simply trust any statement and go with it. That's the job of a review to test and evaluate consistently. The issue was that the app crashes on Windows N system, which I could reproduce and fix in this common environment, based also on your solution. I would have even liked to test it in CI on Windows N, but there are no runners or one must start a VM, which introduces far more overhead, than it actually solves. If any further errors occur, we should find the source of it, so we know why it happens and can reliably fix it. |
I tested this example and it works, you were right, too many things have changed in a year, right now there's no crash at that place, false positives are also gone |
|
@krll-kov thank you for the time you invested to provide the initial fix and the testing. |
Commits marqués comme analyses et non portes: - 310304d ci: Upgrade CI Flutter version to v3.44.6 (bluefireteam#2014) — cosmétique CI - 244a4a0 ci: Support verbose debugging in tests (bluefireteam#2015) — cosmétique CI - 0b89829 test: Set timeouts for integration tests (bluefireteam#2016) — cosmétique tests - 6efd636 refactor: Handle common ReleaseMode.release logic in Dart (bluefireteam#2017) — redondant, deja gere nativement - fa6ec8e feat!: Add PlayingStateUpdate event (bluefireteam#1995) — over-engineering, 500 lignes pour rien - cd475c7 fix(windows): Crash Windows N Media Feature Pack (bluefireteam#2019) — try-catch global du fork couvre deja le cas
3 participants
Description
Fixes crash on missing Media Feature Pack on N and KN versions of Windows (European and Korean)
Now plugin uses delayed load mechanism for important dlls and provides errors accordingly.
Checklist
fix:,feat:,refactor:,docs:,chore:,test:,ci:etc).///, where necessary.Breaking Change
Related Issues
Fixes #1956