Bug 153908

Summary: Arrayify for a typed array shouldn't create a monster
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: andre.bargull, commit-queue, ggaren, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch mark.lam: review+

Description Filip Pizlo 2016-02-04 20:59:53 PST
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.
Comment 1 Filip Pizlo 2016-02-04 21:02:09 PST
Created attachment 270724 [details]
the patch
Comment 2 Mark Lam 2016-02-05 10:03:55 PST
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.
Comment 3 Filip Pizlo 2016-02-05 10:34:14 PST
(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.
Comment 4 Filip Pizlo 2016-02-05 10:36:56 PST
(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 5 Mark Lam 2016-02-05 10:40:08 PST
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.
Comment 6 Filip Pizlo 2016-02-05 11:20:30 PST
(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.
Comment 7 Filip Pizlo 2016-02-05 11:52:45 PST
Landed in http://trac.webkit.org/changeset/196179
Comment 8 Mark Lam 2016-07-13 14:07:52 PDT
*** Bug 153487 has been marked as a duplicate of this bug. ***