Attachment 343417 Details for Bug 186960 – Patch

[patch] Patch

bug-186960-20180622210947.patch (text/plain), 7.75 KB, created by Keith Miller on 2018-06-22 21:09:48 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-06-22 21:09:48 PDT

Size: 7.75 KB

patch

obsolete

>Subversion Revision: 233110
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index f143143d747d74d8aef92085aba21c2e7240b389..1c9377e7a9622806d94d58e9b7e3f3cb32e35050 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-06-22  Keith Miller  <keith_miller@apple.com>
>+
>+        unshift should zero unused property storage
>+        https://bugs.webkit.org/show_bug.cgi?id=186960
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch adds the zeroed unused property storage assertion to
>+        one more place it was missing.
>+
>+        * runtime/JSArray.cpp:
>+        (JSC::JSArray::unshiftCountSlowCase):
>+        * runtime/JSObjectInlines.h:
>+        (JSC::JSObject::putDirectInternal):
>+
> 2018-06-22  Keith Miller  <keith_miller@apple.com>
> 
>         performProxyCall should toThis the value passed to its handler
>diff --git a/Source/JavaScriptCore/runtime/JSArray.cpp b/Source/JavaScriptCore/runtime/JSArray.cpp
>index 83b5145f2ab0176af43db9a58f4275a33710270c..731e45b36e77a8a422ad527e03cea8b59a23ac74 100644
>--- a/Source/JavaScriptCore/runtime/JSArray.cpp
>+++ b/Source/JavaScriptCore/runtime/JSArray.cpp
>@@ -347,7 +347,7 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
>     // Step 2:
>     // We're either going to choose to allocate a new ArrayStorage, or we're going to reuse the existing one.
> 
>-    void* newAllocBase = 0;
>+    void* newAllocBase = nullptr;
>     unsigned newStorageCapacity;
>     bool allocatedNewStorage;
>     // If the current storage array is sufficiently large (but not too large!) then just keep using it.
>@@ -356,11 +356,11 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
>         newStorageCapacity = currentCapacity;
>         allocatedNewStorage = false;
>     } else {
>-        size_t newSize = Butterfly::totalSize(0, propertyCapacity, true, ArrayStorage::sizeFor(desiredCapacity));
>-        newAllocBase = vm.jsValueGigacageAuxiliarySpace.allocateNonVirtual(
>-            vm, newSize, nullptr, AllocationFailureMode::ReturnNull);
>-        if (!newAllocBase)
>+        const unsigned preCapacity = 0;
>+        Butterfly* newButterfly = Butterfly::tryCreateUninitialized(vm, this, preCapacity, propertyCapacity, true, ArrayStorage::sizeFor(desiredCapacity));
>+        if (!newButterfly)
>             return false;
>+        newAllocBase = newButterfly->base(preCapacity, propertyCapacity);
>         newStorageCapacity = desiredCapacity;
>         allocatedNewStorage = true;
>     }
>@@ -385,23 +385,25 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
> 
>     unsigned newVectorLength = requiredVectorLength + postCapacity;
>     RELEASE_ASSERT(newVectorLength <= MAX_STORAGE_VECTOR_LENGTH);
>-    unsigned newIndexBias = newStorageCapacity - newVectorLength;
>+    unsigned preCapacity = newStorageCapacity - newVectorLength;
> 
>-    Butterfly* newButterfly = Butterfly::fromBase(newAllocBase, newIndexBias, propertyCapacity);
>+    Butterfly* newButterfly = Butterfly::fromBase(newAllocBase, preCapacity, propertyCapacity);
> 
>     if (addToFront) {
>         ASSERT(count + usedVectorLength <= newVectorLength);
>         memmove(newButterfly->arrayStorage()->m_vector + count, storage->m_vector, sizeof(JSValue) * usedVectorLength);
>         memmove(newButterfly->propertyStorage() - propertySize, butterfly->propertyStorage() - propertySize, sizeof(JSValue) * propertySize + sizeof(IndexingHeader) + ArrayStorage::sizeFor(0));
>-        
>+
>         if (allocatedNewStorage) {
>             // We will set the vectorLength to newVectorLength. We populated requiredVectorLength
>             // (usedVectorLength + count), which is less. Clear the difference.
>             for (unsigned i = requiredVectorLength; i < newVectorLength; ++i)
>                 newButterfly->arrayStorage()->m_vector[i].clear();
>+            // We don't need to zero the pre-capacity because it is not available to use as property storage.
>+            memset(newButterfly->base(0, propertyCapacity), 0, (propertyCapacity - propertySize) * sizeof(JSValue));
>         }
>-    } else if ((newAllocBase != butterfly->base(structure)) || (newIndexBias != storage->m_indexBias)) {
>-        memmove(newButterfly->propertyStorage() - propertySize, butterfly->propertyStorage() - propertySize, sizeof(JSValue) * propertySize + sizeof(IndexingHeader) + ArrayStorage::sizeFor(0));
>+    } else if ((newAllocBase != butterfly->base(structure)) || (preCapacity != storage->m_indexBias)) {
>+        memmove(newButterfly->propertyStorage() - propertyCapacity, butterfly->propertyStorage() - propertyCapacity, sizeof(JSValue) * propertyCapacity + sizeof(IndexingHeader) + ArrayStorage::sizeFor(0));
>         memmove(newButterfly->arrayStorage()->m_vector, storage->m_vector, sizeof(JSValue) * usedVectorLength);
>         
>         for (unsigned i = requiredVectorLength; i < newVectorLength; i++)
>@@ -409,7 +411,7 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
>     }
> 
>     newButterfly->arrayStorage()->setVectorLength(newVectorLength);
>-    newButterfly->arrayStorage()->m_indexBias = newIndexBias;
>+    newButterfly->arrayStorage()->m_indexBias = preCapacity;
>     
>     setButterfly(vm, newButterfly);
> 
>diff --git a/Source/JavaScriptCore/runtime/JSObjectInlines.h b/Source/JavaScriptCore/runtime/JSObjectInlines.h
>index 5e93e26d42184c4ad2341aed96d265fda0a5d93e..1a0888ef8c5d61512a2be91e270bb69ed396b81c 100644
>--- a/Source/JavaScriptCore/runtime/JSObjectInlines.h
>+++ b/Source/JavaScriptCore/runtime/JSObjectInlines.h
>@@ -329,6 +329,10 @@ ALWAYS_INLINE bool JSObject::putDirectInternal(VM& vm, PropertyName propertyName
> 
>         validateOffset(offset);
>         ASSERT(newStructure->isValidOffset(offset));
>+
>+        // 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)));
>         putDirect(vm, offset, value);
>         setStructure(vm, newStructure);
>         slot.setNewProperty(this, offset);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 2197c3cf7b97fb622617874999a1e2089968c0a0..fce2da024cc6ddf8b0d117c83574336765480c85 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-06-22  Keith Miller  <keith_miller@apple.com>
>+
>+        unshift should zero unused property storage
>+        https://bugs.webkit.org/show_bug.cgi?id=186960
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/array-unshift-zero-property-storage.js: Added.
>+        (run):
>+        (test):
>+
> 2018-06-22  Keith Miller  <keith_miller@apple.com>
> 
>         performProxyCall should toThis the value passed to its handler
>diff --git a/JSTests/stress/array-unshift-zero-property-storage.js b/JSTests/stress/array-unshift-zero-property-storage.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..b4ed0332b9251c7b03be18883b41f3b8a0e25f65
>--- /dev/null
>+++ b/JSTests/stress/array-unshift-zero-property-storage.js
>@@ -0,0 +1,34 @@
>+let foo = "get some property storage";
>+let first = "new first element";
>+let bar = "ensure property storage is zeroed";
>+
>+function run(array) {
>+    array.foo = foo;
>+    array.unshift(first, ...new Array(100));
>+    array.bar = bar;
>+    return array;
>+}
>+noInline(run);
>+
>+function test() {
>+    let array = run([]);
>+    if (array.foo !== foo)
>+        throw new Error();
>+    if (array.bar !== bar)
>+        throw new Error();
>+    if (array[0] !== first)
>+        throw new Error();
>+
>+    array = [];
>+    array.unshift(1);
>+    array = run(array);
>+    if (array.foo !== foo)
>+        throw new Error();
>+    if (array.bar !== bar)
>+        throw new Error();
>+    if (array[0] !== first)
>+        throw new Error();
>+}
>+
>+for (let i = 0; i < 1; i++)
>+    test();

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186960: 343417