Bug 164453 – test262: DataView with explicit undefined byteLength should be the same as it not being present

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
DataView with explicit undefined byteLength should be the same as it not being present

Currently we coerce the undefined to 0 with normal number conversions.

Test:
var buffer = new ArrayBuffer(8);
assert( new DataView(buffer, 0).byteLength === new DataView(buffer, 0, undefined).byteLength )

Spec:
> 24.2.2.1 DataView ( buffer [ , byteOffset [ , byteLength ] ] )
> https://tc39.github.io/ecma262/#sec-dataview-buffer-byteoffset-bytelength
>
> When the DataView is called with at least one argument buffer, the following steps are taken:
>
> ...
>     8. If byteLength is either not present or undefined, then
>         Let viewByteLength be bufferByteLength - offset.
>     9. Else,
>         Let viewByteLength be ? ToIndex(byteLength).
>         If offset+viewByteLength > bufferByteLength, throw a RangeError exception.
> ...

[Joseph Pecoraro]

Created attachment 293998 [details]
[PATCH] Proposed Fix

[Darin Adler]

Comment on attachment 293998 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:248
>          if (argCount > 2) {
> -            length = exec->uncheckedArgument(2).toIndex(exec, ViewClass::TypedArrayStorageType == TypeDataView ? "byteLength" : "length");
> -            RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +            if (ViewClass::TypedArrayStorageType == TypeDataView) {
> +                // If the DataView byteLength is present but undefined, treat it as missing.
> +                JSValue byteLengthValue = exec->uncheckedArgument(2);
> +                if (!byteLengthValue.isUndefined()) {
> +                    length = byteLengthValue.toIndex(exec, "byteLength");
> +                    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +                }
> +            } else {
> +                length = exec->uncheckedArgument(2).toIndex(exec, "length");
> +                RETURN_IF_EXCEPTION(scope, encodedJSValue()); 
> +            }
>          }

The old school way of doing this is to call exec->argument(2) and check for undefined. It would be pretty easy to do it that way.

    if (ViewClass::TypedArrayStorageType == TypeDataView) {
        // If the DataView byteLength is present but undefined, treat it as missing.
        JSValue byteLengthValue = exec->argument(2);
        if (!byteLength.isUndefined()) {
            length = byteLengthValue.toIndex(exec, "byteLength");
            RETURN_IF_EXCEPTION(scope, encodedJSValue());
        }
    } else {
        if (argCount > 2) {
            length = exec->uncheckedArgument(2).toIndex(exec, "length");
            RETURN_IF_EXCEPTION(scope, encodedJSValue());
        }
    }

Not sure if it’s better, though. The code it generates is possibly slightly less efficient and I’m not sure it’s easier to read.

[Joseph Pecoraro]

Comment on attachment 293998 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:248
>>          }
> 
> The old school way of doing this is to call exec->argument(2) and check for undefined. It would be pretty easy to do it that way.
> 
>     if (ViewClass::TypedArrayStorageType == TypeDataView) {
>         // If the DataView byteLength is present but undefined, treat it as missing.
>         JSValue byteLengthValue = exec->argument(2);
>         if (!byteLength.isUndefined()) {
>             length = byteLengthValue.toIndex(exec, "byteLength");
>             RETURN_IF_EXCEPTION(scope, encodedJSValue());
>         }
>     } else {
>         if (argCount > 2) {
>             length = exec->uncheckedArgument(2).toIndex(exec, "length");
>             RETURN_IF_EXCEPTION(scope, encodedJSValue());
>         }
>     }
> 
> Not sure if it’s better, though. The code it generates is possibly slightly less efficient and I’m not sure it’s easier to read.

Oh I see, moving the argCount check around. I like the readability advantages of having argCount around both of these (and given the pattern of other code in this function).

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/208640>