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

@@ -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

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)) {
+                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<size_t>(data);
-            editor.emplace("yabridge plugin", plugin, x11_handle);
+            Editor& editor_instance =
+                editor.emplace<Editor>("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;

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> editor;
+    std::variant<std::monostate, Editor, EditorOpening> editor;
 };