Bug 219600 – Support JS type reflections in WebAssembly JS-API

• Component: JavaScriptCore
• Status: RESOLVED FIXED

Support JS type reflections in the JS-API, an overview of the API changes are outlined here: https://github.com/WebAssembly/js-types/blob/master/proposals/js-types/Overview.md

Created attachment 415554 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415554 [details]
Patch

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

Looks good, but put r- since I found bugs, And, can you update the LayoutTests results to make EWS green?

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:74
> +        JSValue minSizeValue = memoryDescriptor->get(globalObject, minimum);

We need

RETURN_IF_EXCEPTION(throwScope, encodedJSValue());

here since `->get(...)` can throw an error.

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:85
> +    JSValue minSizeValue = memoryDescriptor->get(globalObject, minimumIdent);

Ditto.

[Yusuke Suzuki]

Comment on attachment 415554 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:77
> +            return JSValue::encode(throwException(globalObject, throwScope, createTypeError(globalObject, "WebAssembly.Memory specify exactly one of 'initial' or 'minimum'")));

Use `return throwVMTypeError(...)`.

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:87
> +        return JSValue::encode(throwException(globalObject, throwScope, createTypeError(globalObject, "WebAssembly.Table exactly one of 'initial' or 'minimum'")));

Ditto.

Created attachment 415736 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415736 [details]
Patch

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

Can you update LayoutTests results to make EWS green?
Currently, win, mac-wk1, mac-wk2, and mac-debug-wk1 are red.

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:78
> +            return throwVMTypeError(globalObject, throwScope, "WebAssembly.Memory specify exactly one of 'initial' or 'minimum'");

Maybe, "'initial' and 'minimum' options are specified at the same time" would be better.

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:88
> +        return throwVMTypeError(globalObject, throwScope, "WebAssembly.Table exactly one of 'initial' or 'minimum'");

Ditto.

[Radar WebKit Bug Importer]

<rdar://problem/72295339>

Created attachment 416294 [details]
Patch

I've amended the patch to include the improved error message. I took a look at the failing tests and it looks like they're web-platform tests for the .type() method on WebAssembly.Memory, WebAssembly.Table and WebAssembly.Global. Those along with WebAssembly.Function I plan to make in a future pull requests, they should be failing currently on master too as it's currently missing. I was planning to land this in smaller sections (minimum constructor parameter, .type method and then WebAssembly.Function implementation).

Thanks for the review!

[Yusuke Suzuki]

(In reply to jtallon from comment #8)
> I've amended the patch to include the improved error message. I took a look
> at the failing tests and it looks like they're web-platform tests for the
> .type() method on WebAssembly.Memory, WebAssembly.Table and
> WebAssembly.Global. Those along with WebAssembly.Function I plan to make in
> a future pull requests, they should be failing currently on master too as
> it's currently missing. I was planning to land this in smaller sections
> (minimum constructor parameter, .type method and then WebAssembly.Function
> implementation).
> 
> Thanks for the review!

In WebKit, we cannot land the patch with failing patch.
Can you update the xxx-expected.txt files in LayoutTests to make them green? :)

[Yusuke Suzuki]

Comment on attachment 416294 [details]
Patch

Needing rebaseline of LayoutTests expected files.

[Yusuke Suzuki]

You can see what LayoutTests are failing.
If this failure is due to updated expected.txt files, you can update `LayoutTests/.../test-name-expected.txt` to update the expectation of these tests.
Then you can make it green.
If the failure is due to the bug of the patch, we need to fix the bug before landing.

In this case, it seems that tests are failing because expectation is updated. So please update xxx-expected.txt files to update expectations of these tests.
https://ews-build.s3-us-west-2.amazonaws.com/macOS-Mojave-Release-WK2-Tests-EWS/r416294-24678/results.html

Thanks!

Created attachment 416502 [details]
Patch

Created attachment 416508 [details]
Patch

Created attachment 416510 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416510 [details]
Patch

r=me

[EWS]

/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Yusuke Suzuki]

Comment on attachment 416510 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:5
> +

Please keep `Reviewed by NOBODY (OOPS!).` thing.
This will be replaced by commit-queue, and we cannot land it without that.

ChangeLog format is,

2020-12-07 Jessica Tallon <[e-mail]>

        [JSC] Add minimum parameter to the WASM JS-API for Memory & Table.
        https://bugs.webkit.org/show_bug.cgi?id=219600

        Reviewed by NOBODY (OOPS!).

        Comments.

> LayoutTests/ChangeLog:5
> +

Please keep `Reviewed by NOBODY (OOPS!).` thing.
This will be replaced by commit-queue, and we cannot land it without that.

Created attachment 416600 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416600 [details]
Patch

r=me

[EWS]

Committed r271039: <https://trac.webkit.org/changeset/271039>

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

[Truitt Savell]

The changes in r271039 appear to have broken fast/images/webp-as-image.html on Big Sur

History:
https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as-image.html

Diff:
https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r271064%20(686)/fast/images/webp-as-image-diffs.html

it looks like this test is failing to load an image

[Yusuke Suzuki]

(In reply to Truitt Savell from comment #21)
> The changes in r271039 appear to have broken fast/images/webp-as-image.html
> on Big Sur
> 
> History:
> https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as-
> image.html
> 
> Diff:
> https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/
> r271064%20(686)/fast/images/webp-as-image-diffs.html
> 
> it looks like this test is failing to load an image

Hmmmmmmmmm, this sounds super strange. Literally, this patch only changes WebAssembly part.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #22)
> (In reply to Truitt Savell from comment #21)
> > The changes in r271039 appear to have broken fast/images/webp-as-image.html
> > on Big Sur
> > 
> > History:
> > https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as-
> > image.html
> > 
> > Diff:
> > https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/
> > r271064%20(686)/fast/images/webp-as-image-diffs.html
> > 
> > it looks like this test is failing to load an image
> 
> Hmmmmmmmmm, this sounds super strange. Literally, this patch only changes
> WebAssembly part.

That test is 

```
<body>
    <img src="resources/green-400x400.webp">
</body>
```

And the test failure is because the image is not loaded in the actual result.
I don't think this patch is related to this test failure.
My guess is library update for webp etc.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #23)
> (In reply to Yusuke Suzuki from comment #22)
> > (In reply to Truitt Savell from comment #21)
> > > The changes in r271039 appear to have broken fast/images/webp-as-image.html
> > > on Big Sur
> > > 
> > > History:
> > > https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as-
> > > image.html
> > > 
> > > Diff:
> > > https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/
> > > r271064%20(686)/fast/images/webp-as-image-diffs.html
> > > 
> > > it looks like this test is failing to load an image
> > 
> > Hmmmmmmmmm, this sounds super strange. Literally, this patch only changes
> > WebAssembly part.
> 
> That test is 
> 
> ```
> <body>
>     <img src="resources/green-400x400.webp">
> </body>
> ```
> 
> And the test failure is because the image is not loaded in the actual result.
> I don't think this patch is related to this test failure.
> My guess is library update for webp etc.

Reproduced this failure with r271038. So this revision is not related to the failure.

[Alexey Proskuryakov]

Right, we had the version of macOS updated right then. This is almost certainly a dupe of bug 219977.