Changeset 243289 in webkit

Timestamp:

Mar 21, 2019, 7:36:12 AM (7 years ago)

Author:

commit-queue@webkit.org

Message:

[GLIB] User data not correctly passed to callback of functions and constructors with no parameters
​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):

Location:

trunk

Files:

• Source/JavaScriptCore/API/glib/JSCClass.cpp (modified) (1 diff)
• Source/JavaScriptCore/API/glib/JSCValue.cpp (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/API/glib/JSCClass.cpp

@@ -555 +555 @@
 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();

trunk/Source/JavaScriptCore/API/glib/JSCValue.cpp

@@ -1141 +1141 @@
 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();

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Tools/ChangeLog

@@ +1 @@
+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>
 

trunk/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp

@@ -848 +848 @@
 }
 
+static gboolean checkUserData(GFile* file)
+{
+    return G_IS_FILE(file);
+}
+
 static void testJSCFunction()
 {
@@ -1093 +1098 @@
         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()));
+    }
 }
 
@@ -1396 +1425 @@
     }, f);
     return f;
+}
+
+static Foo* fooCreateWithUserData(GFile* file)
+{
+    g_assert_true(G_IS_FILE(file));
+    return fooCreate();
 }
 
@@ -1799 +1834 @@
         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);