search

DISTRHO / DPF

DISTRHO Plugin Framework
ISC License
666 stars 96 forks source link

Nested event loop crash #299

Closed reuk closed 3 years ago

reuk commented 3 years ago

Environment

Steps to reproduce

Thread 0 Crashed:: reaper  Dispatch queue: com.apple.main-thread
0   d_info_ui.dylib                 0x00000001183480e8 puglUpdate + 280 (mac.m:1244)
1   d_info_ui.dylib                 0x0000000118351ab2 DGL::Application::PrivateData::idle(unsigned int) + 146
2   d_info_ui.dylib                 0x000000011834f5ab DGL::Application::idle() + 27 (Application.cpp:34)
3   d_info_ui.dylib                 0x000000011834ee9f DISTRHO::UIExporter::plugin_idle() + 79 (DistrhoUIInternal.hpp:249)
4   d_info_ui.dylib                 0x000000011834ee35 DISTRHO::UiLv2::lv2ui_idle() + 101 (DistrhoUILV2.cpp:199)
5   d_info_ui.dylib                 0x000000011834eae8 DISTRHO::lv2ui_idle(void*) + 24 (DistrhoUILV2.cpp:593)
6   com.cockos.reaper               0x000000010d105e9a lv2_run_idle() + 714
7   com.cockos.reaper               0x000000010cb54b41 runMiscTimers() + 1169
8   com.cockos.reaper               0x000000010cb54041 Main_OnTimer(HWND__*, unsigned long) + 2609
9   com.cockos.reaper               0x000000010d1acddd MainProc(HWND__*, unsigned int, unsigned long, long) + 2493
10  com.cockos.reaper               0x000000010d012276 SwellDialogDefaultWindowProc(HWND__*, unsigned int, unsigned long, long) + 438
11  com.apple.Foundation            0x00007fff369fad5a __NSFireTimer + 67
12  com.apple.CoreFoundation        0x00007fff3431d279 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
13  com.apple.CoreFoundation        0x00007fff3431cddf __CFRunLoopDoTimer + 859
14  com.apple.CoreFoundation        0x00007fff3431c8c7 __CFRunLoopDoTimers + 322
15  com.apple.CoreFoundation        0x00007fff3430167a __CFRunLoopRun + 1871
16  com.apple.CoreFoundation        0x00007fff343008ce CFRunLoopRunSpecific + 462
17  com.apple.HIToolbox             0x00007fff32f2cabd RunCurrentEventLoopInMode + 292
18  com.apple.HIToolbox             0x00007fff32f2c7d5 ReceiveNextEventCommon + 584
19  com.apple.HIToolbox             0x00007fff32f2c579 _BlockUntilNextEventMatchingListInModeWithFilter + 64
20  com.apple.AppKit                0x00007fff31573039 _DPSNextEvent + 883
21  com.apple.AppKit                0x00007fff31571880 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352
22  com.apple.AppKit                0x00007fff3156358e -[NSApplication run] + 658
23  com.apple.AppKit                0x00007fff31535396 NSApplicationMain + 777
24  com.cockos.reaper               0x000000010ca81934 start + 52

As far as I can tell, puglUpdate calls sendEvent to process UI events. Occasionally it will process an event which closes the UI window and the LV2 UI instance. As the stack unwinds, it tries to use some freed resource, crashing the program.

I'm not sure whether this is a bug in DPF or pugl (or elsewhere?) but DPF seems like the most likely candidate. On platforms where the process has a single shared event loop (windows/mac) I don't think it should be necessary to process application events from within a plugin's UI. To test this theory, I tried commenting out the event-handling bit of puglUpdate and building the Parameters demo plugin. Loading the modified plugin in REAPER, the blue squares still turn orange in response to mouse clicks as you'd expect.

It seems reasonable that the idle callback should not be used to invoke any action that might cause the UI to be destroyed, but perhaps I'm missing some important context here.

I did consider that this might be a bug in REAPER rather than in the plugin, but fixing such a bug would require REAPER to be written in such a way that any event could come from inside an idle callback. This would make it impossible to immediately destroy any plugin UI in response to a user interaction or other event, which doesn't sound practical.

falkTX commented 3 years ago

Very interesting...

I am not familiar with macOS internals, or the new pugl code for it, but if this works fine without calling puglUpdate then we can consider it for macOS builds. But a few precautions needed:

  1. plugin ui idle still needs to be called by the host. this is what dpf uses to drive some of its own events (so they are in sync with the host and dont try to render faster than the host; some juce plugins have this issue)
  2. pugl timers are still triggered, accessible in DPF via Window::addCallback() (not to be confused with the Application method with the same name, the Window one provides a timeout arg)
  3. extra windows created by a plugin UI still appear on screen and behave normally (though there is a crash around this on macOS at the moment)

If those 3 cases still work, then we are safe.

reuk commented 3 years ago

Thanks for the info.

I am not familiar with macOS internals, or the new pugl code for it, but if this works fine without calling puglUpdate then we can consider it for macOS builds.

I'm not necessarily suggesting to avoid calling puglUpdate altogether, as I think this would break some of the other points you mentioned. The calls to puglDispatchSimpleEvent (..., PUGL_UPDATE) and [view->impl->drawView displayIfNeeded] look important (I haven't tried testing without them yet).

That being said, the sendEvent part of that function seems quite dangerous, and I suspect it could be removed in plugins without any adverse effects.

Perhaps pugl needs to be aware of how it is being used, and disable the event processing code if it is not necessary, e.g. in a plugin. Alternatively, puglUpdate could be split into separate puglUpdateWithInternalEventLoop and puglUpdateWithExternalEventLoop functions, allowing users to pick the appropriate version to call depending on the context.

Having said that, this is starting to sound more like an issue with pugl, so I've opened an issue on that repo too.

lucianoiam commented 3 years ago

This happens on Mac/Live/VST as well with puglUpdate being the culprit. Removing the call avoids the crash without noticeable side effects but yes it is a cheap hack... Adding some more evidence:

Thread 0 Crashed:: MainThread  Dispatch queue: com.apple.main-thread
0   net.sf.distrho.d_states         0x0000000120ef7628 puglUpdate + 904
1   net.sf.distrho.d_states         0x0000000120f006ac DGL::Application::PrivateData::idle(unsigned int) + 76
2   net.sf.distrho.d_states         0x0000000120efe620 DISTRHO::UIVst::idle() + 192
3   net.sf.distrho.d_states         0x0000000120efb3c7 DISTRHO::PluginVst::vst_dispatcher(int, int, long, void*, float) + 823
...

crash_report.zip

https://user-images.githubusercontent.com/930494/127978368-8c303b38-cda3-4c1f-8126-673fdbf67f0d.mov

falkTX commented 3 years ago

I have merged @reuk 's fixes into DISTRHO/pugl repository and updated the DPF submodules. This should take care of the crashes, thanks a lot!

Testing is much appreciated.

lucianoiam commented 3 years ago

Tried all DPF examples in both Live 10 and REAPER and the crashes are gone.

falkTX commented 3 years ago

Closing this now, as I forgot to do it before