Attachment 346046 Details for Bug 188151 – proposed patch.

[patch] proposed patch.

bug-188151.patch (text/plain), 7.73 KB, created by Mark Lam on 2018-07-29 15:59:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2018-07-29 15:59:40 PDT

Size: 7.73 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 234349)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2018-07-29  Mark Lam  <mark.lam@apple.com>
>+
>+        We should only assert a slot for putting a new property is empty if it is from a newly allocated butterfly.
>+        https://bugs.webkit.org/show_bug.cgi?id=188151
>+        <rdar://problem/42020385>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/regress-188151.js: Added.
>+
> 2018-07-25  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         [JSC] Record CoW status in ArrayProfile correctly
>Index: JSTests/stress/regress-188151.js
>===================================================================
>--- JSTests/stress/regress-188151.js	(nonexistent)
>+++ JSTests/stress/regress-188151.js	(working copy)
>@@ -0,0 +1,10 @@
>+// This test should not crash.
>+
>+var arr = [4, 5, 6];
>+arr.push(10);
>+arr.pop();
>+Object.defineProperty(arr, "foo", { value: 0x42 })
>+
>+arr.shift()
>+arr.splice(0, 0, 11, 12)
>+Object.defineProperty(arr, "bar", { value: 0x24 })
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 234349)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,36 @@
>+2018-07-29  Mark Lam  <mark.lam@apple.com>
>+
>+        We should only assert a slot for putting a new property is empty if it is from a newly allocated butterfly.
>+        https://bugs.webkit.org/show_bug.cgi?id=188151
>+        <rdar://problem/42020385>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When putting a new property, JSObject::prepareToPutDirectWithoutTransition() and
>+        JSObject::putDirectInternal() currently always asserts that the target slot
>+        contains an effectively empty value (and is safe for GC scanning) before the put.
>+        This assertion may be false if the slot is from the existing butterfly and not a
>+        newly allocated one.
>+
>+        For example, an array splice operation may shift entries in the existing
>+        butterfly, and therefore, result in unused property slots containing valid
>+        values.  As a result, the slot for the new property may already contain a
>+        non-empty value (before the put) that is safe for GC.  But because it is
>+        non-empty, it will fail the assertion.
>+
>+        This patch fixes the assertion to only do this empty check if the butterfly is
>+        actually newly allocated.
>+
>+        We also introduce a JSValue::isEffectivelyEmptyAndSafeForGCScanning() to better
>+        document what that check is actually doing.
>+
>+        * runtime/JSCJSValue.h:
>+        * runtime/JSCJSValueInlines.h:
>+        (JSC::JSValue::isEffectivelyEmptyAndSafeForGCScanning const):
>+        * runtime/JSObjectInlines.h:
>+        (JSC::JSObject::prepareToPutDirectWithoutTransition):
>+        (JSC::JSObject::putDirectInternal):
>+
> 2018-07-27  Mark Lam  <mark.lam@apple.com>
> 
>         Add some crash info to Heap::checkConn() RELEASE_ASSERTs.
>Index: Source/JavaScriptCore/runtime/JSCJSValue.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSCJSValue.h	(revision 234345)
>+++ Source/JavaScriptCore/runtime/JSCJSValue.h	(working copy)
>@@ -239,6 +239,8 @@ public:
>     template<typename Target> bool inherits(VM&) const;
>     const ClassInfo* classInfoOrNull(VM&) const;
>         
>+    bool isEffectivelyEmptyAndSafeForGCScanning() const;
>+
>     // Extracting the value.
>     bool getString(ExecState*, WTF::String&) const;
>     WTF::String getString(ExecState*) const; // null string if not a string
>Index: Source/JavaScriptCore/runtime/JSCJSValueInlines.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSCJSValueInlines.h	(revision 234345)
>+++ Source/JavaScriptCore/runtime/JSCJSValueInlines.h	(working copy)
>@@ -527,6 +527,11 @@ ALWAYS_INLINE JSCell* JSValue::asCell() 
> 
> #endif // USE(JSVALUE64)
> 
>+inline bool JSValue::isEffectivelyEmptyAndSafeForGCScanning() const
>+{
>+    return !*this || !u.asInt64;
>+}
>+
> inline int64_t tryConvertToInt52(double number)
> {
>     if (number != number)
>Index: Source/JavaScriptCore/runtime/JSObjectInlines.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSObjectInlines.h	(revision 234345)
>+++ Source/JavaScriptCore/runtime/JSObjectInlines.h	(working copy)
>@@ -195,7 +195,8 @@ ALWAYS_INLINE PropertyOffset JSObject::p
>         vm, propertyName, attributes,
>         [&] (const GCSafeConcurrentJSLocker&, PropertyOffset offset, PropertyOffset newLastOffset) {
>             unsigned newOutOfLineCapacity = Structure::outOfLineCapacity(newLastOffset);
>-            if (newOutOfLineCapacity != oldOutOfLineCapacity) {
>+            bool needToAllocateNewButterfly = (newOutOfLineCapacity != oldOutOfLineCapacity);
>+            if (needToAllocateNewButterfly) {
>                 Butterfly* butterfly = allocateMoreOutOfLineStorage(vm, oldOutOfLineCapacity, newOutOfLineCapacity);
>                 nukeStructureAndSetButterfly(vm, structureID, butterfly);
>                 structure->setLastOffset(newLastOffset);
>@@ -206,7 +207,7 @@ ALWAYS_INLINE PropertyOffset JSObject::p
> 
>             // This assertion verifies that the concurrent GC won't read garbage if the concurrentGC
>             // is running at the same time we put without transitioning.
>-            ASSERT(!getDirect(offset) || !JSValue::encode(getDirect(offset)));
>+            ASSERT(!needToAllocateNewButterfly || getDirect(offset).isEffectivelyEmptyAndSafeForGCScanning());
>             result = offset;
>         });
>     return result;
>@@ -321,7 +322,8 @@ ALWAYS_INLINE bool JSObject::putDirectIn
>             vm, propertyName, value, slot.context() == PutPropertySlot::PutById);
>         
>         Butterfly* newButterfly = butterfly();
>-        if (currentCapacity != newStructure->outOfLineCapacity()) {
>+        bool needToAllocateNewButterfly = (currentCapacity != newStructure->outOfLineCapacity());
>+        if (needToAllocateNewButterfly) {
>             ASSERT(newStructure != this->structure(vm));
>             newButterfly = allocateMoreOutOfLineStorage(vm, currentCapacity, newStructure->outOfLineCapacity());
>             nukeStructureAndSetButterfly(vm, structureID, newButterfly);
>@@ -332,7 +334,7 @@ ALWAYS_INLINE bool JSObject::putDirectIn
> 
>         // This assertion verifies that the concurrent GC won't read garbage if the concurrentGC
>         // is running at the same time we put without transitioning.
>-        ASSERT(!getDirect(offset) || !JSValue::encode(getDirect(offset)));
>+        ASSERT(!needToAllocateNewButterfly || getDirect(offset).isEffectivelyEmptyAndSafeForGCScanning());
>         putDirect(vm, offset, value);
>         setStructure(vm, newStructure);
>         slot.setNewProperty(this, offset);
>@@ -381,14 +383,15 @@ ALWAYS_INLINE bool JSObject::putDirectIn
>     size_t oldCapacity = structure->outOfLineCapacity();
>     size_t newCapacity = newStructure->outOfLineCapacity();
>     ASSERT(oldCapacity <= newCapacity);
>-    if (oldCapacity != newCapacity) {
>+    bool needToAllocateNewButterfly = (oldCapacity != newCapacity);
>+    if (needToAllocateNewButterfly) {
>         Butterfly* newButterfly = allocateMoreOutOfLineStorage(vm, oldCapacity, newCapacity);
>         nukeStructureAndSetButterfly(vm, structureID, newButterfly);
>     }
> 
>     // This assertion verifies that the concurrent GC won't read garbage if the concurrentGC
>     // is running at the same time we put without transitioning.
>-    ASSERT(!getDirect(offset) || !JSValue::encode(getDirect(offset)));
>+    ASSERT(!needToAllocateNewButterfly || getDirect(offset).isEffectivelyEmptyAndSafeForGCScanning());
>     putDirect(vm, offset, value);
>     setStructure(vm, newStructure);
>     slot.setNewProperty(this, offset);

Actions: View | Formatted Diff | Diff

Attachments on bug 188151: 346046 | 350877