From 0900dc9e188303e672e4e938c5904c179f9223e9 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 12 May 2020 01:01:18 +0200 Subject: [PATCH] Work around race condition in certain plugins Some plugins would either crash or freeze on the next Win32 message loop when `effEditGetRect` gets called before `effEditOpen` and we run the message loop between these two event calls. Fixes the issue with Superior Drummer 3 in Bitwig mentioned in #12 and a similar issue with the Roland Cloud synths. --- CHANGELOG.md | 3 ++ src/wine-host/wine-bridge.cpp | 64 +++++++++++++++++++++++------------ src/wine-host/wine-bridge.h | 27 ++++++++++++++- 3 files changed, 72 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afc4db00..37883f9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Versioning](https://semver.org/spec/v2.0.0.html). - Added a workaround for plugins that improperly defer part of their initialization process without telling the host. This fixes startup behavior for the Roland Cloud plugins. +- Added a workaround for a rare race condition in certain plugins caused by + incorrect assumptions in plugin's editor handling. Fixes the editor for + Superior Drummer 3 and the Roland Cloud synths in Bitwig Studio. - Fixed potential issue with plugins not returning their editor size. ## [1.1.2] - 2020-05-09 diff --git a/src/wine-host/wine-bridge.cpp b/src/wine-host/wine-bridge.cpp index ef4fc5cf..387c8bd1 100644 --- a/src/wine-host/wine-bridge.cpp +++ b/src/wine-host/wine-bridge.cpp @@ -154,23 +154,29 @@ void WineBridge::handle_dispatch() { // Because of the way the Win32 API works we have to process events // on the same thread as the one the window was created on, and that - // thread is the thread that's handling dispatcher calls. - if (editor.has_value()) { - // This will handle Win32 events similar to the loop below, and - // it will also handle any X11 events. - editor->handle_events(); - } else { - MSG msg; + // thread is the thread that's handling dispatcher calls. Some + // plugins will also rely on the Win32 message loop to run tasks on + // a timer and to defer loading, so we have to make sure to always + // run this loop. The only exception is a in specific situation that + // can cause a race condition in some plugins because of incorrect + // assumptions made by the plugin. See the dostring for + // `WineBridge::editor` for more information. + std::visit(overload{[](Editor& editor) { editor.handle_events(); }, + [](std::monostate&) { + MSG msg; - // Since some plugins rely on the Win32 message API even for - // non-editor related tasks (such as deferring the loading of - // presets using a timer), we have to run a message loop even - // when the editor is closed. - while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { - TranslateMessage(&msg); - DispatchMessage(&msg); - } - } + while (PeekMessage(&msg, nullptr, 0, 0, + PM_REMOVE)) { + TranslateMessage(&msg); + DispatchMessage(&msg); + } + }, + [](EditorOpening&) { + // Don't handle any events in this + // particular case as explained in + // `WineBridge::editor` + }}, + editor); } } catch (const boost::system::system_error&) { // The plugin has cut off communications, so we can shut down this host @@ -314,22 +320,38 @@ intptr_t WineBridge::dispatch_wrapper(AEffect* plugin, // We have to intercept GUI open calls since we can't use // the X11 window handle passed by the host switch (opcode) { + case effEditGetRect: { + // Some plugins will have a race condition if the message loops gets + // handled between the call to `effEditGetRect()` and + // `effEditOpen()`, although this behavior never appears on Windows + // as hosts will always either call these functions in sequence or + // in reverse. We need to temporarily stop handling messages when + // this happens. + if (!std::holds_alternative(editor)) { + editor = EditorOpening{}; + } + + return plugin->dispatcher(plugin, opcode, index, value, data, + option); + } break; case effEditOpen: { // Create a Win32 window through Wine, embed it into the window - // provided by the host, and let the plugin embed itself into the - // Wine window + // provided by the host, and let the plugin embed itself into + // the Wine window const auto x11_handle = reinterpret_cast(data); - editor.emplace("yabridge plugin", plugin, x11_handle); + Editor& editor_instance = + editor.emplace("yabridge plugin", plugin, x11_handle); return plugin->dispatcher(plugin, opcode, index, value, - editor->win32_handle.get(), option); + editor_instance.win32_handle.get(), + option); } break; case effEditClose: { const intptr_t return_value = plugin->dispatcher(plugin, opcode, index, value, data, option); // Cleanup is handled through RAII - editor = std::nullopt; + editor = std::monostate(); return return_value; } break; diff --git a/src/wine-host/wine-bridge.h b/src/wine-host/wine-bridge.h index 060eec21..d03a6411 100644 --- a/src/wine-host/wine-bridge.h +++ b/src/wine-host/wine-bridge.h @@ -33,6 +33,13 @@ #include "editor.h" #include "utils.h" +/** + * A marker struct to indicate that the editor is about to be opened. + * + * @see WineBridge::editor + */ +struct EditorOpening {}; + /** * This handles the communication between the Linux native VST plugin and the * Wine VST host. The functions below should be used as callback functions in an @@ -186,6 +193,24 @@ class WineBridge { * The plugin editor window. Allows embedding the plugin's editor into a * Wine window, and embedding that Wine window into a window provided by the * host. Should be empty when the editor is not open. + * + * This field can have three possible states: + * + * - `std::nullopt` when the editor is closed. + * - An `Editor` object when the editor is open. + * - `EditorOpening` when the editor is not yet open, but the host has + * already called `effEditGetRect()` and is about to call `effEditOpen()`. + * This is needed because there is a race condition in some bugs that + * cause them to crash or enter an infinite Win32 message loop when + * `effEditGetRect()` gets dispatched and we then enter the message loop + * loop before `effEditOpen()` gets called. Most plugins will handle this + * just fine, but a select few plugins make the assumption that the editor + * is already open once `effEditGetRect()` has been called, even if + * `effEditOpen` has not yet been dispatched. VST hsots on Windows will + * call these two events in sequence, so the bug would never occur there. + * To work around this we'll use this third state to temporarily stop + * processing Windows events in the one or two ticks between these two + * events. */ - std::optional editor; + std::variant editor; };