Bug 153908 – Arrayify for a typed array shouldn't create a monster

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 270724 [details]
the patch

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

(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.

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/196179

[Mark Lam]

*** Bug 153487 has been marked as a duplicate of this bug. ***