[webkit-reviews] review denied: [Bug 174516] Butterfly storage need not be initialized for indexing type Undecided. : [Attachment 333923] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 174516: Butterfly storage need not be initialized for indexing type
Undecided.
https://bugs.webkit.org/show_bug.cgi?id=174516

Attachment 333923: Patch

https://bugs.webkit.org/attachment.cgi?id=333923&action=review




--- Comment #19 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 333923
  --> https://bugs.webkit.org/attachment.cgi?id=333923
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +	   That patch had revealed an underlying bug:
arrayProtoPrivateFunConcatMemcpy forgot to initialize
> +	   part of its result when given an array with undecided and an array
with doubles.
> +	   As a result it was rolled-out in r227435
> +

Is there anywhere else that might depend on this behavior?

> Source/JavaScriptCore/runtime/ArrayConventions.h:154
> +    else if (type == ArrayWithInt32 || type == ArrayWithContiguous)

What if these aren't arrays? Or are we guaranteed type here has the IsArray bit
set? If so, it's worth an assert.

e.g, do we see Int32Shape/ContigousShape here?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1206
> +    } else if (UNLIKELY(source->indexingType() == ArrayWithUndecided)) {

Do we not care about undecided shape? Or only ArrayWithUndecided?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1213
> +	       JSValue value = source->tryGetIndexQuickly(i);

This changes the code to no longer test if the result is empty.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1221
> +static bool moveElements(ExecState* exec, VM& vm, JSArray* target, unsigned
targetOffset, JSArray* source)

Maybe we should make this not return a boolean. Either it throws or it
succeeds.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1267
> +    if (UNLIKELY(!success))
> +	   return encodedJSValue();

It would be wrong to just return JSValue() to JS code. A javascript call should
never return JSValue(). However, this code is not wrong, since moveElements
*only* returns false if an exception happens. Let's just turn this into an
assert then. I'm not sure why we had this check before given the behavior.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1320
> +    if (type == NonArray) {

I don't get how this is correct. mergeIndexingTypeForCopying returns NonArray
for ArrayStorage.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1340
> +    if (UNLIKELY(!success))
> +	   return encodedJSValue();

ditto here. This branch is never taken.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1344
> +    if (UNLIKELY(!success))
>	   return encodedJSValue();

ditto here, and you don't reassign to success, so it would've done nothing
regardless.