Bug 93476 – [Qt] Port meta method/signal/slot handling in run-time bridge to use JSC C API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Hausmann]

[Qt] Port meta method/signal/slot handling in run-time bridge to use JSC C API

[Simon Hausmann]

Created attachment 157476 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 157476 [details]
Patch

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

Much cleaner though!

> Source/WebCore/ChangeLog:13
> +        The use of functions objects (JSObjectMakeFunction) we loose the ability to store the

By using... ? English is a bit weird

> Source/WebCore/ChangeLog:16
> +

So both use function objects. Maybe this could be rewritten a bit to be more clear

> Source/WebCore/bridge/qt/qt_class.cpp:80
> +    QtRuntimeMethod* method= qtinst->m_methods.value(name);

missing space before =

> Source/WebCore/bridge/qt/qt_class.cpp:82
> +    if (method)
> +        return toJS(method->jsObjectRef(context, exception));

why not just if (QtRuntime...

> Source/WebCore/bridge/qt/qt_class.cpp:95
> +            qtinst->m_methods.insert(name, method);
> +            return toJS(method->jsObjectRef(context, exception));

newline before return?

> Source/WebCore/bridge/qt/qt_instance.cpp:100
>      // clean up (unprotect from gc) the JSValues we've created

Fix comment to be style compliant?

> Source/WebCore/bridge/qt/qt_runtime.cpp:1348
> +QtRuntimeMethod::QtRuntimeMethod(JSContextRef ctx, JSValueRef* exception, QObject* object, const QByteArray& identifier, int index, int flags, QtInstance *instance)

* placement :-)

> Source/WebCore/bridge/qt/qt_runtime.cpp:1378
> +    QVarLengthArray<QVariant, 10> vargs;
> +    void* qargs[11];

Comment why this exact size is used?

> Source/WebCore/bridge/qt/qt_runtime.cpp:1454
> +    // object.method.connect(); object.method.disconnect();

Maybe that comment could be better?

> Source/WebCore/bridge/qt/qt_runtime.cpp:1463
> +    // object.slot.connect(...); object.slot.disconnect(...);
> +    if ((!(d->m_flags & QtRuntimeMethod::MethodIsSignal))) {
> +        setException(context, exception, QStringLiteral("QtMetaMethod.%3: %1::%2() is not a signal").arg(QString::fromUtf8(d->m_object.data()->metaObject()->className())).arg(QString::fromAscii(d->m_identifier)).arg(functionName));

the comment says slot, not signal?

[Simon Hausmann]

All very valid points. Will fix tomorrow. Thanks for the review!

[Simon Hausmann]

Comment on attachment 157476 [details]
Patch

Clearing review, will upload a new patch today.

[Simon Hausmann]

Comment on attachment 157476 [details]
Patch

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

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1378
>> +    void* qargs[11];
> 
> Comment why this exact size is used?

Well spotted, the old code actually had a if (argumentCount > 10) return; check in there that got lost. I'll re-add it, then
this is clearer.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1463
>> +        setException(context, exception, QStringLiteral("QtMetaMethod.%3: %1::%2() is not a signal").arg(QString::fromUtf8(d->m_object.data()->metaObject()->className())).arg(QString::fromAscii(d->m_identifier)).arg(functionName));
> 
> the comment says slot, not signal?

Yeah, you cannot call connect on a slot, you can only call it on a signal.
I'll remove this and the earlier comment, as they're more confusing than helping. The error message that follows the comment does a much better job at explaining this
rare error situation anyway :)

[Simon Hausmann]

Created attachment 157717 [details]
Patch

[Simon Hausmann]

Created attachment 157731 [details]
Patch

Updated patch, give credits to Noam

[Kenneth Rohde Christiansen]

Comment on attachment 157717 [details]
Patch

Thought I am no expert on this, I looked through it a couple of times and it makes sense. I don't know all the details, so you might want someone else to look it through as well

[Simon Hausmann]

(In reply to comment #8)
> (From update of attachment 157717 [details])
> Thought I am no expert on this, I looked through it a couple of times and it makes sense. I don't know all the details, so you might want someone else to look it through as well

Caio, could you help us here with an informal review? :)

[Caio Marcelo de Oliveira Filho]

Comment on attachment 157731 [details]
Patch

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

Overall looks good to me, I have some suggestions and two potential issues that I think need to be fixed, see below.

> Source/WebCore/bridge/qt/qt_class.cpp:110
> +            int flags = m.methodType() == QMetaMethod::Signal ? QtRuntimeMethod::MethodIsSignal : 0;
> +            QtRuntimeMethod* method = new QtRuntimeMethod(context, exception, static_cast<QtInstance*>(inst)->getObject(), normal, index, flags, qtinst);
> +            qtinst->m_methods.insert(name, method);
> +            return toJS(method->jsObjectRef(context, exception));

We could try to avoid this duplication. One possible approach could be first figure out if there's a valid index and then if so run this block of code.

// Sketchy pseudo code.
int index = -1;
if (index = indexOfMethod(normal)) {
    if (isPrivate)
        index = -1;
}

if (index == -1) {
    // Search by basename.
}

if (index == -1) {
    return jsUndefined;
}

// Create runtime method with index.


Optional: I'm also wondering while reading this if we could just "return early" if the exact match is private, simplifying the code further. This would change behavior for cases where our object have a private foo(int) and another public foo(), and we are asking explicitly for obj["foo(int)"]. Currently we return the method for "foo", I think should be OK to return jsUndefined in this case.

> Source/WebCore/bridge/qt/qt_instance.cpp:97
> +    // Clean up (unprotect from gc) the JSValues we've created.

This comment doesn't seem to be truth. If I got this right, we are not GC protecting the methods.

> Source/WebCore/bridge/qt/qt_runtime.cpp:1300
> +    JSClassDefinition classDef = {
> +        0, 0, m_identifier.constData(), prototypeForSignalsAndSlots(), 0, 0,
> +        0, 0, 0, 0, 0, 0, 0, call, 0, 0, 0
> +    };
> +    m_classRef = JSClassCreate(&classDef);

Do we need a different class for each meta method?

> Source/WebCore/bridge/qt/qt_runtime.cpp:1311
> +    QtRuntimeMethod* d = reinterpret_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
> +    QObject* obj = d->m_object;

We might have a problem here.

Without this patch runtime methods are JS values, and our method table stored weak references for it. Even if we cleared our method table, a reference for a method in JS would keep its QtRuntimeMetaMethod alive.

With the patch, QtRuntimeMethod objects themselves are deleted when we clear the method table. So if we still have a reference for that method in JS, and call it, we'll end up here with an invalid QtRuntimeMethod pointer.

One solution I could think of is adding a Null check here for the JSObjectGetPrivate result, and in QtRuntimeMethod destructor, if we have m_jsObject, use JSObjectSetPrivate to clear its private data.

> Source/WebCore/bridge/qt/qt_runtime.cpp:1374
> +    JSStringRef actualNameStr = JSStringCreateWithUTF8CString(m_identifier.constData());

Use JSRetainPtr then you can avoid the JSStringRelease below.

> Source/WebCore/bridge/qt/qt_runtime.cpp:1442
> +        // object.signal.connect(object, someFunction);
> +        targetObject = JSValueToObject(context, arguments[0], exception);
> +        if (JSValueIsObject(context, arguments[1]))
> +            targetFunction = JSValueToObject(context, arguments[1], exception);

In this case we need to check if arguments[1] is really a function.

Also: in the previous version we treated the case in which arguments[1] was a string, for example: 'object.signal.connect(targetObject, "slotName")'. I think we could still do it here. Even better if we had a test for it. :)

[Simon Hausmann]

Comment on attachment 157731 [details]
Patch

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

Great, thanks a lot for the review, Caio! I'm taking this patch now out of the review queue to revise it.

>> Source/WebCore/bridge/qt/qt_class.cpp:110
>> +            return toJS(method->jsObjectRef(context, exception));
> 
> We could try to avoid this duplication. One possible approach could be first figure out if there's a valid index and then if so run this block of code.
> 
> // Sketchy pseudo code.
> int index = -1;
> if (index = indexOfMethod(normal)) {
>     if (isPrivate)
>         index = -1;
> }
> 
> if (index == -1) {
>     // Search by basename.
> }
> 
> if (index == -1) {
>     return jsUndefined;
> }
> 
> // Create runtime method with index.
> 
> 
> Optional: I'm also wondering while reading this if we could just "return early" if the exact match is private, simplifying the code further. This would change behavior for cases where our object have a private foo(int) and another public foo(), and we are asking explicitly for obj["foo(int)"]. Currently we return the method for "foo", I think should be OK to return jsUndefined in this case.

Good idea, I'll play with cleaning this up a little bit.

>> Source/WebCore/bridge/qt/qt_instance.cpp:97
>> +    // Clean up (unprotect from gc) the JSValues we've created.
> 
> This comment doesn't seem to be truth. If I got this right, we are not GC protecting the methods.

That's true, since the methods aren't JS objects anymore.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1300
>> +    m_classRef = JSClassCreate(&classDef);
> 
> Do we need a different class for each meta method?

If we want to store the identifier in it then yes.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1311
>> +    QObject* obj = d->m_object;
> 
> We might have a problem here.
> 
> Without this patch runtime methods are JS values, and our method table stored weak references for it. Even if we cleared our method table, a reference for a method in JS would keep its QtRuntimeMetaMethod alive.
> 
> With the patch, QtRuntimeMethod objects themselves are deleted when we clear the method table. So if we still have a reference for that method in JS, and call it, we'll end up here with an invalid QtRuntimeMethod pointer.
> 
> One solution I could think of is adding a Null check here for the JSObjectGetPrivate result, and in QtRuntimeMethod destructor, if we have m_jsObject, use JSObjectSetPrivate to clear its private data.

That's a good idea. I have to check how this combines with my follow-up patches that change the storage model slightly. But your point seems valid.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1374
>> +    JSStringRef actualNameStr = JSStringCreateWithUTF8CString(m_identifier.constData());
> 
> Use JSRetainPtr then you can avoid the JSStringRelease below.

Yeah, I guess :)

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1442
>> +            targetFunction = JSValueToObject(context, arguments[1], exception);
> 
> In this case we need to check if arguments[1] is really a function.
> 
> Also: in the previous version we treated the case in which arguments[1] was a string, for example: 'object.signal.connect(targetObject, "slotName")'. I think we could still do it here. Even better if we had a test for it. :)

Well spotted! I'll try to fix this.

[Simon Hausmann]

Created attachment 157879 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 157879 [details]
Patch

r=me, but let's have Caio look through it once more and maybe give him a bit of credit in the changelog

[Caio Marcelo de Oliveira Filho]

Comment on attachment 157879 [details]
Patch

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

Looks good to me. :-)

> Source/WebCore/bridge/qt/qt_runtime.cpp:1452
> +            JSValueRef ex = 0;
> +            JSRetainPtr<JSStringRef> functionName(Adopt, JSValueToStringCopy(context, arguments[1], &ex));

Nitpick: "ex"?

[Simon Hausmann]

Committed r125428: <http://trac.webkit.org/changeset/125428>