[GLIB] User data not correctly passed to callback of functions and constructors with...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Mar 2019 14:36:12 +0000 (14:36 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Mar 2019 14:36:12 +0000 (14:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196073

Patch by Carlos Garcia Campos <cgarcia@igalia.com> on 2019-03-21
Reviewed by Michael Catanzaro.

Source/JavaScriptCore:

This is because GClosure always expects a first parameter as instance. In case of functions or constructors with
no parameters we insert a fake instance which is just a null pointer that is ignored by the callback. But
if the function/constructor has user data the callback will expect one parameter for the user data. In that case
we can simply swap instance/user data so that the fake instance will be the second argument and user data the
first one.

* API/glib/JSCClass.cpp:
(jscClassCreateConstructor): Use g_cclosure_new_swap() if parameters is empty and user data was provided.
* API/glib/JSCValue.cpp:
(jscValueFunctionCreate): Ditto.

Tools:

Add test cases to check functions and constructors with no arguments but receiving user data.

* TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:
(checkUserData):
(testJSCFunction):
(fooCreateWithUserData):
(testJSCClass):

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

Source/JavaScriptCore/API/glib/JSCClass.cpp
Source/JavaScriptCore/API/glib/JSCValue.cpp
Source/JavaScriptCore/ChangeLog
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp

index 7df7c4a..3c0faed 100644 (file)
@@ -554,8 +554,14 @@ JSCClass* jsc_class_get_parent(JSCClass* jscClass)
 
 static GRefPtr<JSCValue> jscClassCreateConstructor(JSCClass* jscClass, const char* name, GCallback callback, gpointer userData, GDestroyNotify destroyNotify, GType returnType, Optional<Vector<GType>>&& parameters)
 {
+    // If the constructor doesn't have arguments, we need to swap the fake instance and user data to ensure
+    // user data is the first parameter and fake instance ignored.
+    GRefPtr<GClosure> closure;
+    if (parameters && parameters->isEmpty() && userData)
+        closure = adoptGRef(g_cclosure_new_swap(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
+    else
+        closure = adoptGRef(g_cclosure_new(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
     JSCClassPrivate* priv = jscClass->priv;
-    GRefPtr<GClosure> closure = adoptGRef(g_cclosure_new(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
     JSC::ExecState* exec = toJS(jscContextGetJSContext(priv->context));
     JSC::VM& vm = exec->vm();
     JSC::JSLockHolder locker(vm);
index ef86e13..76b892f 100644 (file)
@@ -1140,7 +1140,13 @@ void jsc_value_object_define_property_accessor(JSCValue* value, const char* prop
 
 static GRefPtr<JSCValue> jscValueFunctionCreate(JSCContext* context, const char* name, GCallback callback, gpointer userData, GDestroyNotify destroyNotify, GType returnType, Optional<Vector<GType>>&& parameters)
 {
-    GRefPtr<GClosure> closure = adoptGRef(g_cclosure_new(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
+    GRefPtr<GClosure> closure;
+    // If the function doesn't have arguments, we need to swap the fake instance and user data to ensure
+    // user data is the first parameter and fake instance ignored.
+    if (parameters && parameters->isEmpty() && userData)
+        closure = adoptGRef(g_cclosure_new_swap(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
+    else
+        closure = adoptGRef(g_cclosure_new(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
     JSC::ExecState* exec = toJS(jscContextGetJSContext(context));
     JSC::VM& vm = exec->vm();
     JSC::JSLockHolder locker(vm);
index 5391757..1be5bcc 100644 (file)
@@ -1,3 +1,21 @@
+2019-03-21  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GLIB] User data not correctly passed to callback of functions and constructors with no parameters
+        https://bugs.webkit.org/show_bug.cgi?id=196073
+
+        Reviewed by Michael Catanzaro.
+
+        This is because GClosure always expects a first parameter as instance. In case of functions or constructors with
+        no parameters we insert a fake instance which is just a null pointer that is ignored by the callback. But
+        if the function/constructor has user data the callback will expect one parameter for the user data. In that case
+        we can simply swap instance/user data so that the fake instance will be the second argument and user data the
+        first one.
+
+        * API/glib/JSCClass.cpp:
+        (jscClassCreateConstructor): Use g_cclosure_new_swap() if parameters is empty and user data was provided.
+        * API/glib/JSCValue.cpp:
+        (jscValueFunctionCreate): Ditto.
+
 2019-03-21  Pablo Saavedra  <psaavedra@igalia.com>
 
         [JSC][32-bit] Build failure after r243232
index effaae1..bc72a56 100644 (file)
@@ -1,5 +1,20 @@
 2019-03-21  Carlos Garcia Campos  <cgarcia@igalia.com>
 
+        [GLIB] User data not correctly passed to callback of functions and constructors with no parameters
+        https://bugs.webkit.org/show_bug.cgi?id=196073
+
+        Reviewed by Michael Catanzaro.
+
+        Add test cases to check functions and constructors with no arguments but receiving user data.
+
+        * TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:
+        (checkUserData):
+        (testJSCFunction):
+        (fooCreateWithUserData):
+        (testJSCClass):
+
+2019-03-21  Carlos Garcia Campos  <cgarcia@igalia.com>
+
         Unreviewed. Fix GTK build with GLib < 2.58 after r243285.
 
         Add g_assert_cmpfloat_with_epsilon macro if not defined.
index f5f65d9..5e75f9a 100644 (file)
@@ -847,6 +847,11 @@ static char* joinFunction(const char* const* strv, const char* sep)
     return g_strjoinv(sep, const_cast<char**>(strv));
 }
 
+static gboolean checkUserData(GFile* file)
+{
+    return G_IS_FILE(file);
+}
+
 static void testJSCFunction()
 {
     {
@@ -1092,6 +1097,30 @@ static void testJSCFunction()
         g_assert_true(jsc_value_is_number(value.get()));
         g_assert_cmpint(jsc_value_to_int32(value.get()), ==, 0);
     }
+
+    {
+        LeakChecker checker;
+        GRefPtr<JSCContext> context = adoptGRef(jsc_context_new());
+        checker.watch(context.get());
+        ExceptionHandler exceptionHandler(context.get());
+
+        GFile* file = g_file_new_for_path(".");
+        checker.watch(file);
+        GRefPtr<JSCValue> function = adoptGRef(jsc_value_new_function(context.get(), "checkUserData", G_CALLBACK(checkUserData),
+            file, g_object_unref, G_TYPE_BOOLEAN, 0, G_TYPE_NONE));
+        checker.watch(function.get());
+        jsc_context_set_value(context.get(), "checkUserData", function.get());
+
+        GRefPtr<JSCValue> value = adoptGRef(jsc_context_evaluate(context.get(), "checkUserData()", -1));
+        checker.watch(value.get());
+        g_assert_true(jsc_value_is_boolean(value.get()));
+        g_assert_true(jsc_value_to_boolean(value.get()));
+
+        value = adoptGRef(jsc_value_function_call(function.get(), G_TYPE_NONE));
+        checker.watch(value.get());
+        g_assert_true(jsc_value_is_boolean(value.get()));
+        g_assert_true(jsc_value_to_boolean(value.get()));
+    }
 }
 
 static void testJSCObject()
@@ -1397,6 +1426,12 @@ static Foo* fooCreateWithFooV(GPtrArray* values)
     return f;
 }
 
+static Foo* fooCreateWithUserData(GFile* file)
+{
+    g_assert_true(G_IS_FILE(file));
+    return fooCreate();
+}
+
 static void fooFree(Foo* foo)
 {
     foo->~Foo();
@@ -1799,6 +1834,19 @@ static void testJSCClass()
         g_assert_true(jsc_value_is_number(value.get()));
         g_assert_cmpint(jsc_value_to_int32(value.get()), ==, 0);
 
+        GFile* file = g_file_new_for_path(".");
+        checker.watch(file);
+        GRefPtr<JSCValue> constructorUserData = adoptGRef(jsc_class_add_constructor(jscClass, "CreateWithUserData", G_CALLBACK(fooCreateWithUserData),
+            file, g_object_unref, G_TYPE_POINTER, 0, G_TYPE_NONE));
+        checker.watch(constructorUserData.get());
+        g_assert_true(jsc_value_is_constructor(constructorUserData.get()));
+        jsc_value_object_set_property(constructor.get(), "CreateWithUserData", constructorUserData.get());
+
+        GRefPtr<JSCValue> foo5 = adoptGRef(jsc_context_evaluate(context.get(), "f5 = new Foo.CreateWithUserData();", -1));
+        checker.watch(foo5.get());
+        g_assert_true(jsc_value_is_object(foo5.get()));
+        g_assert_true(jsc_value_object_is_instance_of(foo5.get(), jsc_class_get_name(jscClass)));
+
         JSCClass* otherClass = jsc_context_register_class(context.get(), "Baz", nullptr, nullptr, g_free);
         checker.watch(otherClass);
         g_assert_false(jsc_class_get_parent(otherClass));