Bug 93476

Created attachment 157476 [details] Patch

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?

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

Comment on attachment 157476 [details] Patch Clearing review, will upload a new patch today.

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 :)

Created attachment 157717 [details] Patch

Created attachment 157731 [details] Patch Updated patch, give credits to Noam

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

(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? :)

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. :)

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.

Created attachment 157879 [details] Patch

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

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"?

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