Pass pointers to IMessage objects around

Instead of serializing the actual `YaMessage`, for the reasons mentioned
in the comments. This was needed to stop iZotope VocalSynth 2 in Ardour
from segfaulting when editing parameters, because that plugin is
apparently being very naughty.

src/common/logging/vst3.cpp

@@ -126,13 +126,15 @@ bool Vst3Logger::log_request(bool is_host_vst,
 bool Vst3Logger::log_request(bool is_host_vst,
                              const YaConnectionPoint::Notify& request) {
     return log_request_base(is_host_vst, [&](auto& message) {
+        // We can safely print the pointer as long we don't dereference it
         message << request.instance_id
-                << ": IConnectionPoint::notify(message = <IMessage* ";
+                << ": IConnectionPoint::notify(message = <IMessage* "
+                << request.message_ptr.get_original();
         if (const char* id =
-                const_cast<YaMessage&>(request.message).getMessageID()) {
-            message << "with ID = \"" << id << "\"";
+                const_cast<YaMessagePtr&>(request.message_ptr).getMessageID()) {
+            message << " with ID = \"" << id << "\"";
         } else {
-            message << "without an ID";
+            message << " without an ID";
         }
         message << ">)";
     });

src/common/serialization/vst3/README.md

@@ -42,6 +42,7 @@ implemented for serialization purposes:
 | `YaAttributeList`    | `IAttributeList`    |                                                                        |
 | `YaEventList`        | `IEventList`        | Comes with a lot of serialization wrappers around the related structs. |
 | `YaMessage`          | `IMessage`          |                                                                        |
+| `YaMessagePtr`       | `IMessage`          | Should be used in inter process communication to exchange messages     |
 | `YaParameterChanges` | `IParameterChanges` |                                                                        |
 | `YaParamValueQueue`  | `IParamValueQueue`  |                                                                        |
 | `VectorStream`       | `IBStream`          | Used for serializing data streams.                                     |

src/common/serialization/vst3/message.cpp

@@ -16,6 +16,52 @@
 
 #include "message.h"
 
+YaMessagePtr::YaMessagePtr(){FUNKNOWN_CTOR}
+
+YaMessagePtr::YaMessagePtr(IMessage& message)
+    : message_id(message.getMessageID()
+                     ? std::make_optional<std::string>(message.getMessageID())
+                     : std::nullopt),
+      original_message_ptr(static_cast<native_size_t>(
+          reinterpret_cast<size_t>(&message))){FUNKNOWN_CTOR}
+
+      YaMessagePtr::~YaMessagePtr() {
+    FUNKNOWN_DTOR
+}
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor"
+IMPLEMENT_FUNKNOWN_METHODS(YaMessagePtr,
+                           Steinberg::Vst::IMessage,
+                           Steinberg::Vst::IMessage::iid)
+#pragma GCC diagnostic pop
+
+Steinberg::Vst::IMessage* YaMessagePtr::get_original() const {
+    // See the docstrings on `YaMessage` and `YaMessagePtr`
+    return reinterpret_cast<IMessage*>(
+        static_cast<size_t>(original_message_ptr));
+}
+
+Steinberg::FIDString PLUGIN_API YaMessagePtr::getMessageID() {
+    if (message_id) {
+        return message_id->c_str();
+    } else {
+        return nullptr;
+    }
+}
+
+void PLUGIN_API YaMessagePtr::setMessageID(Steinberg::FIDString id /*in*/) {
+    if (id) {
+        message_id = id;
+    } else {
+        message_id.reset();
+    }
+}
+
+Steinberg::Vst::IAttributeList* PLUGIN_API YaMessagePtr::getAttributes() {
+    return &attribute_list;
+}
+
 YaMessage::YaMessage(){FUNKNOWN_CTOR}
 
 YaMessage::~YaMessage() {

src/common/serialization/vst3/message.h

@@ -19,6 +19,7 @@
 #include <bitsery/ext/std_optional.h>
 #include <pluginterfaces/vst/ivstmessage.h>
 
+#include "../common.h"
 #include "attribute-list.h"
 #include "base.h"
 
@@ -26,15 +27,94 @@
 #pragma GCC diagnostic ignored "-Wnon-virtual-dtor"
 
 /**
- * Wraps around `IMessage` for serialization purposes. We create instances of
- * these in `IHostApplication::createInstance()` so the Windows VST3 plugin can
- * send messages between objects. There is one huge caveat here: it is
- * impossible to work with arbitrary `IMessage` objects, since there's no way to
- * retrieve all of the keys in the attribute list. With this approach we support
- * hosts that indirectly connect the processor and the controller through a
- * proxy (like Ardour), but we still require a dynamic cast from the `IMessage*`
- * passed to `YaConnectionPoint::notify()` to a `YaMessage*` for this to work
- * for the above mentioned reason.
+ * A serialization wrapper around `IMessage`. As explained in `YaMessage`, we
+ * can't exchange the regular `YaMessage` object when dealing with
+ * `IConnectionPoint` connection proxies. Instead, we'll use this wrapper that
+ * only stores the ID (for logging purposes) and a pointer to the original
+ * object. That way we can pass the original message created by the plugin to
+ * the receiver without having to know what object the host's connection proxy
+ * is actually connecting us to.
+ *
+ * @note THis object should _not_ be passed to the plugin directly. The only
+ *   purpose of this object is to be able to pass the original `IMessage*`
+ *   object passed the connection proxy to the receiver, by wrapping a pointer
+ *   to it in this object. `YaMessagePtr::get_original()` can be used to
+ *   retrieve the original object.
+ */
+class YaMessagePtr : public Steinberg::Vst::IMessage {
+   public:
+    YaMessagePtr();
+
+    /**
+     * Create a proxy for this message. We'll store the message's ID for logging
+     * purposes as well as a pointer to it so we can retrieve the object after a
+     * round trip from the Wine plugin host, to the native plugin, to the host,
+     * back to the native plugin, and then finally back to the Wine plugin host
+     * again.
+     */
+    explicit YaMessagePtr(IMessage& message);
+
+    ~YaMessagePtr();
+
+    DECLARE_FUNKNOWN_METHODS
+
+    /**
+     * Get back a pointer to the original `IMessage` object passed to the
+     * constructor. This should be used on the Wine plugin host side when
+     * handling `IConnectionPoint::notify`.
+     */
+    Steinberg::Vst::IMessage* get_original() const;
+
+    virtual Steinberg::FIDString PLUGIN_API getMessageID() override;
+    virtual void PLUGIN_API
+    setMessageID(Steinberg::FIDString id /*in*/) override;
+    virtual Steinberg::Vst::IAttributeList* PLUGIN_API getAttributes() override;
+
+    template <typename S>
+    void serialize(S& s) {
+        s.ext(message_id, bitsery::ext::StdOptional{},
+              [](S& s, std::string& id) { s.text1b(id, 1024); });
+        s.value8b(original_message_ptr);
+    }
+
+   private:
+    /**
+     * The implementation that comes with the SDK returns a null pointer when
+     * the ID has not yet been set, so we'll do the same thing.
+     */
+    std::optional<std::string> message_id;
+
+    /**
+     * The pointer to the message passed during the constructor, as a 64-bit
+     * unsigned integer. This way we can retrieve the original object after a
+     * round trip.
+     */
+    native_size_t original_message_ptr = 0;
+
+    /**
+     * An empty attribute list, in case the host checks this for some reason.
+     */
+    YaAttributeList attribute_list;
+};
+
+/**
+ * A `IMessage` implementation the plugin can use to exchange messages with. We
+ * create instances of these in `IHostApplication::createInstance()` so the
+ * Windows VST3 plugin can send messages between objects. A plugin's controller
+ * or processor will fill the message with data and then try to send it to the
+ * connected object using `IConnectionPoint::notify()`. For directly connected
+ * objects this works exactly like you'd expect. When the host places a proxy
+ * between the two, it becomes a bit more interesting, and we'll have to proxy
+ * that proxy. In that case we won't send the actual `YaMessage` object from the
+ * Wine plugin host to the native plugin, and then back to the Wine plugin host.
+ * Instead, we'll send a thin wrapper that only stores a name and a pointer to
+ * the actual object. This is needed in case the plugin tries to store the
+ * `IMessage` object, thinking it's backed by a smart pointer. This means that
+ * the message we pass while handling `IConnectionPoint::notify` should live as
+ * long as the original message object, thus we'll use a pointer to get back the
+ * original message object.
+ *
+ * @relates YaMessagePtr
  */
 class YaMessage : public Steinberg::Vst::IMessage {
    public:
@@ -53,13 +133,6 @@ class YaMessage : public Steinberg::Vst::IMessage {
     setMessageID(Steinberg::FIDString id /*in*/) override;
     virtual Steinberg::Vst::IAttributeList* PLUGIN_API getAttributes() override;
 
-    template <typename S>
-    void serialize(S& s) {
-        s.ext(message_id, bitsery::ext::StdOptional{},
-              [](S& s, std::string& id) { s.text1b(id, 1024); });
-        s.object(attribute_list);
-    }
-
    private:
     /**
      * The implementation that comes with the SDK returns a null pointer when

src/common/serialization/vst3/plugin/connection-point.h

@@ -178,21 +178,24 @@ class YaConnectionPoint : public Steinberg::Vst::IConnectionPoint {
      * the Wine plugin host. Since `IAttributeList` does not have any way to
      * iterate over all values, we only support messages sent by plugins using
      * our own implementation of the interface, since there's no way to
-     * serialize them otherwise. This `IConnectionPoint::notify()`
-     * implementation is also only used with hosts that do not connect objects
-     * directly and use connection proxies instead.
+     * serialize them otherwise. Additionally, plugins may store the `IMessage`
+     * pointer for later usage, so we have to pass through a pointer to the
+     * original message so it has the same lifetime as the original message.
+     * This `IConnectionPoint::notify()` implementation is also only used with
+     * hosts that do not connect objects directly and use connection proxies
+     * instead.
      */
     struct Notify {
         using Response = UniversalTResult;
 
         native_size_t instance_id;
 
-        YaMessage message;
+        YaMessagePtr message_ptr;
 
         template <typename S>
         void serialize(S& s) {
             s.value8b(instance_id);
-            s.object(message);
+            s.object(message_ptr);
         }
     };
 

src/plugin/bridges/vst3-impls/plugin-proxy.cpp

@@ -250,14 +250,18 @@ tresult PLUGIN_API
 Vst3PluginProxyImpl::notify(Steinberg::Vst::IMessage* message) {
     // Since there is no way to enumerate over all values in an
     // `IAttributeList`, we can only support relaying messages that were sent by
-    // our own objects. This is only needed to support hosts that place a
-    // connection proxy between two objects instead of connecting them directly.
-    // If the objects are connected directly we also connected them directly on
-    // the Wine side, so we don't have to do any additional when those objects
-    // pass through messages.
-    if (auto message_impl = dynamic_cast<YaMessage*>(message)) {
+    // our own objects. Additionally, the `IMessage*` we end up passing to the
+    // plugin needs to have the same lifetime as the original object, because
+    // some plugins are being a bit naughty. That's why we pass around a pointer
+    // to the original message object.
+    // All of this is only needed to support hosts that place a connection proxy
+    // between two objects instead of connecting them directly.  If the objects
+    // are connected directly we also connected them directly on the Wine side,
+    // so we don't have to do any additional when those objects pass through
+    // messages.
+    if (auto message_ptr = dynamic_cast<YaMessagePtr*>(message)) {
         return bridge.send_message(YaConnectionPoint::Notify{
-            .instance_id = instance_id(), .message = *message_impl});
+            .instance_id = instance_id(), .message_ptr = *message_ptr});
     } else {
         bridge.logger.log(
             "WARNING: Unknown message type passed to "

src/plugin/bridges/vst3.cpp

@@ -110,7 +110,7 @@ Vst3PluginBridge::Vst3PluginBridge()
                     -> YaConnectionPoint::Notify::Response {
                     return plugin_proxies.at(request.instance_id)
                         .get()
-                        .connection_point_proxy->notify(&request.message);
+                        .connection_point_proxy->notify(&request.message_ptr);
                 },
                 [&](const YaHostApplication::GetName& request)
                     -> YaHostApplication::GetName::Response {

src/wine-host/bridges/vst3-impls/connection-point-proxy.cpp

@@ -55,15 +55,14 @@ Vst3ConnectionPointProxyImpl::disconnect(IConnectionPoint* /*other*/) {
 
 tresult PLUGIN_API
 Vst3ConnectionPointProxyImpl::notify(Steinberg::Vst::IMessage* message) {
-    // As explained in `YaMessage` and `Vst3PluginProxyImpl::notify`, we can
-    // only support our own `IMessage implementation here`
-    if (auto message_impl = dynamic_cast<YaMessage*>(message)) {
-        return bridge.send_message(YaConnectionPoint::Notify{
-            .instance_id = owner_instance_id(), .message = *message_impl});
+    if (message) {
+        return bridge.send_message(
+            YaConnectionPoint::Notify{.instance_id = owner_instance_id(),
+                                      .message_ptr = YaMessagePtr(*message)});
     } else {
-        std::cerr << "WARNING: Unknown message type passed to "
+        std::cerr << "WARNING: Null pointer passed to "
                      "'IConnectionPoint::notify()', ignoring"
                   << std::endl;
-        return Steinberg::kNotImplemented;
+        return Steinberg::kInvalidArgument;
     }
 }

src/wine-host/bridges/vst3.cpp

@@ -223,21 +223,21 @@ void Vst3Bridge::run() {
                     return result;
                 }
             },
-            [&](YaConnectionPoint::Notify& request)
+            [&](const YaConnectionPoint::Notify& request)
                 -> YaConnectionPoint::Notify::Response {
-                // FIXME: This needs to be redesigned. We should only send a
-                //        pointer to the `IMessage` instead of copying the
-                //        actual message. What ends up happening is that iZotope
-                //        VocalSynth 2 exchanges pointers to the processor and
-                //        the controller. Then at some point during an
-                //        `IAudioProcessor::process()` call after a parameter
-                //        changes, it will try to access the pointers stored in
-                //        that message. So to be able to support that, the
-                //        message object we pass to notify here still has to be
-                //        alive at that point. This goes 100% against the design
-                //        of VST3, but there's nothing we can do about it.
+                // NOTE: We're using a few tricks here to pass through a pointer
+                //       to the _original_ `IMessage` object passed to a
+                //       connection proxy. This is needed because some plugins
+                //       like iZotope VocalSynth 2 use these messages to
+                //       exchange pointers between their objects so they can
+                //       break out of VST3's separation, but they might also
+                //       store the message object and not the actual pointers.
+                //       We should thus be passing a (raw) pointer to the
+                //       original object so we can pretend none of this wrapping
+                //       and serializing has ever happened.
                 return object_instances[request.instance_id]
-                    .connection_point->notify(&request.message);
+                    .connection_point->notify(
+                        request.message_ptr.get_original());
             },
             [&](YaEditController::SetComponentState& request)
                 -> YaEditController::SetComponentState::Response {