Bug 199783

Summary: [JSC] Improve wasm wpt test results by fixing miscellaneous issues
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, jfbastien, mark.lam, mcatanzaro, rmorisset, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Current Score
none
Patch
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch mark.lam: review+

Description Yusuke Suzuki 2019-07-14 03:30:53 PDT
...
Comment 1 Yusuke Suzuki 2019-07-14 03:31:21 PDT
Created attachment 374083 [details]
Current Score
Comment 2 Yusuke Suzuki 2019-07-14 04:04:49 PDT
Created attachment 374084 [details]
Patch
Comment 3 EWS Watchlist 2019-07-14 04:09:19 PDT
Attachment 374084 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:342:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 1 in 163 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Yusuke Suzuki 2019-07-14 04:10:39 PDT
Comment on attachment 374084 [details]
Patch

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

Add several notes.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/constructor/instantiate-bad-imports.any-expected.txt:3
> +Harness Error (FAIL), message = TypeError: undefined is not an object (evaluating 'WebAssembly.Global.prototype')

WebAssembly.Global is not implemented.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/instance/constructor.any-expected.txt:11
> +FAIL exports WebAssembly.Module doesn't parse at byte 87: 3th Export isn't immutable, named 'global' (evaluating 'new WebAssembly.Module(buffer)')

We should fix it in a separate patch.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/instance/constructor.any.worker-expected.txt:11
> +FAIL exports WebAssembly.Module doesn't parse at byte 87: 3th Export isn't immutable, named 'global' (evaluating 'new WebAssembly.Module(buffer)')

We should fix it in a separate patch.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/interface.any-expected.txt:79
> +FAIL WebAssembly.Module.exports assert_equals: expected (undefined) undefined but got (function) function "function exports() {
> +    [native code]
> +}"
> +PASS WebAssembly.Module.exports: name 
> +PASS WebAssembly.Module.exports: length 
> +FAIL WebAssembly.Module.imports assert_equals: expected (undefined) undefined but got (function) function "function imports() {
> +    [native code]
> +}"
> +PASS WebAssembly.Module.imports: name 
> +PASS WebAssembly.Module.imports: length 
> +FAIL WebAssembly.Module.customSections assert_equals: expected (undefined) undefined but got (function) function "function customSections() {
> +    [native code]
> +}"
> +PASS WebAssembly.Module.customSections: name 
> +PASS WebAssembly.Module.customSections: length 
> +PASS WebAssembly.Instance.exports 
> +PASS WebAssembly.Instance.exports: getter 
> +PASS WebAssembly.Instance.exports: setter 
> +FAIL WebAssembly.Memory.grow assert_equals: expected (undefined) undefined but got (function) function "function grow() {
> +    [native code]
> +}"
> +PASS WebAssembly.Memory.grow: name 
> +PASS WebAssembly.Memory.grow: length 
> +PASS WebAssembly.Memory.buffer 
> +PASS WebAssembly.Memory.buffer: getter 
> +PASS WebAssembly.Memory.buffer: setter 
> +FAIL WebAssembly.Table.grow assert_equals: expected (undefined) undefined but got (function) function "function grow() {
> +    [native code]
> +}"
> +PASS WebAssembly.Table.grow: name 
> +PASS WebAssembly.Table.grow: length 
> +FAIL WebAssembly.Table.get assert_equals: expected (undefined) undefined but got (function) function "function get() {
> +    [native code]
> +}"
> +PASS WebAssembly.Table.get: name 
> +PASS WebAssembly.Table.get: length 
> +FAIL WebAssembly.Table.set assert_equals: expected (undefined) undefined but got (function) function "function set() {
> +    [native code]
> +}"

The imported wpt revision is old, and this is bug in the test side. In the upstream, it is fixed.
When upgrading our wpt, we can make them PASS.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/interface.any.worker-expected.txt:80
> +FAIL WebAssembly.Module.exports assert_equals: expected (undefined) undefined but got (function) function "function exports() {
> +    [native code]
> +}"
> +PASS WebAssembly.Module.exports: name 
> +PASS WebAssembly.Module.exports: length 
> +FAIL WebAssembly.Module.imports assert_equals: expected (undefined) undefined but got (function) function "function imports() {
> +    [native code]
> +}"
> +PASS WebAssembly.Module.imports: name 
> +PASS WebAssembly.Module.imports: length 
> +FAIL WebAssembly.Module.customSections assert_equals: expected (undefined) undefined but got (function) function "function customSections() {
> +    [native code]
> +}"
> +PASS WebAssembly.Module.customSections: name 
> +PASS WebAssembly.Module.customSections: length 
> +PASS WebAssembly.Instance.exports 
> +PASS WebAssembly.Instance.exports: getter 
> +PASS WebAssembly.Instance.exports: setter 
> +FAIL WebAssembly.Memory.grow assert_equals: expected (undefined) undefined but got (function) function "function grow() {
> +    [native code]
> +}"
> +PASS WebAssembly.Memory.grow: name 
> +PASS WebAssembly.Memory.grow: length 
> +PASS WebAssembly.Memory.buffer 
> +PASS WebAssembly.Memory.buffer: getter 
> +PASS WebAssembly.Memory.buffer: setter 
> +FAIL WebAssembly.Table.grow assert_equals: expected (undefined) undefined but got (function) function "function grow() {
> +    [native code]
> +}"
> +PASS WebAssembly.Table.grow: name 
> +PASS WebAssembly.Table.grow: length 
> +FAIL WebAssembly.Table.get assert_equals: expected (undefined) undefined but got (function) function "function get() {
> +    [native code]
> +}"
> +PASS WebAssembly.Table.get: name 
> +PASS WebAssembly.Table.get: length 
> +FAIL WebAssembly.Table.set assert_equals: expected (undefined) undefined but got (function) function "function set() {
> +    [native code]
> +}"

WPT implementation is broken, and it is fixed in the upstream.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/module/customSections.any-expected.txt:9
> +FAIL Custom sections with U+FFFD assert_equals: expected 1 but got 0

We should fix it in a separate patch.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/module/customSections.any.worker-expected.txt:9
> +FAIL Custom sections with U+FFFD assert_equals: expected 1 but got 0

We should fix it in a separate patch.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/module/exports.any-expected.txt:7
> +FAIL exports WebAssembly.Module doesn't parse at byte 87: 3th Export isn't immutable, named 'global' (evaluating 'new WebAssembly.Module(buffer)')

We should fix it in a separate patch.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/module/exports.any.worker-expected.txt:7
> +FAIL exports WebAssembly.Module doesn't parse at byte 87: 3th Export isn't immutable, named 'global' (evaluating 'new WebAssembly.Module(buffer)')

We should fix it in the separate patch.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/table/get-set.any-expected.txt:11
> +FAIL Basic assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Growing assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting out-of-bounds assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting non-function assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting non-wasm function assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting non-wasm arrow function assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")

WPT implementation is broken, and it is fixed in the upstream.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/table/get-set.any.worker-expected.txt:11
> +FAIL Basic assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Growing assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting out-of-bounds assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting non-function assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting non-wasm function assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Setting non-wasm arrow function assert_throws: undefined: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")

WPT implementation is broken, and it is fixed in the upstream.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/table/grow.any-expected.txt:6
> +FAIL Basic assert_throws: before: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Reached maximum assert_throws: before: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Exceeded maximum assert_throws: before: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")

WPT implementation is broken, and it is fixed in the upstream.

> LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/table/grow.any.worker-expected.txt:6
> +FAIL Basic assert_throws: before: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Reached maximum assert_throws: before: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")
> +FAIL Exceeded maximum assert_throws: before: table.get(-1) function "() => table.get(-1)" threw object "TypeError: Expect an integer argument in the range: [0, 2^32 - 1]" ("TypeError") expected object "RangeError" ("RangeError")

WPT implementation is broken, and it is fixed in the upstream.
Comment 5 EWS Watchlist 2019-07-14 05:46:25 PDT
Comment on attachment 374084 [details]
Patch

Attachment 374084 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12737001

Number of test failures exceeded the failure limit.
Comment 6 EWS Watchlist 2019-07-14 05:46:26 PDT
Created attachment 374085 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5
Comment 7 EWS Watchlist 2019-07-14 06:07:38 PDT
Comment on attachment 374084 [details]
Patch

Attachment 374084 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12736996

New failing tests:
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-eager-jettison
wasm.yaml/wasm/spec-tests/jsapi.js.default-wasm
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-air
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-tls-context
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-collect-continuously
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-call-ic
apiTests
Comment 8 Yusuke Suzuki 2019-07-15 00:20:49 PDT
Created attachment 374105 [details]
Patch
Comment 9 EWS Watchlist 2019-07-15 00:25:51 PDT
Attachment 374105 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:342:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 1 in 167 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Robin Morisset 2019-07-15 11:40:15 PDT
Comment on attachment 374105 [details]
Patch

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

This looks reasonable to me overall, but I don't feel knowledgeable enough about this code to give it the r=me.

> Source/JavaScriptCore/ChangeLog:12
> +        2. Fix various attributes. It does not match to the usual JSC builtin's convension. But this change

convention => convention

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:205
> +

This space is not needed.

> Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:115
> +    if (Options::useWebAssemblyStreamingApi()) {

I have not seen this in the Changelog, it seems like the kind of thing that could be nice to highlight.
Comment 11 Yusuke Suzuki 2019-07-15 11:56:08 PDT
Comment on attachment 374105 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:12
>> +        2. Fix various attributes. It does not match to the usual JSC builtin's convension. But this change
> 
> convention => convention

Thanks, fixed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:205
>> +
> 
> This space is not needed.

Fixed.

>> Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:115
>> +    if (Options::useWebAssemblyStreamingApi()) {
> 
> I have not seen this in the Changelog, it seems like the kind of thing that could be nice to highlight.

This is (1)'s change: removing WebAssemblyPrototype and merging WebAssemblyPrototype into JSWebAssembly :)
Comment 12 Mark Lam 2019-07-15 12:10:45 PDT
Comment on attachment 374105 [details]
Patch

LGTM
Comment 13 Yusuke Suzuki 2019-07-15 12:27:02 PDT
Committed r247440: <https://trac.webkit.org/changeset/247440>
Comment 14 Radar WebKit Bug Importer 2019-07-15 12:28:22 PDT
<rdar://problem/53116419>
Comment 15 Michael Catanzaro 2019-07-15 13:35:58 PDT
../../Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp: In member function ‘void JSC::JSWebAssembly::finishCreation(JSC::VM&, JSC::JSGlobalObject*)’:
../../Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:116:69: error: ‘webAssemblyCompileStreamingCodeGenerator’ was not declared in this scope
         JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION("compileStreaming", webAssemblyCompileStreamingCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum));
                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/runtime/JSObject.h:1636:58: note: in definition of macro ‘JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION’
         vm, globalObject, makeIdentifier(vm, (jsName)), (generatorName)(vm), (attributes))
                                                          ^~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:116:69: note: suggested alternative: ‘webAssemblyCompileStreamingInternal’
         JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION("compileStreaming", webAssemblyCompileStreamingCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum));
                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/runtime/JSObject.h:1636:58: note: in definition of macro ‘JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION’
         vm, globalObject, makeIdentifier(vm, (jsName)), (generatorName)(vm), (attributes))
                                                          ^~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:117:73: error: ‘webAssemblyInstantiateStreamingCodeGenerator’ was not declared in this scope
         JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION("instantiateStreaming", webAssemblyInstantiateStreamingCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum));
                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/runtime/JSObject.h:1636:58: note: in definition of macro ‘JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION’
         vm, globalObject, makeIdentifier(vm, (jsName)), (generatorName)(vm), (attributes))
                                                          ^~~~~~~~~~~~~
../../Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp:117:73: note: suggested alternative: ‘webAssemblyInstantiateStreamingInternal’
         JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION("instantiateStreaming", webAssemblyInstantiateStreamingCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum));
                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/runtime/JSObject.h:1636:58: note: in definition of macro ‘JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION’
         vm, globalObject, makeIdentifier(vm, (jsName)), (generatorName)(vm), (attributes))
                                                          ^~~~~~~~~~~~~
Comment 16 Michael Catanzaro 2019-07-15 13:41:52 PDT
This affects clean builds and the solution isn't obvious. I'll do a rollout until you have time to look into it!
Comment 17 Michael Catanzaro 2019-07-15 13:42:44 PDT
Reverted r247440 for reason:

Broke builds

Committed r247445: <https://trac.webkit.org/changeset/247445>
Comment 18 Yusuke Suzuki 2019-07-15 15:42:02 PDT
Committed r247457: <https://trac.webkit.org/changeset/247457>
Comment 19 Truitt Savell 2019-07-19 11:01:47 PDT
It looks like the imported/w3c/web-platform-tests/wasm/jsapi/interface.any.worker.html

added in https://trac.webkit.org/changeset/247457/webkit

is flaky on Mac Debug.
History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fwasm%2Fjsapi%2Finterface.any.worker.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/wasm/jsapi/interface.any.worker-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/wasm/jsapi/interface.any.worker-actual.txt
@@ -1,5 +1,3 @@
-CONSOLE MESSAGE: line 1774: TypeError: null is not an object (evaluating 'this.message_target.removeEventListener')
-CONSOLE MESSAGE: line 154: TypeError: undefined is not an object (evaluating 'WebAssembly.Global.prototype')
 
 Harness Error (FAIL), message = TypeError: undefined is not an object (evaluating 'WebAssembly.Global.prototype')
Comment 20 Yusuke Suzuki 2021-01-27 16:04:49 PST
*** Bug 173180 has been marked as a duplicate of this bug. ***