Arrayify over a typed array will repurpose the indexing header for something different than what typed arrays expect, leading to an object that's in a bizarre state.
Created attachment 270724 [details] the patch
Comment on attachment 270724 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=270724&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:597 > - // this case if we ever cared. > - enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, ensureArrayStorageSlow(vm)); > + // this case if we ever cared. Note that ensureArrayStorage() can return null if the object > + // doesn't support traditional indexed properties. At the time of writing, this just affects > + // typed arrays. > + if (ArrayStorage* storage = ensureArrayStorageSlow(vm)) > + enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, storage); JSObject::enterDictionaryIndexingMode() seems to be called from JSArray::setLengthWritable() and JSObject::preventExtensions() with the assumption that it will succeed in entering indexing mode. Can you please explain why it is ok to simply do nothing is we fail to ensureArrayStorageSlow()? > Source/JavaScriptCore/runtime/JSObject.cpp:2548 > - > - return Butterfly::createOrGrowPropertyStorage(m_butterfly.get(this), vm, this, structure(vm), oldSize, newSize); > + > + Butterfly* result = Butterfly::createOrGrowPropertyStorage(m_butterfly.get(this), vm, this, structure(vm), oldSize, newSize); > + > + return result; I think this is not needed. Please revert.
(In reply to comment #2) > Comment on attachment 270724 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270724&action=review > > > Source/JavaScriptCore/runtime/JSObject.cpp:597 > > - // this case if we ever cared. > > - enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, ensureArrayStorageSlow(vm)); > > + // this case if we ever cared. Note that ensureArrayStorage() can return null if the object > > + // doesn't support traditional indexed properties. At the time of writing, this just affects > > + // typed arrays. > > + if (ArrayStorage* storage = ensureArrayStorageSlow(vm)) > > + enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, storage); > > JSObject::enterDictionaryIndexingMode() seems to be called from > JSArray::setLengthWritable() and JSObject::preventExtensions() with the > assumption that it will succeed in entering indexing mode. Can you please > explain why it is ok to simply do nothing is we fail to > ensureArrayStorageSlow()? JSArray will always return non-null from ensureArrayStorageSlow(), since a JSArray is not a typed array. > > > Source/JavaScriptCore/runtime/JSObject.cpp:2548 > > - > > - return Butterfly::createOrGrowPropertyStorage(m_butterfly.get(this), vm, this, structure(vm), oldSize, newSize); > > + > > + Butterfly* result = Butterfly::createOrGrowPropertyStorage(m_butterfly.get(this), vm, this, structure(vm), oldSize, newSize); > > + > > + return result; > > I think this is not needed. Please revert. Sure.
(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 270724 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=270724&action=review > > > > > Source/JavaScriptCore/runtime/JSObject.cpp:597 > > > - // this case if we ever cared. > > > - enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, ensureArrayStorageSlow(vm)); > > > + // this case if we ever cared. Note that ensureArrayStorage() can return null if the object > > > + // doesn't support traditional indexed properties. At the time of writing, this just affects > > > + // typed arrays. > > > + if (ArrayStorage* storage = ensureArrayStorageSlow(vm)) > > > + enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, storage); > > > > JSObject::enterDictionaryIndexingMode() seems to be called from > > JSArray::setLengthWritable() and JSObject::preventExtensions() with the > > assumption that it will succeed in entering indexing mode. Can you please > > explain why it is ok to simply do nothing is we fail to > > ensureArrayStorageSlow()? > > JSArray will always return non-null from ensureArrayStorageSlow(), since a > JSArray is not a typed array. And typed arrays are unaffected by things like preventExtensions(). Broadly, there's no effect on typed arrays from doing enterDictionaryIndexingMode() since typed arrays override normal JS indexing behavior already and so we just need to make sure that the usual JSObject indexing stuff doesn't do anything. In trunk, JSObject indexing stuff sometimes corrupts the typed array's state, and this patch fixes that.
Comment on attachment 270724 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=270724&action=review r=me >>>> Source/JavaScriptCore/runtime/JSObject.cpp:597 >>>> + enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, storage); >>> >>> JSObject::enterDictionaryIndexingMode() seems to be called from JSArray::setLengthWritable() and JSObject::preventExtensions() with the assumption that it will succeed in entering indexing mode. Can you please explain why it is ok to simply do nothing is we fail to ensureArrayStorageSlow()? >> >> JSArray will always return non-null from ensureArrayStorageSlow(), since a JSArray is not a typed array. > > And typed arrays are unaffected by things like preventExtensions(). Broadly, there's no effect on typed arrays from doing enterDictionaryIndexingMode() since typed arrays override normal JS indexing behavior already and so we just need to make sure that the usual JSObject indexing stuff doesn't do anything. > > In trunk, JSObject indexing stuff sometimes corrupts the typed array's state, and this patch fixes that. Sounds good. I feel like we should have an else case here for when !storage, and ASSERT that the self object is a TypedArray in that case.
(In reply to comment #5) > Comment on attachment 270724 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270724&action=review > > r=me > > >>>> Source/JavaScriptCore/runtime/JSObject.cpp:597 > >>>> + enterDictionaryIndexingModeWhenArrayStorageAlreadyExists(vm, storage); > >>> > >>> JSObject::enterDictionaryIndexingMode() seems to be called from JSArray::setLengthWritable() and JSObject::preventExtensions() with the assumption that it will succeed in entering indexing mode. Can you please explain why it is ok to simply do nothing is we fail to ensureArrayStorageSlow()? > >> > >> JSArray will always return non-null from ensureArrayStorageSlow(), since a JSArray is not a typed array. > > > > And typed arrays are unaffected by things like preventExtensions(). Broadly, there's no effect on typed arrays from doing enterDictionaryIndexingMode() since typed arrays override normal JS indexing behavior already and so we just need to make sure that the usual JSObject indexing stuff doesn't do anything. > > > > In trunk, JSObject indexing stuff sometimes corrupts the typed array's state, and this patch fixes that. > > Sounds good. I feel like we should have an else case here for when > !storage, and ASSERT that the self object is a TypedArray in that case. If anything the assert would say something about hijacksIndexingHeader(). I created that method to make it easier to add other typed-array-like objects in the future.
Landed in http://trac.webkit.org/changeset/196179
*** Bug 153487 has been marked as a duplicate of this bug. ***