Changeset 126287 in webkit


Ignore:
Timestamp:
Aug 22, 2012 3:42:10 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

    r126286 r126287  
     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-22  Christophe Dumez  <christophe.dumez@intel.com>
    211

  • trunk/LayoutTests/platform/qt/Skipped

    r126282 r126287  
    27312731svg/custom/use-instanceRoot-as-event-target.xhtml
    27322732
    2733 # https://bugs.webkit.org/show_bug.cgi?id=93897
    2734 fast/profiler/nested-start-and-stop-profiler.html
    2735 
    27362733# New test introduced in r125648 fast/events/autoscroll-in-textarea.html fails
    27372734# https://bugs.webkit.org/show_bug.cgi?id=94076

  • trunk/Source/WebCore/ChangeLog

    r126286 r126287  
     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-22  Christophe Dumez  <christophe.dumez@intel.com>
    261

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

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

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

    r125873 r126287  
    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

    r126161 r126287  
    12831283{
    12841284    static JSClassDefinition classDef = {
    1285         0, 0, 0, 0, 0, 0,
    1286         0, 0, 0, 0, 0, 0, 0, QtRuntimeMethod::call, 0, 0, 0
     1285        0, kJSClassAttributeNoAutomaticPrototype, 0, 0, 0, 0,
     1286        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
    12871287    };
    12881288    static JSClassRef cls = JSClassCreate(&classDef);
    … …  
    13011301QtRuntimeMethod::~QtRuntimeMethod()
    13021302{
    1303     if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
    1304         JSObjectSetPrivate(cachedWrapper, 0);
    1305     m_instance->m_cachedMethods.remove(this);
    13061303}
    13071304
    13081305JSValueRef QtRuntimeMethod::call(JSContextRef context, JSObjectRef function, JSObjectRef /*thisObject*/, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
    13091306{
    1310     QtRuntimeMethod* d = reinterpret_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
     1307    QtRuntimeMethod* d = toRuntimeMethod(context, function);
    13111308    if (!d) {
    13121309        setException(context, exception, QStringLiteral("cannot call function of deleted runtime method"));
    … …  
    13511348JSObjectRef QtRuntimeMethod::jsObjectRef(JSContextRef context, JSValueRef* exception)
    13521349{
    1353     if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
    1354         return cachedWrapper;
    1355 
    1356     static const JSClassDefinition classDefForConnect = {
    1357         0, 0, "connect", 0, 0, 0,
    1358         0, 0, 0, 0, 0, 0, 0, connect, 0, 0, 0
    1359     };
    1360 
    1361     static const JSClassDefinition classDefForDisconnect = {
    1362         0, 0, "disconnect", 0, 0, 0,
    1363         0, 0, 0, 0, 0, 0, 0, disconnect, 0, 0, 0
    1364     };
    1365 
    1366     static JSClassRef classRefConnect = JSClassCreate(&classDefForConnect);
    1367     static JSClassRef classRefDisconnect = JSClassCreate(&classDefForDisconnect);
    1368     bool isSignal = m_flags & MethodIsSignal;
    1369     JSObjectRef object = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
    1370     JSObjectRef connectFunction = JSObjectMake(context, classRefConnect, this);
    1371     JSObjectRef disconnectFunction = JSObjectMake(context, classRefDisconnect, this);
    1372     JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
     1350    if (m_jsObject)
     1351        return toRef(m_jsObject.get());
    13731352
    13741353    static JSStringRef connectStr = JSStringCreateWithUTF8CString("connect");
    13751354    static JSStringRef disconnectStr = JSStringCreateWithUTF8CString("disconnect");
    1376     static JSStringRef lengthStr = JSStringCreateWithUTF8CString("length");
    1377     static JSStringRef nameStr = JSStringCreateWithUTF8CString("name");
    13781355    JSRetainPtr<JSStringRef> actualNameStr(Adopt, JSStringCreateWithUTF8CString(m_identifier.constData()));
    13791356
    1380     JSObjectSetProperty(context, connectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
    1381     JSObjectSetProperty(context, connectFunction, nameStr, JSValueMakeString(context, connectStr), attributes, exception);
    1382     JSObjectSetProperty(context, disconnectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
    1383     JSObjectSetProperty(context, disconnectFunction, nameStr, JSValueMakeString(context, disconnectStr), attributes, exception);
    1384 
     1357    JSObjectRef object = JSObjectMakeFunctionWithCallback(context, actualNameStr.get(), call);
     1358    JSValueProtect(context, object);
     1359
     1360    JSObjectRef generalFunctionProto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
     1361    JSObjectRef runtimeMethodProto = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
     1362    JSObjectSetPrototype(context, runtimeMethodProto, generalFunctionProto);
     1363
     1364    JSObjectSetPrototype(context, object, runtimeMethodProto);
     1365
     1366    JSObjectRef connectFunction = JSObjectMakeFunctionWithCallback(context, connectStr, connect);
     1367    JSObjectSetPrototype(context, connectFunction, runtimeMethodProto);
     1368
     1369    JSObjectRef disconnectFunction = JSObjectMakeFunctionWithCallback(context, disconnectStr, disconnect);
     1370    JSObjectSetPrototype(context, disconnectFunction, runtimeMethodProto);
     1371
     1372    const JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
    13851373    JSObjectSetProperty(context, object, connectStr, connectFunction, attributes, exception);
    13861374    JSObjectSetProperty(context, object, disconnectStr, disconnectFunction, attributes, exception);
    1387     JSObjectSetProperty(context, object, lengthStr, JSValueMakeNumber(context, 0), attributes, exception);
    1388     JSObjectSetProperty(context, object, nameStr, JSValueMakeString(context, actualNameStr.get()), attributes, exception);
    1389 
    1390     m_instance->m_cachedMethods.set(context, this, object);
     1375
     1376    m_jsObject = PassWeak<JSObject>(toJS(object));
    13911377
    13921378    return object;
    13931379}
    13941380
     1381QtRuntimeMethod* QtRuntimeMethod::toRuntimeMethod(JSContextRef context, JSObjectRef object)
     1382{
     1383    JSObjectRef proto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
     1384    if (!proto)
     1385        return 0;
     1386    if (!JSValueIsObjectOfClass(context, proto, prototypeForSignalsAndSlots()))
     1387        return 0;
     1388    return static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(proto));
     1389}
     1390
    13951391JSValueRef QtRuntimeMethod::connectOrDisconnect(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect)
    13961392{
    1397     QtRuntimeMethod* d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(thisObject));
     1393    QtRuntimeMethod* d = toRuntimeMethod(context, thisObject);
    13981394    if (!d)
    1399         d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
     1395        d = toRuntimeMethod(context, function);
     1396    if (!d) {
     1397        QString errorStr = QStringLiteral("QtMetaMethod.%1: Cannot connect to/from deleted QObject").arg(connect ?  QStringLiteral("connect") : QStringLiteral("disconnect"));
     1398        setException(context, exception, errorStr);
     1399        return JSValueMakeUndefined(context);
     1400    }
    14001401
    14011402    QString functionName = connect ? QStringLiteral("connect") : QStringLiteral("disconnect");
    … …  
    14331434        // object.signal.connect(someFunction);
    14341435        if (JSObjectIsFunction(context, targetFunction)) {
    1435             if (JSValueIsObjectOfClass(context, targetFunction, prototypeForSignalsAndSlots())) {
    1436                 // object.signal.connect(otherObject.slot);
    1437                 if (QtRuntimeMethod* targetMethod = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(targetFunction)))
    1438                     targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
    1439             }
     1436            // object.signal.connect(otherObject.slot);
     1437            if (QtRuntimeMethod* targetMethod = toRuntimeMethod(context, targetFunction))
     1438                targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
    14401439        } else
    14411440            targetFunction = 0;

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

    r126161 r126287  
    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

    r126161 r126287  
     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-21  Sheriff Bot  <webkit.review.bot@gmail.com>
    218

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

    r126161 r126287  
    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.