Bug 187543 – constructArray() should always allocate the requested length.

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

[Mark Lam]

Currently, it does not when we're having a bad time.

<rdar://problem/41947884>

[Mark Lam]

Created attachment 344745 [details]
proposed patch.

[Saam Barati]

Comment on attachment 344745 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=344745&action=review

> Source/JavaScriptCore/runtime/JSArray.cpp:1384
> +    // FIXME: We only need this for subclasses of Array because we might need to allocate a new structure to change
> +    // indexing types while initializing. If this triggered a GC then we might scan our currently uninitialized
> +    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811

I'm confused. Why don't we just select the correct indexing type from the beginning? I also don't understand why this just effects subclasses but nothing else

[Mark Lam]

Comment on attachment 344745 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=344745&action=review

>> Source/JavaScriptCore/runtime/JSArray.cpp:1384
>> +    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
> 
> I'm confused. Why don't we just select the correct indexing type from the beginning? I also don't understand why this just effects subclasses but nothing else

Because we don't know the final indexing type of the array up front.  For context, expand the code below and see the 2 other variants of constructArray() and a constructArrayNegativeIndexed() that calls this constructArray().  Those 3 functions will iterate some list of JSValues and initialize the butterfly with the values that come in.  In the process of doing so, the indexing type may change.  If the structure is for the original array indexing types, then the structures transitioned to are one of the other pre-allocated array structures i.e. no allocation needed, and no GC can be triggered.  If we have a subclass, then new structures will need to be allocated, and therefore GC can be triggered.  The GC can therefore see the uninitialized values in the butterfly.

BTW, this comment came from the original code above.

[Saam Barati]

Comment on attachment 344745 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=344745&action=review

r=me

> Source/JavaScriptCore/runtime/JSArray.cpp:109
> +void JSArray::eagerlyInitializeButterfly(ObjectInitializationScope& scope, JSArray* array, unsigned initialLength)

It feels like this could perhaps have an even more specific name since it's used in one super special place.

[Mark Lam]

Comment on attachment 344745 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=344745&action=review

Thanks for the review.

>> Source/JavaScriptCore/runtime/JSArray.cpp:109
>> +void JSArray::eagerlyInitializeButterfly(ObjectInitializationScope& scope, JSArray* array, unsigned initialLength)
> 
> It feels like this could perhaps have an even more specific name since it's used in one super special place.

I couldn't think of a better name, and still can't yet.  I'll just keep it as is for now.  We can always rename it if we think of a better name later.

[Mark Lam]

Landed in r233722: <http://trac.webkit.org/r233722>.