Attachment 423998 Details for Bug 223629 – Patch

[patch] Patch

bug-223629-20210323015321.patch (text/plain), 7.93 KB, created by Yusuke Suzuki on 2021-03-23 01:53:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2021-03-23 01:53:22 PDT

Size: 7.93 KB

patch

obsolete

>Subversion Revision: 274852
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 160892f750da22ebe0ab089d69cefe3f72633a9e..5a97d756d3055892270ffb6ea57c0fb68e5d0660 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2021-03-23  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Functor for WeakGCMap::ensureValue must not invoke GC
>+        https://bugs.webkit.org/show_bug.cgi?id=223629
>+        <rdar://problem/75619217>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The functor for WeakGCMap::ensureValue must not invoke GC. GC can prune entries in WeakGCMap.
>+        So we can modify underlying HashMap while we are just touching it for HashMap::ensure. This
>+        can corrupt HashMap. To ensure this invariant, we put DisallowGC for WeakGCMap::ensureValue.
>+        So we cannot invoke GC in the functor of that function (otherwise, assertion hits).
>+
>+        And we use DeferGC in createCustomGetterFunction / createCustomSetterFunction to avoid invoking
>+        GC in WeakGCMap::ensureValue. This defers GC invocation until this DeferGC scope is destroyed,
>+        and ensures that functor invoked by WeakGCMap::ensureValue will not cause GC.
>+
>+        * runtime/JSObject.cpp:
>+        (JSC::createCustomGetterFunction):
>+        (JSC::createCustomSetterFunction):
>+        (JSC::JSObject::getOwnPropertyDescriptor):
>+        * runtime/WeakGCMap.h:
>+
> 2021-03-22  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] JSCustomGetterFunction/JSCustomSetterFunction should use Identifier for their field
>diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp
>index 79fbe94b7fdb17e155da0ca9561479026b16842b..bcdbde52d9345b78fa011da0a94f721bfbf3eed4 100644
>--- a/Source/JavaScriptCore/runtime/JSObject.cpp
>+++ b/Source/JavaScriptCore/runtime/JSObject.cpp
>@@ -3521,17 +3521,23 @@ Butterfly* JSObject::allocateMoreOutOfLineStorage(VM& vm, size_t oldSize, size_t
>     return Butterfly::createOrGrowPropertyStorage(butterfly(), vm, this, structure(vm), oldSize, newSize);
> }
> 
>-static JSCustomGetterFunction* createCustomGetterFunction(JSGlobalObject* globalObject, PropertyName propertyName, GetValueFunc getValueFunc, Optional<DOMAttributeAnnotation> domAttribute)
>+static JSCustomGetterFunction* createCustomGetterFunction(JSGlobalObject* globalObject, VM& vm, PropertyName propertyName, GetValueFunc getValueFunc, Optional<DOMAttributeAnnotation> domAttribute)
> {
>+    // WeakGCMap::ensureValue's functor must not invoke GC since GC can modify WeakGCMap in the middle of HashMap::ensure.
>+    // We use DeferGC here (1) not to invoke GC when executing WeakGCMap::ensureValue and (2) to avoid looking up HashMap twice.
>+    DeferGC deferGC(vm.heap);
>     return globalObject->customGetterFunctionMap().ensureValue(getValueFunc, [&] {
>-        return JSCustomGetterFunction::create(globalObject->vm(), globalObject, propertyName, getValueFunc, domAttribute);
>+        return JSCustomGetterFunction::create(vm, globalObject, propertyName, getValueFunc, domAttribute);
>     });
> }
> 
>-static JSCustomSetterFunction* createCustomSetterFunction(JSGlobalObject* globalObject, PropertyName propertyName, PutValueFunc putValueFunc)
>+static JSCustomSetterFunction* createCustomSetterFunction(JSGlobalObject* globalObject, VM& vm, PropertyName propertyName, PutValueFunc putValueFunc)
> {
>+    // WeakGCMap::ensureValue's functor must not invoke GC since GC can modify WeakGCMap in the middle of HashMap::ensure.
>+    // We use DeferGC here (1) not to invoke GC when executing WeakGCMap::ensureValue and (2) to avoid looking up HashMap twice.
>+    DeferGC deferGC(vm.heap);
>     return globalObject->customSetterFunctionMap().ensureValue(putValueFunc, [&] {
>-        return JSCustomSetterFunction::create(globalObject->vm(), globalObject, propertyName, putValueFunc);
>+        return JSCustomSetterFunction::create(vm, globalObject, propertyName, putValueFunc);
>     });
> }
> 
>@@ -3553,9 +3559,9 @@ bool JSObject::getOwnPropertyDescriptor(JSGlobalObject* globalObject, PropertyNa
>         descriptor.setAccessorDescriptor((slot.attributes() | PropertyAttribute::Accessor) & ~PropertyAttribute::CustomAccessor);
>         JSGlobalObject* slotBaseGlobalObject = slot.slotBase()->globalObject(vm);
>         if (slot.customGetter())
>-            descriptor.setGetter(createCustomGetterFunction(slotBaseGlobalObject, propertyName, slot.customGetter(), slot.domAttribute()));
>+            descriptor.setGetter(createCustomGetterFunction(slotBaseGlobalObject, vm, propertyName, slot.customGetter(), slot.domAttribute()));
>         if (slot.customSetter())
>-            descriptor.setSetter(createCustomSetterFunction(slotBaseGlobalObject, propertyName, slot.customSetter()));
>+            descriptor.setSetter(createCustomSetterFunction(slotBaseGlobalObject, vm, propertyName, slot.customSetter()));
>     } else {
>         JSValue value = slot.getValue(globalObject, propertyName);
>         RETURN_IF_EXCEPTION(scope, false);
>diff --git a/Source/JavaScriptCore/runtime/WeakGCMap.h b/Source/JavaScriptCore/runtime/WeakGCMap.h
>index e380f295b01cabdecc86d6fc4022469d235218a7..ed95e8b723fc9767b0376c3f0423149316db83ff 100644
>--- a/Source/JavaScriptCore/runtime/WeakGCMap.h
>+++ b/Source/JavaScriptCore/runtime/WeakGCMap.h
>@@ -25,6 +25,7 @@
> 
> #pragma once
> 
>+#include "DeferGC.h"
> #include "Weak.h"
> #include <wtf/HashMap.h>
> 
>@@ -67,6 +68,9 @@ class WeakGCMap final : public WeakGCMapBase {
>     template<typename Functor>
>     ValueArg* ensureValue(const KeyType& key, Functor&& functor)
>     {
>+        // If functor invokes GC, GC can prune WeakGCMap, and manipulate HashMap while we are touching it in ensure function.
>+        // The functor must not invoke GC.
>+        DisallowGC disallowGC;
>         AddResult result = m_map.ensure(key, std::forward<Functor>(functor));
>         ValueArg* value = result.iterator->value.get();
>         if (!result.isNewEntry && !value) {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index fb3d7adb1646ddb685865127ebb4ecc0d38feea3..65bef66df48a38e8d1acb1a3a4436c61742d8045 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2021-03-23  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Functor for WeakGCMap::ensureValue must not invoke GC
>+        https://bugs.webkit.org/show_bug.cgi?id=223629
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * js/dom/weak-gc-map-ensure-value-should-not-invoke-gc-expected.txt: Added.
>+        * js/dom/weak-gc-map-ensure-value-should-not-invoke-gc.html: Added.
>+
> 2021-03-23  Said Abou-Hallawa  <said@apple.com>
> 
>         SVG links navigate only to internal animation elements and <view> anchors
>diff --git a/LayoutTests/js/dom/weak-gc-map-ensure-value-should-not-invoke-gc-expected.txt b/LayoutTests/js/dom/weak-gc-map-ensure-value-should-not-invoke-gc-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..4c12cd5ba8320ef6e1bc68c604653c57b34e934b
>--- /dev/null
>+++ b/LayoutTests/js/dom/weak-gc-map-ensure-value-should-not-invoke-gc-expected.txt
>@@ -0,0 +1,9 @@
>+Do not invoke GC while modifying WeakGCMap
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/js/dom/weak-gc-map-ensure-value-should-not-invoke-gc.html b/LayoutTests/js/dom/weak-gc-map-ensure-value-should-not-invoke-gc.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..4e9984ea65bf9728c93b5acbe3591f70332eeceb
>--- /dev/null
>+++ b/LayoutTests/js/dom/weak-gc-map-ensure-value-should-not-invoke-gc.html
>@@ -0,0 +1,15 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../../resources/js-test-pre.js"></script>
>+</head>
>+<body>
>+<script>
>+description('Do not invoke GC while modifying WeakGCMap');
>+for (let name of Object.keys(this)) {
>+    Object.getOwnPropertyDescriptor(this, name);
>+}
>+</script>
>+<script src="../../resources/js-test-post.js"></script>
>+</body>
>+</html>

Flags:

mark.lam: review+
ews-feeder: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 223629: 423997 | 423998 | 424043