Bug 220323 – [WASM-References] Add optional default value parameter for Table.constructor, Table.grow and Table.set

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Dmitry]

[WASM-References] Add optional default value parameter for Table.grow

[Dmitry]

Created attachment 416994 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416994 [details]
Patch

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

r- since I found a bug. We also need description more about the change.

> Source/JavaScriptCore/ChangeLog:8
> +        Introduce the Table.grow optional default value.

ChangeLog is super important in WebKit project.
Please add more comments about changes in ChangeLog.
We need more description about the change in particular, for feature change.

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:100
> +    JSValue defaultValue = callFrame->argument(1);
> +    if (defaultValue.isUndefined())
> +        defaultValue = jsNull();
>      uint32_t oldLength = table->length();
>  
> -    if (!table->grow(delta))
> +    if (!table->grow(delta, defaultValue))
>          return JSValue::encode(throwException(globalObject, throwScope, createRangeError(globalObject, "WebAssembly.Table.prototype.grow could not grow the table"_s)));

I think this has a bug.
If table is function ref table, then values that can be configured to the table is limited.
Can you align the implementation to the spec's change? https://webassembly.github.io/reference-types/js-api/index.html#dom-table-grow

5. If value is missing,
5.1. Let ref be DefaultValue(elementType).
6. Otherwise,
6.1. Let ref be ? ToWebAssemblyValue(value, elementType).

[Yusuke Suzuki]

Comment on attachment 416994 [details]
Patch

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:100
>>          return JSValue::encode(throwException(globalObject, throwScope, createRangeError(globalObject, "WebAssembly.Table.prototype.grow could not grow the table"_s)));
> 
> I think this has a bug.
> If table is function ref table, then values that can be configured to the table is limited.
> Can you align the implementation to the spec's change? https://webassembly.github.io/reference-types/js-api/index.html#dom-table-grow
> 
> 5. If value is missing,
> 5.1. Let ref be DefaultValue(elementType).
> 6. Otherwise,
> 6.1. Let ref be ? ToWebAssemblyValue(value, elementType).

And can you add tests for the above thing :) ?

[Dmitry]

Created attachment 417077 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 417077 [details]
Patch

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

r=me with EWS fix. LayoutTest contain reference-types tests for Tables. expect files need to be rebaselined.

> Source/JavaScriptCore/ChangeLog:3
> +        [WASM-References] Add optional default value parameter for Table.grow

Let's change the title since this patch includes constructor and Table#set support too.

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:124
> +    JSValue jsWebAssemblyTableValue = JSWebAssemblyTable::tryCreate(globalObject, vm, webAssemblyTableStructure, wasmTable.releaseNonNull());

Let's just receive it as `JSWebAssemblyTable* jsWebAssemblyTableValue = ...`.

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:129
> +        JSWebAssemblyTable* table = jsDynamicCast<JSWebAssemblyTable*>(vm, jsWebAssemblyTableValue);

Remove this line.

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:98
> +        return JSValue::encode(throwException(globalObject, throwScope, createTypeError(globalObject, "WebAssembly.Table.prototype.grow expects the second argument to be null or an instance of WebAssembly.Function"_s)));

Use `throwVMTypeError` instead.

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:101
> +    if (!table->grow(delta, defaultValue))

Let's change the old code with throwVMTypeError.

> JSTests/ChangeLog:3
> +        [WASM-References] Add optional default value parameter for Table.grow

Ditto.

[Yusuke Suzuki]

Hmmm, it seems that many imported wpt tests are failing because of WebAssembly.Table's extension. And sometimes it involves many other non-related Table tests. This is not great since it regresses the coverage of testing.
Can you import js-api tests separately from https://github.com/WebAssembly/reference-types and ensure they are passing?

[Dmitry]

Created attachment 417157 [details]
Patch

[Dmitry]

Created attachment 417172 [details]
Patch

[Dmitry]

Created attachment 417243 [details]
Patch

[EWS]

Committed r271303: <https://trac.webkit.org/changeset/271303>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417243 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72934842>