Bug 104540 – [Qt] JS bridge does not transmit QVariants anymore in Qt5

• Component: WebKit Qt
• Status: RESOLVED FIXED

Windows 7 and Windows XP
Visual Studio 2010
Qt5 RC
(I can not test if any other platforms are affected.)

In Qt 4.7.x and 4.8.x JSON object were transmitted as QVariant into the C++ Slot:


---JavaScript extract:---
var jsonTest = {
    service: "LogService",
    type: "request",
    methodName: "logIt",
    count: performCounter
};
this.Bridge.slotQVariant(jsonTest);


---C++ extract---
void JavaScriptConnection::slotQVariant(QVariant data)
{
   //... data can be converted to QVariantMap
}

This worked fine, but with Qt5 we get the error 'Trying to construct an instance of an invalid type, type id: 732670891'. It helps to make in JS a 'JSON.stringify(jsonTest)' and to expect a QString in the C++ Slot:

---JavaScript extract:---
var jsonTest = {
    //... see above
};
this.Bridge.slotQString(JSON.stringify(jsonTest));


---C++ extract---
void JavaScriptConnection::slotQString(QString data)
{
   //...
}


However, this behavior change seems not correct, because (first) it breaks compatability for no reason and (second) it has performance drawbacks on C++ side, because the QString has to be converted.
[Furthermore there's a severe speed decrease, and I do not mean the mentioned conversion, when using the bridge several 10000s of times, but I will post this as a different bug]

As attachment I added a VS2010 project. It's stripped down to the bare problem, so you can hopefully and quickly see the problem. Only the Release build is configured correctly. There is some time measurement on C++ side, but this is of no relevance here.

Created attachment 178525 [details]
VS2010 solution for JS Bridge problems with QVariant and performance measurements

The error "Trying to construct..." is only shown when you start the sample program from within Visual Studio. If you start the program normally, just nothing happens (at first glance, nothing is shown, but internally nothing works). To make the error obvious in everycase just add the following line to the C++ slot:

void JavaScriptConnection::slotQVariant(QVariant dataString)
{
    QVariantMap myVMap = dataString.toMap(); //added, to make the changed behaviour obvious
    //....
}

With this line added (this is what works in Qt 4.7 and Qt 4.8, but no longer in Qt 5) you can see an error if you start the program from outside Visual Studio, too.

[Michael Brüning]

The problem here is not with QVariants, but with QVariantMaps. 

For some reason, in case of a QVariantMap, the resulting QVariantMap is not converted correctly in QMetaObject::metcall when it is called from JSValueRef QtRuntimeMethod::call in Source/WebCore/bridge/qt/qt_runtime.cpp . I suspect that something has changed in the way QVariantMap pointers are handled by the QVariant constructor / debugging this atm.

[Michael Brüning]

A temporary workaround until we have found a proper fix is to explicitly declare the parameter for the slot as a QVariantMap instead of QVariant if one can be sure that a JSON object will be returned.

I can give the temporary workaround a try. HOWEVER, the problem already occured when expecting the QVariant as parmeter -- without converting it to a QVariantMap afterwards. The QVariantMap conversion just made the problem more obvious (see my comment #2, I hope that comment was clear enough).

[Michael Brüning]

(In reply to comment #5)
> I can give the temporary workaround a try. HOWEVER, the problem already occured when expecting the QVariant as parmeter -- without converting it to a QVariantMap afterwards. The QVariantMap conversion just made the problem more obvious (see my comment #2, I hope that comment was clear enough).

Your comment was clear enough, no worries :). 
Actually, expecting a QVariant parameter in the signature of the slot when a QVariantMap is being returned is what makes it not work. Whatever happens to that  QVariant in the slot afterwards is not really relevant, as the corruption / conversion error from QVariantMap to QVariant occurs inside the JS bridge code before the slot is even called. 

So when returning a JSON object through the bridge, this:

void MyObject::slotHandleJSONObject(QVariantMap map) // okay, no conversion from QVariantMap to QVariant
{
   // process data from map
   ...
}

would work, while this:

void MyObject::slotHandleJSONObject(QVariant variant) // corrupted by the conversion from QVariantMap to QVariant inside the JS bridge
{
   QVariantMap map = variant.topMap(); // contains corrupted data
}

Anyway, this needs to be fixed within the JS bridge / Qt, the solution outlined above is just for temporarily making it work until that fix rolls in...

(In reply to comment #6)
Okay, I understand, thanks for your explanation.
I applied your temporary fix and it works -- good thing that we (so far) only send JSON.
However (there's always a however, sorry), with the changed C++ slot to QVariantMap I could now test the same program in Qt 4.8.4 and Qt 5 RC1:
I saw that the Qt5 performance of the JS bridge is much worse (see how I measure time in the slots in my first attachement). When sending 100000 JSONs objects I need with Qt 4.8.4 approximately 700-800 ms, while it takes in Qt5 1400-1500 ms. Maybe this is side-effect of the QVariant problem, maybe some other problem. I know, sending 100000 JSON object is not very realistic, but a doubling in time should be worth a little investigation... 
What do we do about this? Report new bug? Shall I wait for the fix and retest the timing again?

[Michael Brüning]

(In reply to comment #7)
> (In reply to comment #6)
> Okay, I understand, thanks for your explanation.
> I applied your temporary fix and it works -- good thing that we (so far) only send JSON.
> However (there's always a however, sorry), with the changed C++ slot to QVariantMap I could now test the same program in Qt 4.8.4 and Qt 5 RC1:
> I saw that the Qt5 performance of the JS bridge is much worse (see how I measure time in the slots in my first attachement). When sending 100000 JSONs objects I need with Qt 4.8.4 approximately 700-800 ms, while it takes in Qt5 1400-1500 ms. Maybe this is side-effect of the QVariant problem, maybe some other problem. I know, sending 100000 JSON object is not very realistic, but a doubling in time should be worth a little investigation... 
> What do we do about this? Report new bug? Shall I wait for the fix and retest the timing again?

I agree that a doubling in time is undesirable to say the least. I'd suggest waiting for the fix for now, though.

[Simon Hausmann]

Yeah, let's treat those as two different things. There's a bug that we need to fix clearly (we want to retain behavioural compatibility generally).

Performance is another aspect. We did some refactoring of the bridge code to replace the use of ever changing internal API with "external" API, the code became a real maintenance burden for the project. That work isn't complete yet though, there's more to be done. Unfortunately we're going to have to trade _some_ performance for the reduce maintenance, but a factor of two would be good to avoid. Don't hesitate to file a second bug for that for a separate investigation.

[Michael Brüning]

Created attachment 179472 [details]
Reduced test case.

The passing of QVariantMap "raw" pointers seems to have changed internally from Qt4 to Qt5.

I am attaching a reduced test case of what happens in the Qt JS bridge. This test case works fine with Qt4, but on Qt5 passing the QVariantMap inside a QVariant does not work.

[Jędrzej Nowacki]

I investigated the reduced test case, it uses QMetaObject::metacall (which is a  private btw.) a bit differently then qt core. If you run the test in valgrind you would see that the string version is broken too. Slots taking QVariant as an argument have different code path, changing initialization code too

qargs[1] = &mapInVariant;
qargs[1] = &stringInVariant;

would fix the example.

Checkout QMetaProperty::read(const QObject *object) implementation (qmetaobject.cpp) to see the correct usage of QMetaObject::metacall, it contains something like:

    QVariant value;
    void *argv[] = { 0, &value, &status };
    if (t == QMetaType::QVariant) {
        argv[0] = &value;
    } else {
        value = QVariant(t, (void*)0);
        argv[0] = value.data();
    }

[Michael Brüning]

This approach did not fix it for me and when I tried it, the string example actually worked (at least outside of valgrind). What seems to be the problem is that the data() method when called on a QVariant that holds a QVariantMap seems to return a pointer to the QMap and not the address of the QVariant as it is the case for other types and hence the QVariants / QVariantMaps subsequently constructed/cast from these void pointers contain garbage data (which would also explain why there's different invalid types and sometimes even segfaults when trying to do this).

I made a patch based on how objects are passed around in the generated code for signals and slots and that seems to work.

[Michael Brüning]

Created attachment 179762 [details]
Patch

[Jędrzej Nowacki]

(In reply to comment #12)
> This approach did not fix it for me and when I tried it, the string example actually worked (at least outside of valgrind). What seems to be the problem is that the data() method when called on a QVariant that holds a QVariantMap seems to return a pointer to the QMap and not the address of the QVariant as it is the case for other types and hence the QVariants / QVariantMaps subsequently constructed/cast from these void pointers contain garbage data (which would also explain why there's different invalid types and sometimes even segfaults when trying to do this).
> 
> I made a patch based on how objects are passed around in the generated code for signals and slots and that seems to work.

I said that valgrind shows invalid read in QString case, you can not say that it works by reading undefined state in memory. In your example you were doing equivalent of reinterpret_cast<QVariant>(QString("foo")) which happens to "work" because of optimization in QVariant which copy construct QString into the variant itself.

QVariant::data() always return data do encapsulated data
QVariant(QString("foo"))::data would return a pointer to the QString
QVariant(CustomType())::data would return a pointer to the CustomType

__but__

QVariant has an optimization that allows to not store QVariant in a QVariant. That means that *QVariant(QVariant(QVariant(1)))::data() is exactly the same as *QVariant(1)::data(). It is not new in Qt5 it was like that in Qt4 too. Explicit wrapping QVariant inside QVariant is possible but tricky (you can use something like QVariant(QMetaType::QVariant, &otherVariant).

I do not think your patch works, because it doesn't allocate enough memory for anything bigger then QVariant. Consider to use information from QMetaType about size (QMetaType::sizeof) to allocate enough memory, it may be faster, because it would avoid allocation in most cases. Using QVariant for memory allocation is soo Qt4.0 :-)

[Michael Brüning]

Created attachment 179929 [details]
Valgrind output for string test case only.

Thanks for the comment, the patch did fix the example test case, but caused problems with the API tests.
The string example does not show any invalid reads for me when I run it through valgrind (see attached output if interested). 
Anyway, I'll continue investigating a fix.

[Simon Hausmann]

Ok, I investigated further and tried the provided test case (first attachment). The bug is in fact exactly like Jędrzej pointed out in comment #11:

The moc generated code we end up calling with the metacall() does expect the void* pointers in the argument array to be pointers to the _final_ types. So if a function takes an int as argument, args[1] should be an pointer to an int (args[0] is the pointer to the return value). If the function takes a QVariant, then it must be a pointer to a QVariant, not its data().

The only exception to that is the handling of the JavaScript null type, which we currently incorrectly map to a default constructed QVariant but _should_ map to t a QVariant(QMetaType::VoidStar, 0) and leave QVariant() for "undefined". This is also how it's done in QML. That however is a separate issue and a slight behavioural change that I'll propose for Qt 5.1. The issue of this bug - the QVariant slot not being called - is fixable easily for 5.0.1.

I have a patch that I'll upload shortly.

[Simon Hausmann]

Created attachment 180175 [details]
Patch

[Michael Brüning]

Comment on attachment 180175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180175&action=review

I had a local patch that basically did the same but checked for userType as Jędrzej suggested, but using chosenTypes makes it simpler, provided it also catches the VariantMap, List and Hash :). I'll check in the office tomorrow.

> Source/WebCore/bridge/qt/qt_runtime.cpp:1179
> +            if (chosenTypes[i].isVariant() && !dataPtr)

Can we be sure that dataPtr will be null in this case? I am pretty sure that in the case of the embedded QVariantMaps, data() returned a pointer to embedded Map...

[Michael Brüning]

Comment on attachment 180175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180175&action=review

I had a local patch that basically did the same but checked for userType as Jędrzej suggested, but using chosenTypes makes it simpler, provided it also catches the VariantMap, List and Hash :). I'll check in the office tomorrow.

> Source/WebCore/bridge/qt/qt_runtime.cpp:1179
> +            if (chosenTypes[i].isVariant() && !dataPtr)

Can we be sure that dataPtr will be null in this case? I am pretty sure that in the case of the embedded QVariantMaps, data() returned a pointer to embedded Map...

[Simon Hausmann]

(In reply to comment #19)
> (From update of attachment 180175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180175&action=review
> 
> I had a local patch that basically did the same but checked for userType as Jędrzej suggested, but using chosenTypes makes it simpler, provided it also 
catches the VariantMap, List and Hash :). I'll check in the office tomorrow.

Good, if we came up with the same then that's a good sign ;)
 
> > Source/WebCore/bridge/qt/qt_runtime.cpp:1179
> > +            if (chosenTypes[i].isVariant() && !dataPtr)
> 
> Can we be sure that dataPtr will be null in this case? I am pretty sure that in the case of the embedded QVariantMaps, data() returned a pointer to embedded Map...

I think in case of variant maps and lists this is still correct. QVariantMap is a typedef to a QMap<QString, QVariant> and QVariantList is a typedef to a QList<QVariant>. If you think about the calling convention, then it's clear that the void* _has_ to be a pointer to the QMap or QList, and that's what data() does return.

[Simon Hausmann]

Comment on attachment 180175 [details]
Patch

Clearing review on this one. There's a bug with the refcounting / ownership.

[Simon Hausmann]

Created attachment 180326 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 180326 [details]
Patch

LGTM, good description

[Simon Hausmann]

Committed r138247: <http://trac.webkit.org/changeset/138247>

[Simon Hausmann]

Created attachment 185767 [details]
Patch series to speed up the bridge code path used in the original test case. Needs further work.

[Allan Sandfeld Jensen]

(In reply to comment #25)
> Created an attachment (id=185767) [details]
> Patch series to speed up the bridge code path used in the original test case. Needs further work.

Uhh, there is some very nice stuff in there.