[Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
authorhausmann@webkit.org <hausmann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Aug 2012 10:15:58 +0000 (10:15 +0000)
committerhausmann@webkit.org <hausmann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Aug 2012 10:15:58 +0000 (10:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=93897

Reviewed by Kenneth Rohde Christiansen.

Source/WebCore:

Before r125428 run-time methods (wrapped signals, slots or invokable
functions) were subclasses of JSInternalFunction and therefore real
function objects in the JavaScript sense. r125428 changed them to be
just callable objects, but they did not have Function.prototype as
prototype anymore for example nor was their name correct (resulting in
a layout test failure).

This patch changes run-time methods back to being real function objects
that have a correct name and have Function.prototype in their prototype
change

The objects returned by JSObjectMakeFunctionWithCallbackInjected are
light-weight internal function objects that do not support
JSObject{Set/Get}Private. Therefore we inject our own prototype right
before the Function.prototype prototype, which uses private data to
store a pointer to our C++ QtRuntimeMethod object.  This complicates
the retrieval of the pointer to that instance slightly, which is why
this patch introduces the toRuntimeMethod convenience function that
looks up our prototype first and does a check for type-safety.

At the same time the patch removes the length properties from the
run-time method itself as well as connect/disconnect.  The length
property on a function signifies the number of arguments, but in all
three cases that number is actually variable, because of overloading.
That is why we choose not to expose it in the first place.

In QtInstance we cache the JS wrapper objects for QtRuntimeMethod in a
JSWeakObjectMap. JSWeakObjectMap requires the stored objects to be
either the result of JSObjectMake or the global object of a context ref
(AFAICS), which is ensured using an ASSERT. Objects created via
JSObjectMakeFunctionWithCalllback do not fall into the required
category, cause a failing assertion and can therefore not be stored in
the weak object map.

Consequently this patch removes the use of JSWeakObjectMap again and
goes back to the old way of using the internal Weak<> API, for the time
being. In a future patch the storage will be simplified to not require
the use of a weak object map cache for the run-time methods anymore.

* bridge/qt/qt_instance.cpp: Remove unused WeakMap code.
* bridge/qt/qt_instance.h: Remove method cache.
(QtInstance):
* bridge/qt/qt_runtime.cpp:
(JSC::Bindings::prototypeForSignalsAndSlots):
(JSC::Bindings::QtRuntimeMethod::call):
(JSC::Bindings::QtRuntimeMethod::jsObjectRef):
(JSC::Bindings::QtRuntimeMethod::toRuntimeMethod):
(Bindings):
(JSC::Bindings::QtRuntimeMethod::connectOrDisconnect):
* bridge/qt/qt_runtime.h:
(QtRuntimeMethod): Remove unused member variables.

Source/WebKit/qt:

Fixed some test expectations.

* tests/qobjectbridge/tst_qobjectbridge.cpp:
(tst_QObjectBridge::objectDeleted): Since runtime methods are real function objects again, we
can go back to testing Function.prototype.call, as it was done before r125428.
(tst_QObjectBridge::introspectQtMethods_data): Removed tests for the length property.
(tst_QObjectBridge::introspectQtMethods): Changed test expectation of the properties of
run-time methods back to being non-configurable, as before r125428.

LayoutTests:

* platform/qt/Skipped: Unskip test that is now passing.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@126976 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/qt/Skipped
Source/WebCore/ChangeLog
Source/WebCore/bridge/qt/qt_instance.cpp
Source/WebCore/bridge/qt/qt_instance.h
Source/WebCore/bridge/qt/qt_runtime.cpp
Source/WebCore/bridge/qt/qt_runtime.h
Source/WebKit/qt/ChangeLog
Source/WebKit/qt/tests/qobjectbridge/tst_qobjectbridge.cpp

index d14f292..ed9745a 100644 (file)
@@ -1,3 +1,12 @@
+2012-08-22  Simon Hausmann  <simon.hausmann@nokia.com>
+
+        [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
+        https://bugs.webkit.org/show_bug.cgi?id=93897
+
+        Reviewed by Kenneth Rohde Christiansen.
+
+        * platform/qt/Skipped: Unskip test that is now passing.
+
 2012-08-29  Zoltan Arvai  <zarvai@inf.u-szeged.hu>
 
         [Qt][WK1] Unreviewd gardening. Skip failing css3 filter test.
index 6e952a2..f1ead43 100644 (file)
@@ -2736,9 +2736,6 @@ fast/js/dfg-compare-final-object-to-final-object-or-other-when-both-proven-final
 # https://bugs.webkit.org/show_bug.cgi?id=93812
 svg/custom/use-instanceRoot-as-event-target.xhtml
 
-# https://bugs.webkit.org/show_bug.cgi?id=93897
-fast/profiler/nested-start-and-stop-profiler.html
-
 # New test introduced in r125648 fast/events/autoscroll-in-textarea.html fails
 # https://bugs.webkit.org/show_bug.cgi?id=94076
 fast/events/autoscroll-in-textarea.html
index bc01446..c3f24ee 100644 (file)
@@ -1,3 +1,62 @@
+2012-08-22  Simon Hausmann  <simon.hausmann@nokia.com>
+
+        [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
+        https://bugs.webkit.org/show_bug.cgi?id=93897
+
+        Reviewed by Kenneth Rohde Christiansen.
+
+        Before r125428 run-time methods (wrapped signals, slots or invokable
+        functions) were subclasses of JSInternalFunction and therefore real
+        function objects in the JavaScript sense. r125428 changed them to be
+        just callable objects, but they did not have Function.prototype as
+        prototype anymore for example nor was their name correct (resulting in
+        a layout test failure).
+
+        This patch changes run-time methods back to being real function objects
+        that have a correct name and have Function.prototype in their prototype
+        change
+
+        The objects returned by JSObjectMakeFunctionWithCallbackInjected are
+        light-weight internal function objects that do not support
+        JSObject{Set/Get}Private. Therefore we inject our own prototype right
+        before the Function.prototype prototype, which uses private data to
+        store a pointer to our C++ QtRuntimeMethod object.  This complicates
+        the retrieval of the pointer to that instance slightly, which is why
+        this patch introduces the toRuntimeMethod convenience function that
+        looks up our prototype first and does a check for type-safety.
+
+        At the same time the patch removes the length properties from the
+        run-time method itself as well as connect/disconnect.  The length
+        property on a function signifies the number of arguments, but in all
+        three cases that number is actually variable, because of overloading.
+        That is why we choose not to expose it in the first place.
+
+        In QtInstance we cache the JS wrapper objects for QtRuntimeMethod in a
+        JSWeakObjectMap. JSWeakObjectMap requires the stored objects to be
+        either the result of JSObjectMake or the global object of a context ref
+        (AFAICS), which is ensured using an ASSERT. Objects created via
+        JSObjectMakeFunctionWithCalllback do not fall into the required
+        category, cause a failing assertion and can therefore not be stored in
+        the weak object map.
+
+        Consequently this patch removes the use of JSWeakObjectMap again and
+        goes back to the old way of using the internal Weak<> API, for the time
+        being. In a future patch the storage will be simplified to not require
+        the use of a weak object map cache for the run-time methods anymore.
+
+        * bridge/qt/qt_instance.cpp: Remove unused WeakMap code.
+        * bridge/qt/qt_instance.h: Remove method cache.
+        (QtInstance):
+        * bridge/qt/qt_runtime.cpp:
+        (JSC::Bindings::prototypeForSignalsAndSlots):
+        (JSC::Bindings::QtRuntimeMethod::call):
+        (JSC::Bindings::QtRuntimeMethod::jsObjectRef):
+        (JSC::Bindings::QtRuntimeMethod::toRuntimeMethod):
+        (Bindings):
+        (JSC::Bindings::QtRuntimeMethod::connectOrDisconnect):
+        * bridge/qt/qt_runtime.h:
+        (QtRuntimeMethod): Remove unused member variables.
+
 2012-08-29  Ilya Tikhonovsky  <loislo@chromium.org>
 
         Unreviewed: Single line build fix.
index b9a34ac..0412532 100644 (file)
 namespace JSC {
 namespace Bindings {
 
-static void unusedWeakObjectMapCallback(JSWeakObjectMapRef, void*)
-{
-}
-
-WeakMapImpl::WeakMapImpl(JSContextGroupRef group)
-{
-    m_context = JSGlobalContextCreateInGroup(group, 0);
-    // Deleted by GC when m_context's globalObject gets collected.
-    m_map = JSWeakObjectMapCreate(m_context, 0, unusedWeakObjectMapCallback);
-}
-
-WeakMapImpl::~WeakMapImpl()
-{
-    JSGlobalContextRelease(m_context);
-    m_context = 0;
-    m_map = 0;
-}
-
-typedef HashMap<JSContextGroupRef, RefPtr<WeakMapImpl> > WeakMapSet;
-static WeakMapSet weakMaps;
-
-WeakMap::~WeakMap()
-{
-    // If this is the last WeakMap instance left, then we should remove
-    // the cached WeakMapImpl from the global weakMaps, too.
-    if (m_impl && m_impl->refCount() == 2) {
-        weakMaps.remove(JSContextGetGroup(m_impl->m_context));
-        ASSERT(m_impl->hasOneRef());
-    }
-}
-
-void WeakMap::set(JSContextRef context, void *key, JSObjectRef object)
-{
-    if (!m_impl) {
-        JSContextGroupRef group = JSContextGetGroup(context);
-        WeakMapSet::AddResult entry = weakMaps.add(group, 0);
-        if (entry.isNewEntry)
-            entry.iterator->second = adoptRef(new WeakMapImpl(group));
-        m_impl = entry.iterator->second;
-    }
-    JSWeakObjectMapSet(m_impl->m_context, m_impl->m_map, key, object);
-}
-
-JSObjectRef WeakMap::get(void* key)
-{
-    if (!m_impl)
-        return 0;
-    return JSWeakObjectMapGet(m_impl->m_context, m_impl->m_map, key);
-}
-
-void WeakMap::remove(void *key)
-{
-    if (!m_impl)
-        return;
-    JSWeakObjectMapRemove(m_impl->m_context, m_impl->m_map, key);
-}
-
 // Cache QtInstances
 typedef QMultiHash<void*, QtInstance*> QObjectInstanceMap;
 static QObjectInstanceMap cachedInstances;
index e5d2a93..54df34b 100644 (file)
@@ -109,7 +109,6 @@ private:
     QPointer<QObject> m_object;
     QObject* m_hashkey;
     mutable QHash<QByteArray, QtRuntimeMethod*> m_methods;
-    WeakMap m_cachedMethods;
     mutable QHash<QString, QtField*> m_fields;
     ValueOwnership m_ownership;
 };
index 785e3a0..0db5a87 100644 (file)
@@ -1310,8 +1310,8 @@ static int findSignalIndex(const QMetaObject* meta, int initialIndex, QByteArray
 static JSClassRef prototypeForSignalsAndSlots()
 {
     static JSClassDefinition classDef = {
-        0, 0, 0, 0, 0, 0,
-        0, 0, 0, 0, 0, 0, 0, QtRuntimeMethod::call, 0, 0, 0
+        0, kJSClassAttributeNoAutomaticPrototype, 0, 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
     };
     static JSClassRef cls = JSClassCreate(&classDef);
     return cls;
@@ -1328,14 +1328,11 @@ QtRuntimeMethod::QtRuntimeMethod(JSContextRef ctx, QObject* object, const QByteA
 
 QtRuntimeMethod::~QtRuntimeMethod()
 {
-    if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
-        JSObjectSetPrivate(cachedWrapper, 0);
-    m_instance->m_cachedMethods.remove(this);
 }
 
 JSValueRef QtRuntimeMethod::call(JSContextRef context, JSObjectRef function, JSObjectRef /*thisObject*/, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
 {
-    QtRuntimeMethod* d = reinterpret_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
+    QtRuntimeMethod* d = toRuntimeMethod(context, function);
     if (!d) {
         setException(context, exception, QStringLiteral("cannot call function of deleted runtime method"));
         return JSValueMakeUndefined(context);
@@ -1378,53 +1375,56 @@ JSValueRef QtRuntimeMethod::disconnect(JSContextRef context, JSObjectRef functio
 
 JSObjectRef QtRuntimeMethod::jsObjectRef(JSContextRef context, JSValueRef* exception)
 {
-    if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
-        return cachedWrapper;
-
-    static const JSClassDefinition classDefForConnect = {
-        0, 0, "connect", 0, 0, 0,
-        0, 0, 0, 0, 0, 0, 0, connect, 0, 0, 0
-    };
-
-    static const JSClassDefinition classDefForDisconnect = {
-        0, 0, "disconnect", 0, 0, 0,
-        0, 0, 0, 0, 0, 0, 0, disconnect, 0, 0, 0
-    };
-
-    static JSClassRef classRefConnect = JSClassCreate(&classDefForConnect);
-    static JSClassRef classRefDisconnect = JSClassCreate(&classDefForDisconnect);
-    bool isSignal = m_flags & MethodIsSignal;
-    JSObjectRef object = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
-    JSObjectRef connectFunction = JSObjectMake(context, classRefConnect, this);
-    JSObjectRef disconnectFunction = JSObjectMake(context, classRefDisconnect, this);
-    JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
+    if (m_jsObject)
+        return toRef(m_jsObject.get());
 
     static JSStringRef connectStr = JSStringCreateWithUTF8CString("connect");
     static JSStringRef disconnectStr = JSStringCreateWithUTF8CString("disconnect");
-    static JSStringRef lengthStr = JSStringCreateWithUTF8CString("length");
-    static JSStringRef nameStr = JSStringCreateWithUTF8CString("name");
     JSRetainPtr<JSStringRef> actualNameStr(Adopt, JSStringCreateWithUTF8CString(m_identifier.constData()));
 
-    JSObjectSetProperty(context, connectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
-    JSObjectSetProperty(context, connectFunction, nameStr, JSValueMakeString(context, connectStr), attributes, exception);
-    JSObjectSetProperty(context, disconnectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
-    JSObjectSetProperty(context, disconnectFunction, nameStr, JSValueMakeString(context, disconnectStr), attributes, exception);
+    JSObjectRef object = JSObjectMakeFunctionWithCallback(context, actualNameStr.get(), call);
+
+    JSObjectRef generalFunctionProto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
+    JSObjectRef runtimeMethodProto = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
+    JSObjectSetPrototype(context, runtimeMethodProto, generalFunctionProto);
+
+    JSObjectSetPrototype(context, object, runtimeMethodProto);
+
+    JSObjectRef connectFunction = JSObjectMakeFunctionWithCallback(context, connectStr, connect);
+    JSObjectSetPrototype(context, connectFunction, runtimeMethodProto);
+
+    JSObjectRef disconnectFunction = JSObjectMakeFunctionWithCallback(context, disconnectStr, disconnect);
+    JSObjectSetPrototype(context, disconnectFunction, runtimeMethodProto);
 
+    const JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
     JSObjectSetProperty(context, object, connectStr, connectFunction, attributes, exception);
     JSObjectSetProperty(context, object, disconnectStr, disconnectFunction, attributes, exception);
-    JSObjectSetProperty(context, object, lengthStr, JSValueMakeNumber(context, 0), attributes, exception);
-    JSObjectSetProperty(context, object, nameStr, JSValueMakeString(context, actualNameStr.get()), attributes, exception);
 
-    m_instance->m_cachedMethods.set(context, this, object);
+    m_jsObject = PassWeak<JSObject>(toJS(object));
 
     return object;
 }
 
+QtRuntimeMethod* QtRuntimeMethod::toRuntimeMethod(JSContextRef context, JSObjectRef object)
+{
+    JSObjectRef proto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
+    if (!proto)
+        return 0;
+    if (!JSValueIsObjectOfClass(context, proto, prototypeForSignalsAndSlots()))
+        return 0;
+    return static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(proto));
+}
+
 JSValueRef QtRuntimeMethod::connectOrDisconnect(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect)
 {
-    QtRuntimeMethod* d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(thisObject));
+    QtRuntimeMethod* d = toRuntimeMethod(context, thisObject);
     if (!d)
-        d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
+        d = toRuntimeMethod(context, function);
+    if (!d) {
+        QString errorStr = QStringLiteral("QtMetaMethod.%1: Cannot connect to/from deleted QObject").arg(connect ?  QStringLiteral("connect") : QStringLiteral("disconnect"));
+        setException(context, exception, errorStr);
+        return JSValueMakeUndefined(context);
+    }
 
     QString functionName = connect ? QStringLiteral("connect") : QStringLiteral("disconnect");
 
@@ -1460,11 +1460,9 @@ JSValueRef QtRuntimeMethod::connectOrDisconnect(JSContextRef context, JSObjectRe
 
         // object.signal.connect(someFunction);
         if (JSObjectIsFunction(context, targetFunction)) {
-            if (JSValueIsObjectOfClass(context, targetFunction, prototypeForSignalsAndSlots())) {
-                // object.signal.connect(otherObject.slot);
-                if (QtRuntimeMethod* targetMethod = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(targetFunction)))
-                    targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
-            }
+            // object.signal.connect(otherObject.slot);
+            if (QtRuntimeMethod* targetMethod = toRuntimeMethod(context, targetFunction))
+                targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
         } else
             targetFunction = 0;
     } else {
index ed0f4cf..6f7543d 100644 (file)
@@ -114,14 +114,14 @@ public:
     const QByteArray& name() { return m_identifier; }
 
 private:
-    static const JSStaticFunction connectFunction;
-    static const JSStaticFunction disconnectFunction;
+    static QtRuntimeMethod* toRuntimeMethod(JSContextRef, JSObjectRef);
 
     static JSValueRef connectOrDisconnect(JSContextRef ctx, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect);
     QPointer<QObject> m_object;
     QByteArray m_identifier;
     int m_index;
     int m_flags;
+    Weak<JSObject> m_jsObject;
     QtInstance* m_instance;
 };
 
index b4122c8..c511bb4 100644 (file)
@@ -1,3 +1,19 @@
+2012-08-22  Simon Hausmann  <simon.hausmann@nokia.com>
+
+        [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
+        https://bugs.webkit.org/show_bug.cgi?id=93897
+
+        Reviewed by Kenneth Rohde Christiansen.
+
+        Fixed some test expectations.
+
+        * tests/qobjectbridge/tst_qobjectbridge.cpp:
+        (tst_QObjectBridge::objectDeleted): Since runtime methods are real function objects again, we
+        can go back to testing Function.prototype.call, as it was done before r125428.
+        (tst_QObjectBridge::introspectQtMethods_data): Removed tests for the length property.
+        (tst_QObjectBridge::introspectQtMethods): Changed test expectation of the properties of
+        run-time methods back to being non-configurable, as before r125428.
+
 2012-08-28  Sheriff Bot  <webkit.review.bot@gmail.com>
 
         Unreviewed, rolling out r126914.
index 2ce791d..13d98af 100644 (file)
@@ -1879,7 +1879,7 @@ void tst_QObjectBridge::objectDeleted()
     evalJS("bar.intProperty = 123;");
     QCOMPARE(qobj->intProperty(), 123);
     qobj->resetQtFunctionInvoked();
-    evalJS("bar.myInvokable(bar);");
+    evalJS("bar.myInvokable.call(bar);");
     QCOMPARE(qobj->qtFunctionInvoked(), 0);
 
     // do this, to ensure that we cache that it implements call
@@ -2148,15 +2148,15 @@ void tst_QObjectBridge::introspectQtMethods_data()
     QTest::addColumn<QStringList>("expectedPropertyNames");
 
     QTest::newRow("myObject.mySignal")
-        << "myObject" << "mySignal" << (QStringList() << "connect" << "disconnect" << "length" << "name");
+        << "myObject" << "mySignal" << (QStringList() << "connect" << "disconnect" << "name");
     QTest::newRow("myObject.mySlot")
-        << "myObject" << "mySlot" << (QStringList() << "connect" << "disconnect" << "length" << "name");
+        << "myObject" << "mySlot" << (QStringList() << "connect" << "disconnect" << "name");
     QTest::newRow("myObject.myInvokable")
-        << "myObject" << "myInvokable" << (QStringList() << "connect" << "disconnect" << "length" << "name");
+        << "myObject" << "myInvokable" << (QStringList() << "connect" << "disconnect" << "name");
     QTest::newRow("myObject.mySignal.connect")
-        << "myObject.mySignal" << "connect" << (QStringList() << "length" << "name");
+        << "myObject.mySignal" << "connect" << (QStringList() << "name");
     QTest::newRow("myObject.mySignal.disconnect")
-        << "myObject.mySignal" << "disconnect" << (QStringList() << "length" << "name");
+        << "myObject.mySignal" << "disconnect" << (QStringList() << "name");
 }
 
 void tst_QObjectBridge::introspectQtMethods()
@@ -2177,7 +2177,7 @@ void tst_QObjectBridge::introspectQtMethods()
         QCOMPARE(evalJS("descriptor.set"), sUndefined);
         QCOMPARE(evalJS(QString::fromLatin1("descriptor.value === %0['%1']").arg(methodLookup).arg(name)), sTrue);
         QCOMPARE(evalJS(QString::fromLatin1("descriptor.enumerable")), sFalse);
-        QCOMPARE(evalJS(QString::fromLatin1("descriptor.configurable")), sTrue);
+        QCOMPARE(evalJS(QString::fromLatin1("descriptor.configurable")), sFalse);
     }
 
     QVERIFY(evalJSV("var props=[]; for (var p in myObject.deleteLater) {props.push(p);}; props.sort()").toStringList().isEmpty());