Changeset 126976 in webkit


Ignore:
Timestamp:
Aug 29, 2012 3:15:58 AM (12 years ago)
Author:
Simon Hausmann
Message:

[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.

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.
Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r126973 r126976  
     12012-08-22  Simon Hausmann  <simon.hausmann@nokia.com>
     2
     3        [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
     4        https://bugs.webkit.org/show_bug.cgi?id=93897
     5
     6        Reviewed by Kenneth Rohde Christiansen.
     7
     8        * platform/qt/Skipped: Unskip test that is now passing.
     9
    1102012-08-29  Zoltan Arvai  <zarvai@inf.u-szeged.hu>
    211

  • trunk/LayoutTests/platform/qt/Skipped

    r126860 r126976  
    27372737svg/custom/use-instanceRoot-as-event-target.xhtml
    27382738
    2739 # https://bugs.webkit.org/show_bug.cgi?id=93897
    2740 fast/profiler/nested-start-and-stop-profiler.html
    2741 
    27422739# New test introduced in r125648 fast/events/autoscroll-in-textarea.html fails
    27432740# https://bugs.webkit.org/show_bug.cgi?id=94076

  • trunk/Source/WebCore/ChangeLog

    r126975 r126976  
     12012-08-22  Simon Hausmann  <simon.hausmann@nokia.com>
     2
     3        [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
     4        https://bugs.webkit.org/show_bug.cgi?id=93897
     5
     6        Reviewed by Kenneth Rohde Christiansen.
     7
     8        Before r125428 run-time methods (wrapped signals, slots or invokable
     9        functions) were subclasses of JSInternalFunction and therefore real
     10        function objects in the JavaScript sense. r125428 changed them to be
     11        just callable objects, but they did not have Function.prototype as
     12        prototype anymore for example nor was their name correct (resulting in
     13        a layout test failure).
     14
     15        This patch changes run-time methods back to being real function objects
     16        that have a correct name and have Function.prototype in their prototype
     17        change
     18
     19        The objects returned by JSObjectMakeFunctionWithCallbackInjected are
     20        light-weight internal function objects that do not support
     21        JSObject{Set/Get}Private. Therefore we inject our own prototype right
     22        before the Function.prototype prototype, which uses private data to
     23        store a pointer to our C++ QtRuntimeMethod object.  This complicates
     24        the retrieval of the pointer to that instance slightly, which is why
     25        this patch introduces the toRuntimeMethod convenience function that
     26        looks up our prototype first and does a check for type-safety.
     27
     28        At the same time the patch removes the length properties from the
     29        run-time method itself as well as connect/disconnect.  The length
     30        property on a function signifies the number of arguments, but in all
     31        three cases that number is actually variable, because of overloading.
     32        That is why we choose not to expose it in the first place.
     33
     34        In QtInstance we cache the JS wrapper objects for QtRuntimeMethod in a
     35        JSWeakObjectMap. JSWeakObjectMap requires the stored objects to be
     36        either the result of JSObjectMake or the global object of a context ref
     37        (AFAICS), which is ensured using an ASSERT. Objects created via
     38        JSObjectMakeFunctionWithCalllback do not fall into the required
     39        category, cause a failing assertion and can therefore not be stored in
     40        the weak object map.
     41
     42        Consequently this patch removes the use of JSWeakObjectMap again and
     43        goes back to the old way of using the internal Weak<> API, for the time
     44        being. In a future patch the storage will be simplified to not require
     45        the use of a weak object map cache for the run-time methods anymore.
     46
     47        * bridge/qt/qt_instance.cpp: Remove unused WeakMap code.
     48        * bridge/qt/qt_instance.h: Remove method cache.
     49        (QtInstance):
     50        * bridge/qt/qt_runtime.cpp:
     51        (JSC::Bindings::prototypeForSignalsAndSlots):
     52        (JSC::Bindings::QtRuntimeMethod::call):
     53        (JSC::Bindings::QtRuntimeMethod::jsObjectRef):
     54        (JSC::Bindings::QtRuntimeMethod::toRuntimeMethod):
     55        (Bindings):
     56        (JSC::Bindings::QtRuntimeMethod::connectOrDisconnect):
     57        * bridge/qt/qt_runtime.h:
     58        (QtRuntimeMethod): Remove unused member variables.
     59
    1602012-08-29  Ilya Tikhonovsky  <loislo@chromium.org>
    261

  • trunk/Source/WebCore/bridge/qt/qt_instance.cpp

    r126926 r126976  
    4242namespace Bindings {
    4343
    44 static void unusedWeakObjectMapCallback(JSWeakObjectMapRef, void*)
    45 {
    46 }
    47 
    48 WeakMapImpl::WeakMapImpl(JSContextGroupRef group)
    49 {
    50     m_context = JSGlobalContextCreateInGroup(group, 0);
    51     // Deleted by GC when m_context's globalObject gets collected.
    52     m_map = JSWeakObjectMapCreate(m_context, 0, unusedWeakObjectMapCallback);
    53 }
    54 
    55 WeakMapImpl::~WeakMapImpl()
    56 {
    57     JSGlobalContextRelease(m_context);
    58     m_context = 0;
    59     m_map = 0;
    60 }
    61 
    62 typedef HashMap<JSContextGroupRef, RefPtr<WeakMapImpl> > WeakMapSet;
    63 static WeakMapSet weakMaps;
    64 
    65 WeakMap::~WeakMap()
    66 {
    67     // If this is the last WeakMap instance left, then we should remove
    68     // the cached WeakMapImpl from the global weakMaps, too.
    69     if (m_impl && m_impl->refCount() == 2) {
    70         weakMaps.remove(JSContextGetGroup(m_impl->m_context));
    71         ASSERT(m_impl->hasOneRef());
    72     }
    73 }
    74 
    75 void WeakMap::set(JSContextRef context, void *key, JSObjectRef object)
    76 {
    77     if (!m_impl) {
    78         JSContextGroupRef group = JSContextGetGroup(context);
    79         WeakMapSet::AddResult entry = weakMaps.add(group, 0);
    80         if (entry.isNewEntry)
    81             entry.iterator->second = adoptRef(new WeakMapImpl(group));
    82         m_impl = entry.iterator->second;
    83     }
    84     JSWeakObjectMapSet(m_impl->m_context, m_impl->m_map, key, object);
    85 }
    86 
    87 JSObjectRef WeakMap::get(void* key)
    88 {
    89     if (!m_impl)
    90         return 0;
    91     return JSWeakObjectMapGet(m_impl->m_context, m_impl->m_map, key);
    92 }
    93 
    94 void WeakMap::remove(void *key)
    95 {
    96     if (!m_impl)
    97         return;
    98     JSWeakObjectMapRemove(m_impl->m_context, m_impl->m_map, key);
    99 }
    100 
    10144// Cache QtInstances
    10245typedef QMultiHash<void*, QtInstance*> QObjectInstanceMap;

  • trunk/Source/WebCore/bridge/qt/qt_instance.h

    r126301 r126976  
    110110    QObject* m_hashkey;
    111111    mutable QHash<QByteArray, QtRuntimeMethod*> m_methods;
    112     WeakMap m_cachedMethods;
    113112    mutable QHash<QString, QtField*> m_fields;
    114113    ValueOwnership m_ownership;

  • trunk/Source/WebCore/bridge/qt/qt_runtime.cpp

    r126494 r126976  
    13111311{
    13121312    static JSClassDefinition classDef = {
    1313         0, 0, 0, 0, 0, 0,
    1314         0, 0, 0, 0, 0, 0, 0, QtRuntimeMethod::call, 0, 0, 0
     1313        0, kJSClassAttributeNoAutomaticPrototype, 0, 0, 0, 0,
     1314        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
    13151315    };
    13161316    static JSClassRef cls = JSClassCreate(&classDef);
    … …  
    13291329QtRuntimeMethod::~QtRuntimeMethod()
    13301330{
    1331     if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
    1332         JSObjectSetPrivate(cachedWrapper, 0);
    1333     m_instance->m_cachedMethods.remove(this);
    13341331}
    13351332
    13361333JSValueRef QtRuntimeMethod::call(JSContextRef context, JSObjectRef function, JSObjectRef /*thisObject*/, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
    13371334{
    1338     QtRuntimeMethod* d = reinterpret_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
     1335    QtRuntimeMethod* d = toRuntimeMethod(context, function);
    13391336    if (!d) {
    13401337        setException(context, exception, QStringLiteral("cannot call function of deleted runtime method"));
    … …  
    13791376JSObjectRef QtRuntimeMethod::jsObjectRef(JSContextRef context, JSValueRef* exception)
    13801377{
    1381     if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
    1382         return cachedWrapper;
    1383 
    1384     static const JSClassDefinition classDefForConnect = {
    1385         0, 0, "connect", 0, 0, 0,
    1386         0, 0, 0, 0, 0, 0, 0, connect, 0, 0, 0
    1387     };
    1388 
    1389     static const JSClassDefinition classDefForDisconnect = {
    1390         0, 0, "disconnect", 0, 0, 0,
    1391         0, 0, 0, 0, 0, 0, 0, disconnect, 0, 0, 0
    1392     };
    1393 
    1394     static JSClassRef classRefConnect = JSClassCreate(&classDefForConnect);
    1395     static JSClassRef classRefDisconnect = JSClassCreate(&classDefForDisconnect);
    1396     bool isSignal = m_flags & MethodIsSignal;
    1397     JSObjectRef object = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
    1398     JSObjectRef connectFunction = JSObjectMake(context, classRefConnect, this);
    1399     JSObjectRef disconnectFunction = JSObjectMake(context, classRefDisconnect, this);
    1400     JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
     1378    if (m_jsObject)
     1379        return toRef(m_jsObject.get());
    14011380
    14021381    static JSStringRef connectStr = JSStringCreateWithUTF8CString("connect");
    14031382    static JSStringRef disconnectStr = JSStringCreateWithUTF8CString("disconnect");
    1404     static JSStringRef lengthStr = JSStringCreateWithUTF8CString("length");
    1405     static JSStringRef nameStr = JSStringCreateWithUTF8CString("name");
    14061383    JSRetainPtr<JSStringRef> actualNameStr(Adopt, JSStringCreateWithUTF8CString(m_identifier.constData()));
    14071384
    1408     JSObjectSetProperty(context, connectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
    1409     JSObjectSetProperty(context, connectFunction, nameStr, JSValueMakeString(context, connectStr), attributes, exception);
    1410     JSObjectSetProperty(context, disconnectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
    1411     JSObjectSetProperty(context, disconnectFunction, nameStr, JSValueMakeString(context, disconnectStr), attributes, exception);
    1412 
     1385    JSObjectRef object = JSObjectMakeFunctionWithCallback(context, actualNameStr.get(), call);
     1386
     1387    JSObjectRef generalFunctionProto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
     1388    JSObjectRef runtimeMethodProto = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
     1389    JSObjectSetPrototype(context, runtimeMethodProto, generalFunctionProto);
     1390
     1391    JSObjectSetPrototype(context, object, runtimeMethodProto);
     1392
     1393    JSObjectRef connectFunction = JSObjectMakeFunctionWithCallback(context, connectStr, connect);
     1394    JSObjectSetPrototype(context, connectFunction, runtimeMethodProto);
     1395
     1396    JSObjectRef disconnectFunction = JSObjectMakeFunctionWithCallback(context, disconnectStr, disconnect);
     1397    JSObjectSetPrototype(context, disconnectFunction, runtimeMethodProto);
     1398
     1399    const JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
    14131400    JSObjectSetProperty(context, object, connectStr, connectFunction, attributes, exception);
    14141401    JSObjectSetProperty(context, object, disconnectStr, disconnectFunction, attributes, exception);
    1415     JSObjectSetProperty(context, object, lengthStr, JSValueMakeNumber(context, 0), attributes, exception);
    1416     JSObjectSetProperty(context, object, nameStr, JSValueMakeString(context, actualNameStr.get()), attributes, exception);
    1417 
    1418     m_instance->m_cachedMethods.set(context, this, object);
     1402
     1403    m_jsObject = PassWeak<JSObject>(toJS(object));
    14191404
    14201405    return object;
    14211406}
    14221407
     1408QtRuntimeMethod* QtRuntimeMethod::toRuntimeMethod(JSContextRef context, JSObjectRef object)
     1409{
     1410    JSObjectRef proto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
     1411    if (!proto)
     1412        return 0;
     1413    if (!JSValueIsObjectOfClass(context, proto, prototypeForSignalsAndSlots()))
     1414        return 0;
     1415    return static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(proto));
     1416}
     1417
    14231418JSValueRef QtRuntimeMethod::connectOrDisconnect(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect)
    14241419{
    1425     QtRuntimeMethod* d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(thisObject));
     1420    QtRuntimeMethod* d = toRuntimeMethod(context, thisObject);
    14261421    if (!d)
    1427         d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
     1422        d = toRuntimeMethod(context, function);
     1423    if (!d) {
     1424        QString errorStr = QStringLiteral("QtMetaMethod.%1: Cannot connect to/from deleted QObject").arg(connect ?  QStringLiteral("connect") : QStringLiteral("disconnect"));
     1425        setException(context, exception, errorStr);
     1426        return JSValueMakeUndefined(context);
     1427    }
    14281428
    14291429    QString functionName = connect ? QStringLiteral("connect") : QStringLiteral("disconnect");
    … …  
    14611461        // object.signal.connect(someFunction);
    14621462        if (JSObjectIsFunction(context, targetFunction)) {
    1463             if (JSValueIsObjectOfClass(context, targetFunction, prototypeForSignalsAndSlots())) {
    1464                 // object.signal.connect(otherObject.slot);
    1465                 if (QtRuntimeMethod* targetMethod = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(targetFunction)))
    1466                     targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
    1467             }
     1463            // object.signal.connect(otherObject.slot);
     1464            if (QtRuntimeMethod* targetMethod = toRuntimeMethod(context, targetFunction))
     1465                targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
    14681466        } else
    14691467            targetFunction = 0;

  • trunk/Source/WebCore/bridge/qt/qt_runtime.h

    r126301 r126976  
    115115
    116116private:
    117     static const JSStaticFunction connectFunction;
    118     static const JSStaticFunction disconnectFunction;
     117    static QtRuntimeMethod* toRuntimeMethod(JSContextRef, JSObjectRef);
    119118
    120119    static JSValueRef connectOrDisconnect(JSContextRef ctx, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect);
    … …  
    123122    int m_index;
    124123    int m_flags;
     124    Weak<JSObject> m_jsObject;
    125125    QtInstance* m_instance;
    126126};

  • trunk/Source/WebKit/qt/ChangeLog

    r126926 r126976  
     12012-08-22  Simon Hausmann  <simon.hausmann@nokia.com>
     2
     3        [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails
     4        https://bugs.webkit.org/show_bug.cgi?id=93897
     5
     6        Reviewed by Kenneth Rohde Christiansen.
     7
     8        Fixed some test expectations.
     9
     10        * tests/qobjectbridge/tst_qobjectbridge.cpp:
     11        (tst_QObjectBridge::objectDeleted): Since runtime methods are real function objects again, we
     12        can go back to testing Function.prototype.call, as it was done before r125428.
     13        (tst_QObjectBridge::introspectQtMethods_data): Removed tests for the length property.
     14        (tst_QObjectBridge::introspectQtMethods): Changed test expectation of the properties of
     15        run-time methods back to being non-configurable, as before r125428.
     16
    1172012-08-28  Sheriff Bot  <webkit.review.bot@gmail.com>
    218

  • trunk/Source/WebKit/qt/tests/qobjectbridge/tst_qobjectbridge.cpp

    r126301 r126976  
    18801880    QCOMPARE(qobj->intProperty(), 123);
    18811881    qobj->resetQtFunctionInvoked();
    1882     evalJS("bar.myInvokable(bar);");
     1882    evalJS("bar.myInvokable.call(bar);");
    18831883    QCOMPARE(qobj->qtFunctionInvoked(), 0);
    18841884
    … …  
    21492149
    21502150    QTest::newRow("myObject.mySignal")
    2151         << "myObject" << "mySignal" << (QStringList() << "connect" << "disconnect" << "length" << "name");
     2151        << "myObject" << "mySignal" << (QStringList() << "connect" << "disconnect" << "name");
    21522152    QTest::newRow("myObject.mySlot")
    2153         << "myObject" << "mySlot" << (QStringList() << "connect" << "disconnect" << "length" << "name");
     2153        << "myObject" << "mySlot" << (QStringList() << "connect" << "disconnect" << "name");
    21542154    QTest::newRow("myObject.myInvokable")
    2155         << "myObject" << "myInvokable" << (QStringList() << "connect" << "disconnect" << "length" << "name");
     2155        << "myObject" << "myInvokable" << (QStringList() << "connect" << "disconnect" << "name");
    21562156    QTest::newRow("myObject.mySignal.connect")
    2157         << "myObject.mySignal" << "connect" << (QStringList() << "length" << "name");
     2157        << "myObject.mySignal" << "connect" << (QStringList() << "name");
    21582158    QTest::newRow("myObject.mySignal.disconnect")
    2159         << "myObject.mySignal" << "disconnect" << (QStringList() << "length" << "name");
     2159        << "myObject.mySignal" << "disconnect" << (QStringList() << "name");
    21602160}
    21612161
    … …  
    21782178        QCOMPARE(evalJS(QString::fromLatin1("descriptor.value === %0['%1']").arg(methodLookup).arg(name)), sTrue);
    21792179        QCOMPARE(evalJS(QString::fromLatin1("descriptor.enumerable")), sFalse);
    2180         QCOMPARE(evalJS(QString::fromLatin1("descriptor.configurable")), sTrue);
     2180        QCOMPARE(evalJS(QString::fromLatin1("descriptor.configurable")), sFalse);
    21812181    }
    21822182
Note: See TracChangeset for help on using the changeset viewer.