WebKit Bugzilla
Attachment 346046 Details for
Bug 188151
: We should zero unused property storage when rebalancing array storage.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
proposed patch.
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);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 188151
:
346046
|
350877