Fix issues that caused ActiveQt qutlook sample to crash

The issues reported on qutlook sample appears to be caused by previous
hardening of the QVariant type, which now does not accept copying
uncopyable types.

The fix to this problem seems to be to be more consistent on use of
pointer types (which are copyable), vs value types generated by the
dumpcpp preprocessor.

In the function VARIANTToQVariant, we may end up in constructing a
wrapper object on top of an IDispatch interface. To be able to construct
an object, we need the value type of the wrapper object. Still, we
should pass it out as a pointer type. This change ensures that a pointer
type is returned if a pointer type was requested.

A second similar issue is fixed in QAxBase::internalInvoke, by passing
(what I assume is) the destination type into VARIANTToQVariant. This
prevents decaying pointer types into value types. I am not sure if this
is the right approach here, and it leaves the decay code behind, which
now might be unused.

Task-number: QTBUG-111191
Pick-to: 6.5 6.6
Change-Id: I0331ee5e3f2dbeed2db45e152378c223feb6fc94
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

5 files changed, 135 insertions, 9 deletions

diff --git a/src/activeqt/container/qaxbase.cpp b/src/activeqt/container/qaxbase.cpp
index 973347b..5ba59c3 100644
--- a/src/activeqt/container/qaxbase.cpp
+++ b/src/activeqt/container/qaxbase.cpp
@@ -3452,7 +3452,7 @@ int QAxBase::internalInvoke(QMetaObject::Call call, int index, void **v)
 
     // get return value
     if (hres == S_OK && ret.vt != VT_EMPTY) {
-        QVariantToVoidStar(VARIANTToQVariant(ret, slot.typeName()), v[0], slot.typeName());
+        QVariantToVoidStar(VARIANTToQVariant(ret, slot.typeName(), slot.returnType()), v[0], slot.typeName());
         if (ret.vt != VT_DISPATCH)
             clearVARIANT(&ret);
         else
diff --git a/src/activeqt/shared/qaxtypes.cpp b/src/activeqt/shared/qaxtypes.cpp
index 027511c..0dfbe28 100644
--- a/src/activeqt/shared/qaxtypes.cpp
+++ b/src/activeqt/shared/qaxtypes.cpp
@@ -718,7 +718,10 @@ static QVariant axServer(IUnknown *unknown, const QByteArray &typeName)
 #undef QVARIANT_TO_VARIANT_POD
 
 /*
-    Returns \a arg as a QVariant of type \a type.
+    Returns \a arg as a QVariant of type \a typeName or \a type.
+
+    NOTE: If a \a typeName is specified, value type is assumed. to
+    get/create a pointer type, provide the type id in the \a type argument.
 
     Used by
 
@@ -933,24 +936,43 @@ QVariant VARIANTToQVariant(const VARIANT &arg, const QByteArray &typeName, int t
                 {
                     if (!typeName.isEmpty()) {
                         if (arg.vt & VT_BYREF) {
+                            // When the dispinterface is a return value, just assign it to a QVariant
                             static const int dispatchId = qRegisterMetaType<IDispatch**>("IDispatch**");
                             var = QVariant(QMetaType(dispatchId), &arg.ppdispVal);
                         } else {
 #ifndef QAX_SERVER
                             if (typeName == "QVariant") {
+                                // If a QVariant is requested, wrap the dispinterface in a QAxObject
                                 QAxObject *object = new QAxObject(disp);
                                 var = QVariant::fromValue<QAxObject*>(object);
                             } else if (typeName != "IDispatch*" &&  QMetaType::fromName(typeName).id() != QMetaType::UnknownType) {
-                                QByteArray typeNameStr = QByteArray(typeName);
+                                // Conversion from IDispatch* to a wrapper type is requested. Here, the requested
+                                // wrapper type is constructed around the dispinterface, and then returned as
+                                // a QVariant containing a pointer to the wrapper type.
+
+                                // Calculate the value type from a potential pointer type
+                                QByteArray valueTypeStr = QByteArray(typeName);
                                 int pIndex = typeName.lastIndexOf('*');
                                 if (pIndex != -1)
-                                    typeNameStr = typeName.left(pIndex);
-                                const QMetaType metaType = QMetaType::fromName(typeNameStr);
-                                Q_ASSERT(metaType.id() != QMetaType::UnknownType);
-                                auto object = static_cast<QAxObject*>(qax_createObjectWrapper(metaType.id(), disp));
-                                var = QVariant(metaType, &object);
+                                    valueTypeStr = typeName.left(pIndex);
+
+                                const QMetaType metaValueType = QMetaType::fromName(valueTypeStr);
+                                Q_ASSERT(metaValueType.id() != QMetaType::UnknownType);
+
+                                auto object = static_cast<QAxObject*>(qax_createObjectWrapper(metaValueType.id(), disp));
+
+                                // Return object as the original type
+                                const QMetaType returnType = QMetaType::fromName(typeName);
+                                Q_ASSERT(metaValueType.id() != QMetaType::UnknownType);
+
+                                var = QVariant(returnType, &object);
+
+                                // The result must be a pointer to an instance derived from QObject
+                                Q_ASSERT((var.metaType().flags() & QMetaType::PointerToQObject) != 0);
                             } else {
 #endif
+                                // An IDispatch pointer is requested, no conversion required, just return as QVariant
+                                // containing the pointer.
                                 static const int dispatchId = qRegisterMetaType<IDispatch*>(typeName.constData());
                                 var = QVariant(QMetaType(dispatchId), &disp);
 #ifndef QAX_SERVER
diff --git a/tests/auto/conversion/tst_conversion.cpp b/tests/auto/conversion/tst_conversion.cpp
index 251854b..957ecc4 100644
--- a/tests/auto/conversion/tst_conversion.cpp
+++ b/tests/auto/conversion/tst_conversion.cpp
@@ -51,6 +51,7 @@ private slots:
     void VARIANTToQVariant_IncreasesRefCount_WhenCalledWithQVariantTypeName();
 
     void ObserveThat_VARIANTToQVariant_ReturnsEmptyQVariant_WhenWrappingIDispatchInQAxObjectPtr();
+    void VARIANTToQVariant_CreatesQAxObject_WhenCalledWithMetaTypeId();
 
 private:
     template<typename T>
@@ -427,6 +428,20 @@ void tst_Conversion::ObserveThat_VARIANTToQVariant_ReturnsEmptyQVariant_WhenWrap
     QVERIFY(qVariant.isNull());
 }
 
+void tst_Conversion::VARIANTToQVariant_CreatesQAxObject_WhenCalledWithMetaTypeId()
+{
+    const IDispatchFixture testFixture;
+    QCOMPARE(testFixture.m_iDispatchStub->m_refCount, 2u);
+
+    qRegisterMetaType<QAxObject *>("QAxObject*");
+    qRegisterMetaType<QAxObject>("QAxObject");
+
+    const QVariant qVariant = VARIANTToQVariant(testFixture.m_comVariant, "QAxObject*", QMetaType::fromType<QAxObject*>().id());
+
+    QAxObject *recovered = qVariant.value<QAxObject *>();
+    QVERIFY(recovered != nullptr);
+}
+
 template<typename T>
 void tst_Conversion::addScalarMaxValueRow()
 {
diff --git a/tests/auto/dumpcpp/CMakeLists.txt b/tests/auto/dumpcpp/CMakeLists.txt
index 978af86..a5ff2dc 100644
--- a/tests/auto/dumpcpp/CMakeLists.txt
+++ b/tests/auto/dumpcpp/CMakeLists.txt
@@ -21,3 +21,4 @@ qt_internal_add_test(tst_dumpcpp
 )
 
 qt6_target_typelibs(tst_dumpcpp LIBRARIES "ieframe.dll")
+qt6_target_typelibs(tst_dumpcpp LIBRARIES "msxml6.dll")
diff --git a/tests/auto/dumpcpp/tst_dumpcpp.cpp b/tests/auto/dumpcpp/tst_dumpcpp.cpp
index 2519000..aad0e51 100644
--- a/tests/auto/dumpcpp/tst_dumpcpp.cpp
+++ b/tests/auto/dumpcpp/tst_dumpcpp.cpp
@@ -3,20 +3,53 @@
 
 #include <QtTest/QtTest>
 #include "ieframe.h" // generated header
+#include "msxml6.h"  // generated header
 #include <QApplication>
 
+
+struct XmlFixture
+{
+    MSXML2::DOMDocument60 doc;
+    MSXML2::IXMLDOMNodeList *root;
+
+    const QString xml{
+        R"(
+            <root prop="The root property">
+                The value
+            </root>
+        )" };
+};
+
 class tst_dumpcpp : public QObject
 {
     Q_OBJECT
 
 private slots:
+    void init();
     void toggleAddressBar();
+    void propertyGetter_ReturnsValue_WhenValueIsInt();
+    void propertyGetter_ReturnsValue_WhenValueIsString();
+    void invokeGetter_ReturnsValue_WhenValueInheritsIDispatch();
+    void propertyGetter_ReturnsValue_WhenValueInheritsIDispatch();
+
+    void propertySetter_SetsValue_WhenValueIsVariantInt();
+    void propertySetter_SetsValue_WhenValueIsString();
+    void invoke_SetsValue_WhenValueDerivesFromIDispatch();
+
+private:
+    XmlFixture m_xml;
 };
 
+void tst_dumpcpp::init()
+{
+    m_xml.doc.loadXML(m_xml.xml);
+    m_xml.root = m_xml.doc.childNodes();
+}
+
 // A simple test to verify that an object can be instantiated and interacted with
 void tst_dumpcpp::toggleAddressBar()
 {
-    SHDocVw::WebBrowser* webBrowser = new SHDocVw::WebBrowser;
+    SHDocVw::WebBrowser *webBrowser = new SHDocVw::WebBrowser;
     QVERIFY(webBrowser);
     bool addressBar = webBrowser->AddressBar();
     addressBar = !addressBar;
@@ -25,5 +58,60 @@ void tst_dumpcpp::toggleAddressBar()
     delete webBrowser;
 }
 
+void tst_dumpcpp::propertyGetter_ReturnsValue_WhenValueIsInt()
+{
+    int length = m_xml.root->length();
+    QVERIFY(length == 1);
+}
+
+void tst_dumpcpp::invokeGetter_ReturnsValue_WhenValueInheritsIDispatch()
+{
+    // item(...) takes an argument and is called as a function invocation
+    MSXML2::IXMLDOMNode *firstChild = m_xml.root->item(0);
+    QVERIFY(firstChild);
+}
+
+void tst_dumpcpp::propertyGetter_ReturnsValue_WhenValueInheritsIDispatch()
+{
+    // attributes() takes an argument and is called as property getter
+    MSXML2::IXMLDOMNamedNodeMap *attributes = m_xml.root->item(0)->attributes();
+    QVERIFY(attributes);
+}
+
+void tst_dumpcpp::propertyGetter_ReturnsValue_WhenValueIsString()
+{
+    MSXML2::IXMLDOMNamedNodeMap *attributes = m_xml.root->item(0)->attributes();
+
+    // nodeValue is a property getter
+    QVariant p = attributes->getNamedItem("prop")->nodeValue();
+    QCOMPARE(p, "The root property");
+}
+
+void tst_dumpcpp::propertySetter_SetsValue_WhenValueIsVariantInt()
+{
+    MSXML2::IXMLDOMNamedNodeMap *attributes = m_xml.root->item(0)->attributes();
+    MSXML2::IXMLDOMNode *attribNode = attributes->item(0);
+    attribNode->setNodeValue(QVariant { 42 } );
+
+    QVariant p = attributes->getNamedItem("prop")->nodeValue();
+    QCOMPARE(p, 42);
+}
+
+void tst_dumpcpp::propertySetter_SetsValue_WhenValueIsString()
+{
+    m_xml.root->item(0)->setText("The new value");
+    QCOMPARE(m_xml.root->item(0)->text(), "The new value");
+}
+
+void tst_dumpcpp::invoke_SetsValue_WhenValueDerivesFromIDispatch()
+{
+    MSXML2::IXMLDOMNode *node = m_xml.doc.createNode(MSXML2::NODE_ELEMENT, "sometag", "");
+    node->setText("The new text");
+
+    m_xml.root->item(0)->appendChild(node);
+
+    QCOMPARE(m_xml.root->item(0)->childNodes()->item(1)->text(), "The new text");
+}
+
 QTEST_MAIN(tst_dumpcpp)
 #include "tst_dumpcpp.moc"