Title: [246260] releases/WebKitGTK/webkit-2.24
Revision
246260
Author
[email protected]
Date
2019-06-10 07:39:10 -0700 (Mon, 10 Jun 2019)

Log Message

Merged r245514 - [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/_javascript_Core:

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):

Modified Paths

Diff

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCCallbackFunction.cpp (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCCallbackFunction.cpp	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCCallbackFunction.cpp	2019-06-10 14:39:10 UTC (rev 246260)
@@ -207,7 +207,7 @@
     case G_TYPE_BOXED:
     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;
     default:

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCClass.cpp (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCClass.cpp	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCClass.cpp	2019-06-10 14:39:10 UTC (rev 246260)
@@ -56,7 +56,7 @@
 };
 
 typedef struct _JSCClassPrivate {
-    JSCContext* context;
+    JSGlobalContextRef context;
     CString name;
     JSClassRef jsClass;
     JSCClassVTable* vtable;
@@ -63,7 +63,6 @@
     GDestroyNotify destroyFunction;
     JSCClass* parentClass;
     JSC::Weak<JSC::JSObject> prototype;
-    HashMap<CString, JSC::Weak<JSC::JSObject>> constructors;
 } JSCClassPrivate;
 
 struct _JSCClass {
@@ -283,9 +282,6 @@
     JSCClass* jscClass = JSC_CLASS(object);
 
     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());
         break;
@@ -303,7 +299,7 @@
 
     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:
         jscClass->priv->name = g_value_get_string(value);
@@ -347,7 +343,7 @@
             "JSCContext",
             "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)));
 
     /**
      * JSCClass:name:
@@ -492,11 +488,11 @@
     JSClassDefinition prototypeDefinition = kJSClassDefinitionEmpty;
     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;
 }
 
@@ -505,16 +501,16 @@
     return jscClass->priv->jsClass;
 }
 
-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);
+    return jscContextGetOrCreateJSWrapper(context, priv->jsClass, toRef(priv->prototype.get()), wrappedObject, priv->destroyFunction);
 }
 
-JSGlobalContextRef jscClassCreateContextWithJSWrapper(JSCClass* jscClass, gpointer wrappedObject)
+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);
 }
 
 void jscClassInvalidate(JSCClass* jscClass)
@@ -562,17 +558,17 @@
     else
         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;
 }
 
@@ -711,13 +707,14 @@
 {
     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());
 }
@@ -862,6 +859,7 @@
     JSCClassPrivate* priv = jscClass->priv;
     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);
 }

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCClassPrivate.h (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCClassPrivate.h	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCClassPrivate.h	2019-06-10 14:39:10 UTC (rev 246260)
@@ -27,6 +27,6 @@
 
 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*);

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCContext.cpp (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCContext.cpp	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCContext.cpp	2019-06-10 14:39:10 UTC (rev 246260)
@@ -878,7 +878,7 @@
     g_return_val_if_fail(object && !*object, nullptr);
 
     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();
     auto* jsObject = vm.vmEntryGlobalObject(exec);

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCValue.cpp (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCValue.cpp	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/API/glib/JSCValue.cpp	2019-06-10 14:39:10 UTC (rev 246260)
@@ -602,7 +602,7 @@
     g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr);
     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();
 }
 
 /**

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog	2019-06-10 14:39:10 UTC (rev 246260)
@@ -1,3 +1,35 @@
+2019-05-20  Carlos Garcia Campos  <[email protected]>
+
+        [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-23  Adrian Perez de Castro  <[email protected]>
 
         Fix a few missing header inclusions often masked by by unified sources

Modified: releases/WebKitGTK/webkit-2.24/Tools/ChangeLog (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Tools/ChangeLog	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Tools/ChangeLog	2019-06-10 14:39:10 UTC (rev 246260)
@@ -1,3 +1,17 @@
+2019-05-20  Carlos Garcia Campos  <[email protected]>
+
+        [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-03-18  Adrian Perez de Castro  <[email protected]>
 
         [GTK][WPE] Cryptic error from Tools/gtkdoc/generate-gtkdoc

Modified: releases/WebKitGTK/webkit-2.24/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp	2019-06-10 14:39:10 UTC (rev 246260)
@@ -179,6 +179,13 @@
     g_assert_no_error(error.get());
     GUniquePtr<char> valueString(WebViewTest::_javascript_ResultToCString(_javascript_Result));
     g_assert_cmpstr(valueString.get(), ==, "Foo");
+
+    _javascript_Result = test->runJavaScriptAndWaitUntilFinished("var f = new GFile('.'); f.path();", &error.outPtr());
+    g_assert_nonnull(_javascript_Result);
+    g_assert_no_error(error.get());
+    valueString.reset(WebViewTest::_javascript_ResultToCString(_javascript_Result));
+    GUniquePtr<char> currentDirectory(g_get_current_dir());
+    g_assert_cmpstr(valueString.get(), ==, currentDirectory.get());
 }
 
 static gboolean scriptDialogCallback(WebKitWebView*, WebKitScriptDialog* dialog, gpointer)

Modified: releases/WebKitGTK/webkit-2.24/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp (246259 => 246260)


--- releases/WebKitGTK/webkit-2.24/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp	2019-06-10 14:39:01 UTC (rev 246259)
+++ releases/WebKitGTK/webkit-2.24/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp	2019-06-10 14:39:10 UTC (rev 246260)
@@ -428,6 +428,11 @@
     g_assert_true(JSC_IS_CONTEXT(jsContext.get()));
     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());
 }
 
 static WebKitWebPage* getWebPage(WebKitWebExtension* extension, uint64_t pageID, GDBusMethodInvocation* invocation)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to