Changeset 245514 in webkit

Timestamp:

May 20, 2019 6:48:04 AM (5 years ago)

Author:

Carlos Garcia Campos

Message:

[GLIB] Crash when instantiating a js object registered with jsc_context_register_class on window object cleared
​https://bugs.webkit.org/show_bug.cgi?id=198037

Reviewed by Michael Catanzaro.

Source/JavaScriptCore:

This happens because JSCClass is keeping a pointer to the JSCContext used when the class is registered, and the
context can be destroyed before the class. We can't a reference to the context, because we don't really want to
keep it alive. The life of the JSCClass is not attached to the JSCContext, but to its wrapped global context, so
we can keep a pointer to the JSGlobalContextRef instead and create a new JSCContext wrapping it when
needed. This patch is also making the context property of JSCClass non-readable, which was always the intention,
that's why there isn't a public getter in the API.

• API/glib/JSCCallbackFunction.cpp:

(JSC::JSCCallbackFunction::construct): Pass the context to jscClassGetOrCreateJSWrapper().

• API/glib/JSCClass.cpp:

(jscClassGetProperty): Remove the getter for context property.
(jscClassSetProperty): Get the JSGlobalContextRef from the given JSCContext.
(jsc_class_class_init): Make context writable only.
(jscClassCreate): Use the passed in context instead of the member.
(jscClassGetOrCreateJSWrapper): It receives now the context as parameter.
(jscClassCreateContextWithJSWrapper): Ditto.
(jscClassCreateConstructor): Get or create a JSCContext for our JSGlobalContextRef.
(jscClassAddMethod): Ditto.
(jsc_class_add_property): Ditto.

• API/glib/JSCClassPrivate.h:
• API/glib/JSCContext.cpp:

(jsc_context_evaluate_in_object): Pass the context to jscClassCreateContextWithJSWrapper().

• API/glib/JSCValue.cpp:

(jsc_value_new_object): Pass the context to jscClassGetOrCreateJSWrapper().

Tools:

Add a test case to check the crash is fixed.

• TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:

(testWebExtensionWindowObjectCleared):

• TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp:

(windowObjectCleared):

Location:

trunk

Files:

• Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp (modified) (1 diff)
• Source/JavaScriptCore/API/glib/JSCClass.cpp (modified) (10 diffs)
• Source/JavaScriptCore/API/glib/JSCClassPrivate.h (modified) (1 diff)
• Source/JavaScriptCore/API/glib/JSCContext.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/WebKitGLib/TestWebExtensions.cpp (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp (modified) (1 diff)

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

@@ -208 +208 @@
     case G_TYPE_OBJECT:
         if (auto* ptr = returnValue.data[0].v_pointer)
-            return toRef(jscClassGetOrCreateJSWrapper(m_class.get(), ptr));
+            return toRef(jscClassGetOrCreateJSWrapper(m_class.get(), context.get(), ptr));
         *exception = toRef(JSC::createTypeError(toJS(jsContext), "constructor returned null"_s));
         break;

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

@@ -57 +57 @@
 
 typedef struct _JSCClassPrivate {
-    JSCContext* context;
+    JSGlobalContextRef context;
     CString name;
     JSClassRef jsClass;
@@ -64 +64 @@
     JSCClass* parentClass;
     JSC::Weak<JSC::JSObject> prototype;
-    HashMap<CString, JSC::Weak<JSC::JSObject>> constructors;
 } JSCClassPrivate;
 
@@ -284 +283 @@
 
     switch (propID) {
-    case PROP_CONTEXT:
-        g_value_set_object(value, jscClass->priv->context);
-        break;
     case PROP_NAME:
         g_value_set_string(value, jscClass->priv->name.data());
@@ -304 +300 @@
     switch (propID) {
     case PROP_CONTEXT:
-        jscClass->priv->context = JSC_CONTEXT(g_value_get_object(value));
+        jscClass->priv->context = jscContextGetJSContext(JSC_CONTEXT(g_value_get_object(value)));
         break;
     case PROP_NAME:
@@ -348 +344 @@
             "JSC Context",
             JSC_TYPE_CONTEXT,
-            static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));
+            static_cast<GParamFlags>(WEBKIT_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)));
 
     /**
@@ -493 +489 @@
     prototypeDefinition.className = prototypeName.get();
     JSClassRef prototypeClass = JSClassCreate(&prototypeDefinition);
-    priv->prototype = jscContextGetOrCreateJSWrapper(priv->context, prototypeClass);
+    priv->prototype = jscContextGetOrCreateJSWrapper(context, prototypeClass);
     JSClassRelease(prototypeClass);
 
     if (priv->parentClass)
-        JSObjectSetPrototype(jscContextGetJSContext(priv->context), toRef(priv->prototype.get()), toRef(priv->parentClass->priv->prototype.get()));
+        JSObjectSetPrototype(jscContextGetJSContext(context), toRef(priv->prototype.get()), toRef(priv->parentClass->priv->prototype.get()));
     return jscClass;
 }
@@ -506 +502 @@
 }
 
-JSC::JSObject* jscClassGetOrCreateJSWrapper(JSCClass* jscClass, gpointer wrappedObject)
+JSC::JSObject* jscClassGetOrCreateJSWrapper(JSCClass* jscClass, JSCContext* context, gpointer wrappedObject)
 {
     JSCClassPrivate* priv = jscClass->priv;
-    return jscContextGetOrCreateJSWrapper(priv->context, priv->jsClass, toRef(priv->prototype.get()), wrappedObject, priv->destroyFunction);
-}
-
-JSGlobalContextRef jscClassCreateContextWithJSWrapper(JSCClass* jscClass, gpointer wrappedObject)
+    return jscContextGetOrCreateJSWrapper(context, priv->jsClass, toRef(priv->prototype.get()), wrappedObject, priv->destroyFunction);
+}
+
+JSGlobalContextRef jscClassCreateContextWithJSWrapper(JSCClass* jscClass, JSCContext* context, gpointer wrappedObject)
 {
     JSCClassPrivate* priv = jscClass->priv;
-    return jscContextCreateContextWithJSWrapper(priv->context, priv->jsClass, toRef(priv->prototype.get()), wrappedObject, priv->destroyFunction);
+    return jscContextCreateContextWithJSWrapper(context, priv->jsClass, toRef(priv->prototype.get()), wrappedObject, priv->destroyFunction);
 }
 
@@ -563 +559 @@
         closure = adoptGRef(g_cclosure_new(callback, userData, reinterpret_cast<GClosureNotify>(reinterpret_cast<GCallback>(destroyNotify))));
     JSCClassPrivate* priv = jscClass->priv;
-    JSC::ExecState* exec = toJS(jscContextGetJSContext(priv->context));
+    JSC::ExecState* exec = toJS(priv->context);
     JSC::VM& vm = exec->vm();
     JSC::JSLockHolder locker(vm);
     auto* functionObject = JSC::JSCCallbackFunction::create(vm, exec->lexicalGlobalObject(), String::fromUTF8(name),
         JSC::JSCCallbackFunction::Type::Constructor, jscClass, WTFMove(closure), returnType, WTFMove(parameters));
-    auto constructor = jscContextGetOrCreateValue(priv->context, toRef(functionObject));
-    GRefPtr<JSCValue> prototype = jscContextGetOrCreateValue(priv->context, toRef(priv->prototype.get()));
+    auto context = jscContextGetOrCreate(priv->context);
+    auto constructor = jscContextGetOrCreateValue(context.get(), toRef(functionObject));
+    GRefPtr<JSCValue> prototype = jscContextGetOrCreateValue(context.get(), toRef(priv->prototype.get()));
     auto nonEnumerable = static_cast<JSCValuePropertyFlags>(JSC_VALUE_PROPERTY_CONFIGURABLE | JSC_VALUE_PROPERTY_WRITABLE);
     jsc_value_object_define_property_data(constructor.get(), "prototype", nonEnumerable, prototype.get());
     jsc_value_object_define_property_data(prototype.get(), "constructor", nonEnumerable, constructor.get());
-    priv->constructors.set(name, functionObject);
     return constructor;
 }
@@ -712 +708 @@
     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::ExecState* exec = toJS(priv->context);
     JSC::VM& vm = exec->vm();
     JSC::JSLockHolder locker(vm);
     auto* functionObject = toRef(JSC::JSCCallbackFunction::create(vm, exec->lexicalGlobalObject(), String::fromUTF8(name),
         JSC::JSCCallbackFunction::Type::Method, jscClass, WTFMove(closure), returnType, WTFMove(parameters)));
-    auto method = jscContextGetOrCreateValue(priv->context, functionObject);
-    GRefPtr<JSCValue> prototype = jscContextGetOrCreateValue(priv->context, toRef(priv->prototype.get()));
+    auto context = jscContextGetOrCreate(priv->context);
+    auto method = jscContextGetOrCreateValue(context.get(), functionObject);
+    GRefPtr<JSCValue> prototype = jscContextGetOrCreateValue(context.get(), toRef(priv->prototype.get()));
     auto nonEnumerable = static_cast<JSCValuePropertyFlags>(JSC_VALUE_PROPERTY_CONFIGURABLE | JSC_VALUE_PROPERTY_WRITABLE);
     jsc_value_object_define_property_data(prototype.get(), name, nonEnumerable, method.get());
@@ -863 +860 @@
     g_return_if_fail(priv->context);
 
-    GRefPtr<JSCValue> prototype = jscContextGetOrCreateValue(priv->context, toRef(priv->prototype.get()));
+    auto context = jscContextGetOrCreate(priv->context);
+    GRefPtr<JSCValue> prototype = jscContextGetOrCreateValue(context.get(), toRef(priv->prototype.get()));
     jsc_value_object_define_property_accessor(prototype.get(), name, JSC_VALUE_PROPERTY_CONFIGURABLE, propertyType, getter, setter, userData, destroyNotify);
 }

trunk/Source/JavaScriptCore/API/glib/JSCClassPrivate.h

@@ -28 +28 @@
 GRefPtr<JSCClass> jscClassCreate(JSCContext*, const char*, JSCClass*, JSCClassVTable*, GDestroyNotify);
 JSClassRef jscClassGetJSClass(JSCClass*);
-JSC::JSObject* jscClassGetOrCreateJSWrapper(JSCClass*, gpointer);
-JSGlobalContextRef jscClassCreateContextWithJSWrapper(JSCClass*, gpointer);
+JSC::JSObject* jscClassGetOrCreateJSWrapper(JSCClass*, JSCContext*, gpointer);
+JSGlobalContextRef jscClassCreateContextWithJSWrapper(JSCClass*, JSCContext*, gpointer);
 void jscClassInvalidate(JSCClass*);

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

@@ -879 +879 @@
 
     JSRetainPtr<JSGlobalContextRef> objectContext(Adopt,
-        instance ? jscClassCreateContextWithJSWrapper(objectClass, instance) : JSGlobalContextCreateInGroup(jscVirtualMachineGetContextGroup(context->priv->vm.get()), nullptr));
+        instance ? jscClassCreateContextWithJSWrapper(objectClass, context, instance) : JSGlobalContextCreateInGroup(jscVirtualMachineGetContextGroup(context->priv->vm.get()), nullptr));
     JSC::ExecState* exec = toJS(objectContext.get());
     JSC::VM& vm = exec->vm();

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

@@ -603 +603 @@
     g_return_val_if_fail(!instance || JSC_IS_CLASS(jscClass), nullptr);
 
-    return jscContextGetOrCreateValue(context, instance ? toRef(jscClassGetOrCreateJSWrapper(jscClass, instance)) : JSObjectMake(jscContextGetJSContext(context), nullptr, nullptr)).leakRef();
+    return jscContextGetOrCreateValue(context, instance ? toRef(jscClassGetOrCreateJSWrapper(jscClass, context, instance)) : JSObjectMake(jscContextGetJSContext(context), nullptr, nullptr)).leakRef();
 }
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-05-20  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GLIB] Crash when instantiating a js object registered with jsc_context_register_class on window object cleared
+        https://bugs.webkit.org/show_bug.cgi?id=198037
+
+        Reviewed by Michael Catanzaro.
+
+        This happens because JSCClass is keeping a pointer to the JSCContext used when the class is registered, and the
+        context can be destroyed before the class. We can't a reference to the context, because we don't really want to
+        keep it alive. The life of the JSCClass is not attached to the JSCContext, but to its wrapped global context, so
+        we can keep a pointer to the JSGlobalContextRef instead and create a new JSCContext wrapping it when
+        needed. This patch is also making the context property of JSCClass non-readable, which was always the intention,
+        that's why there isn't a public getter in the API.
+
+        * API/glib/JSCCallbackFunction.cpp:
+        (JSC::JSCCallbackFunction::construct): Pass the context to jscClassGetOrCreateJSWrapper().
+        * API/glib/JSCClass.cpp:
+        (jscClassGetProperty): Remove the getter for context property.
+        (jscClassSetProperty): Get the JSGlobalContextRef from the given JSCContext.
+        (jsc_class_class_init): Make context writable only.
+        (jscClassCreate): Use the passed in context instead of the member.
+        (jscClassGetOrCreateJSWrapper): It receives now the context as parameter.
+        (jscClassCreateContextWithJSWrapper): Ditto.
+        (jscClassCreateConstructor): Get or create a JSCContext for our JSGlobalContextRef.
+        (jscClassAddMethod): Ditto.
+        (jsc_class_add_property): Ditto.
+        * API/glib/JSCClassPrivate.h:
+        * API/glib/JSCContext.cpp:
+        (jsc_context_evaluate_in_object): Pass the context to jscClassCreateContextWithJSWrapper().
+        * API/glib/JSCValue.cpp:
+        (jsc_value_new_object): Pass the context to jscClassGetOrCreateJSWrapper().
+
 2019-05-19  Tadeu Zagallo  <tzagallo@apple.com>
 

trunk/Tools/ChangeLog

@@ +1 @@
+2019-05-20  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GLIB] Crash when instantiating a js object registered with jsc_context_register_class on window object cleared
+        https://bugs.webkit.org/show_bug.cgi?id=198037
+
+        Reviewed by Michael Catanzaro.
+
+        Add a test case to check the crash is fixed.
+
+        * TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:
+        (testWebExtensionWindowObjectCleared):
+        * TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp:
+        (windowObjectCleared):
+
 2019-05-20  Carlos Garcia Campos  <cgarcia@igalia.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp

@@ -180 +180 @@
     GUniquePtr<char> valueString(WebViewTest::javascriptResultToCString(javascriptResult));
     g_assert_cmpstr(valueString.get(), ==, "Foo");
+
+    javascriptResult = test->runJavaScriptAndWaitUntilFinished("var f = new GFile('.'); f.path();", &error.outPtr());
+    g_assert_nonnull(javascriptResult);
+    g_assert_no_error(error.get());
+    valueString.reset(WebViewTest::javascriptResultToCString(javascriptResult));
+    GUniquePtr<char> currentDirectory(g_get_current_dir());
+    g_assert_cmpstr(valueString.get(), ==, currentDirectory.get());
 }
 

trunk/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp

@@ -428 +428 @@
     GRefPtr<JSCValue> function = adoptGRef(jsc_value_new_function(jsContext.get(), "echo", G_CALLBACK(echoCallback), NULL, NULL, G_TYPE_STRING, 1, G_TYPE_STRING));
     jsc_context_set_value(jsContext.get(), "echo", function.get());
+
+    auto* fileClass = jsc_context_register_class(jsContext.get(), "GFile", nullptr, nullptr, static_cast<GDestroyNotify>(g_object_unref));
+    GRefPtr<JSCValue> constructor = adoptGRef(jsc_class_add_constructor(fileClass, "GFile", G_CALLBACK(g_file_new_for_path), nullptr, nullptr, G_TYPE_OBJECT, 1, G_TYPE_STRING));
+    jsc_class_add_method(fileClass, "path", G_CALLBACK(g_file_get_path), nullptr, nullptr, G_TYPE_STRING, 0, G_TYPE_NONE);
+    jsc_context_set_value(jsContext.get(), "GFile", constructor.get());
 }