Attachment 370249 Details for Bug 198037 – Patch

[patch] Patch

jsc-class-context.diff (text/plain), 16.15 KB, created by Carlos Garcia Campos on 2019-05-20 05:43:26 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Carlos Garcia Campos

Created: 2019-05-20 05:43:26 PDT

Size: 16.15 KB

patch

obsolete

>diff --git a/Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp b/Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp
>index 7bd2c1a7b5c..e222a4057a7 100644
>--- a/Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp
>+++ b/Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp
>@@ -207,7 +207,7 @@ JSObjectRef JSCCallbackFunction::construct(JSContextRef callerContext, size_t ar
>     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:
>diff --git a/Source/JavaScriptCore/API/glib/JSCClass.cpp b/Source/JavaScriptCore/API/glib/JSCClass.cpp
>index 3c0faed7a1c..4c054beab8d 100644
>--- a/Source/JavaScriptCore/API/glib/JSCClass.cpp
>+++ b/Source/JavaScriptCore/API/glib/JSCClass.cpp
>@@ -56,14 +56,13 @@ enum {
> };
> 
> typedef struct _JSCClassPrivate {
>-    JSCContext* context;
>+    JSGlobalContextRef context;
>     CString name;
>     JSClassRef jsClass;
>     JSCClassVTable* vtable;
>     GDestroyNotify destroyFunction;
>     JSCClass* parentClass;
>     JSC::Weak<JSC::JSObject> prototype;
>-    HashMap<CString, JSC::Weak<JSC::JSObject>> constructors;
> } JSCClassPrivate;
> 
> struct _JSCClass {
>@@ -283,9 +282,6 @@ static void jscClassGetProperty(GObject* object, guint propID, GValue* value, GP
>     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 @@ static void jscClassSetProperty(GObject* object, guint propID, const GValue* val
> 
>     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 @@ static void jsc_class_class_init(JSCClassClass* klass)
>             "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 @@ GRefPtr<JSCClass> jscClassCreate(JSCContext* context, const char* name, JSCClass
>     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 @@ JSClassRef jscClassGetJSClass(JSCClass* jscClass)
>     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 @@ static GRefPtr<JSCValue> jscClassCreateConstructor(JSCClass* jscClass, const cha
>     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 @@ static void jscClassAddMethod(JSCClass* jscClass, const char* name, GCallback ca
> {
>     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 @@ void jsc_class_add_property(JSCClass* jscClass, const char* name, GType property
>     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);
> }
>diff --git a/Source/JavaScriptCore/API/glib/JSCClassPrivate.h b/Source/JavaScriptCore/API/glib/JSCClassPrivate.h
>index 84a5d4b08a6..605daf526e0 100644
>--- a/Source/JavaScriptCore/API/glib/JSCClassPrivate.h
>+++ b/Source/JavaScriptCore/API/glib/JSCClassPrivate.h
>@@ -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*);
>diff --git a/Source/JavaScriptCore/API/glib/JSCContext.cpp b/Source/JavaScriptCore/API/glib/JSCContext.cpp
>index 37eaeebb3a8..e01e970c7f5 100644
>--- a/Source/JavaScriptCore/API/glib/JSCContext.cpp
>+++ b/Source/JavaScriptCore/API/glib/JSCContext.cpp
>@@ -878,7 +878,7 @@ JSCValue* jsc_context_evaluate_in_object(JSCContext* context, const char* code,
>     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);
>diff --git a/Source/JavaScriptCore/API/glib/JSCValue.cpp b/Source/JavaScriptCore/API/glib/JSCValue.cpp
>index 76b892fea30..5e177495d82 100644
>--- a/Source/JavaScriptCore/API/glib/JSCValue.cpp
>+++ b/Source/JavaScriptCore/API/glib/JSCValue.cpp
>@@ -602,7 +602,7 @@ JSCValue* jsc_value_new_object(JSCContext* context, gpointer instance, JSCClass*
>     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();
> }
> 
> /**
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 11440090226..8cc98044997 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,35 @@
>+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 NOBODY (OOPS!).
>+
>+        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>
> 
>         Add support for %pid in dumpJITMemoryPath
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index cad4337cfba..9a774a9cc74 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,17 @@
>+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 NOBODY (OOPS!).
>+
>+        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>
> 
>         [GLIB] Repeating timer is not stopped when stop is called from the callback
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp b/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp
>index 9810a4d1dd8..f8ea1886aa0 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp
>@@ -179,6 +179,13 @@ static void testWebExtensionWindowObjectCleared(WebViewTest* test, gconstpointer
>     g_assert_no_error(error.get());
>     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());
> }
> 
> static gboolean scriptDialogCallback(WebKitWebView*, WebKitScriptDialog* dialog, gpointer)
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp b/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp
>index 6bdad479a5f..ea46cccfd0e 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp
>@@ -427,6 +427,11 @@ static void windowObjectCleared(WebKitScriptWorld* world, WebKitWebPage* page, W
>     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)

Flags:

mcatanzaro: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198037: 370249