Bug 163846 - [GTK] JSC test wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm and wasm.yaml/wasm/js-api/test_Module.js.default-wasm fail with Exception: ReferenceError: Can't find variable: WebAssembly
Summary: [GTK] JSC test wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm and wasm....
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-22 07:24 PDT by Michael Catanzaro
Modified: 2016-10-26 14:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2016-10-25 18:56 PDT, Yusuke Suzuki
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-10-22 07:24:01 PDT
It looks like this has been broken for a long time. Note that ENABLE_WEBASSEMBLY is still set to OFF for all ports in both WebKitFeatures.cmake and FeatureList.pm:

wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: Exception: ReferenceError: Can't find variable: WebAssembly

30747/37492 (failed 1) ............
                                    
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: module code@/home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/bin/jsc-stress-results/.tests/wasm.yaml/wasm/js-api/test_basic_api.js:26:28

30747/37492 (failed 1) ............
                                    
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: evaluate@[native code]

30747/37492 (failed 1) ............
                                    
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: moduleEvaluation@[native code]

30747/37492 (failed 1) ............
                                    
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: [native code]

30747/37492 (failed 1) ............
                                    
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: promiseReactionJob@[native code]

30747/37492 (failed 1) ............
                                    
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: ERROR: Unexpected exit code: 3
Comment 1 Michael Catanzaro 2016-10-25 16:41:59 PDT
So I am confused as to how this test is passing on Mac, if WebAssembly is currently disabled everywhere.

If this was a layout test, I would just skip the test. Do we have some mechanism for dealing with this in JS tests? If not, I guess it should be temporarily removed?
Comment 2 Michael Catanzaro 2016-10-25 16:43:16 PDT
Oh, one more failure now:

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: Exception: ReferenceError: Can't find variable: WebAssembly

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: EmptyModule@/home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/bin/jsc-stress-results/.tests/wasm.yaml/wasm/js-api/test_Module.js:7:35

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: module code@/home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/bin/jsc-stress-results/.tests/wasm.yaml/wasm/js-api/test_Module.js:9:3

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: evaluate@[native code]

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: moduleEvaluation@[native code]

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: [native code]

30760/37506 ...........
                        
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: promiseReactionJob@[native code]

30760/37506 ...........
30761/37506 .......... 
30761/37506 ..........
                       
wasm.yaml/wasm/js-api/test_basic_api.js.default-wasm: ERROR: Unexpected exit code: 3

30761/37506 ..........
30762/37506 (failed 1) .........
30762/37506 (failed 1) ..........
                                  
wasm.yaml/wasm/js-api/test_Module.js.default-wasm: ERROR: Unexpected exit code: 3
Comment 3 JF Bastien 2016-10-25 16:52:16 PDT
I'm not an expert as how tests work, but I'm guessing the problem came from here:
  https://bugs.webkit.org/attachment.cgi?id=291661

When I added runWebAssembly:
  https://trac.webkit.org/browser/trunk/Tools/Scripts/run-jsc-stress-tests#L1157

Can you confirm that this is the case?
Are the conditions at the top wrong for GTK?

Sorry for asking all these questions, I don't understand how this all holds together.
Comment 4 Michael Catanzaro 2016-10-25 16:59:14 PDT
(In reply to comment #3)
> I'm not an expert as how tests work, but I'm guessing the problem came from
> here:
>   https://bugs.webkit.org/attachment.cgi?id=291661
> 
> When I added runWebAssembly:
>  
> https://trac.webkit.org/browser/trunk/Tools/Scripts/run-jsc-stress-
> tests#L1157
> 
> Can you confirm that this is the case?

Nope, I honestly don't know how to check JS test results that far back. :( I'm sure there's a way....

> Are the conditions at the top wrong for GTK?

They look sane to me.

> Sorry for asking all these questions, I don't understand how this all holds
> together.

Me either! I think we need to figure out how it is that WebAssembly works at all on the Mac port, then decide if it's ready to be turned on for other ports or not. If it's not ready yet, then we should find a way to skip the test conditional on whether or not it's enabled. 

Alternatively, we could modify the test to pass whenever the WebAssembly variable is not defined, and assume that we would notice if such a major feature accidentally disappeared.
Comment 5 JF Bastien 2016-10-25 17:15:38 PDT
> > Are the conditions at the top wrong for GTK?
> 
> They look sane to me.

Hmm, I don't think they are:

$isFTLPlatform = !($architecture == "x86" || $architecture == "arm" || $hostOS == "windows" || $hostOS == "linux" && $architecture == "arm64")

We'd need a way to see if the platform built with ENABLE(WEBASSEMBLY). I'm not sure how this is done.

> Me either! I think we need to figure out how it is that WebAssembly works at
> all on the Mac port, then decide if it's ready to be turned on for other
> ports or not. If it's not ready yet, then we should find a way to skip the
> test conditional on whether or not it's enabled. 

It's not ready yet, and we're only concentrating on Mac bits right now so some of the code will need Linux-isms to work well on Linux. I wouldn't enable it yet, unless someone is willing to do the Linux work.


> Alternatively, we could modify the test to pass whenever the WebAssembly
> variable is not defined, and assume that we would notice if such a major
> feature accidentally disappeared.

I'd rather not since the entire point is to test that WebAssembly works. It's unlikely that we'd mess that up, but still it's not released so nobody would notice.
Comment 6 Yusuke Suzuki 2016-10-25 18:42:57 PDT
(In reply to comment #5)
> > > Are the conditions at the top wrong for GTK?
> > 
> > They look sane to me.
> 
> Hmm, I don't think they are:
> 
> $isFTLPlatform = !($architecture == "x86" || $architecture == "arm" ||
> $hostOS == "windows" || $hostOS == "linux" && $architecture == "arm64")
> 
> We'd need a way to see if the platform built with ENABLE(WEBASSEMBLY). I'm
> not sure how this is done.
> 
> > Me either! I think we need to figure out how it is that WebAssembly works at
> > all on the Mac port, then decide if it's ready to be turned on for other
> > ports or not. If it's not ready yet, then we should find a way to skip the
> > test conditional on whether or not it's enabled. 
> 
> It's not ready yet, and we're only concentrating on Mac bits right now so
> some of the code will need Linux-isms to work well on Linux. I wouldn't
> enable it yet, unless someone is willing to do the Linux work.
> 
> 
> > Alternatively, we could modify the test to pass whenever the WebAssembly
> > variable is not defined, and assume that we would notice if such a major
> > feature accidentally disappeared.
> 
> I'd rather not since the entire point is to test that WebAssembly works.
> It's unlikely that we'd mess that up, but still it's not released so nobody
> would notice.

It seems that ENABLE(WEBASSEMBLY) is ON in Mac ports by default, right?
I think adding the following "darwin" check in runWebAssembly is preferable.

if $hostOS == "darwin"

WEBASSEMBLY flag can be enabled even if it is still under the development, and actually it seems that it is enabled in Mac ports.
Why we can do that safely is there is runtime flag Options::useWebAssembly(). It prevents us from exposing the implementation under the heavy development.

Basically, we can enable WEBASSEMBLY flag.
The reason why we would like to disable this in Linux port is we have a part only focusing on Darwin right now.
So, the reasonable choice is disabling the tests based on the $hostOS.
Comment 7 Yusuke Suzuki 2016-10-25 18:56:37 PDT
Created attachment 292863 [details]
Patch
Comment 8 Yusuke Suzuki 2016-10-25 18:58:44 PDT
BTW, still the problem occurs in GTK port on Darwin.
But I think we have a chance to enable it on Darwin.
Comment 9 JF Bastien 2016-10-25 19:16:39 PDT
Comment on attachment 292863 [details]
Patch

Looks good, though I think we should file a bug to remove this on Linux (and link to it from the comment). Thanks for looking into it, I wasn't sure what the best fix was :-)
Comment 10 Michael Catanzaro 2016-10-26 11:33:57 PDT
Comment on attachment 292863 [details]
Patch

OK it looks sane, thanks.

We don't have any GTK tests bots running Darwin so I don't really care toooo much if it's still broken on that platform, but indeed WebAssembly would not be available there either, so testing the port would make more sense than testing the OS, right?
Comment 11 Yusuke Suzuki 2016-10-26 11:54:03 PDT
(In reply to comment #10)
> Comment on attachment 292863 [details]
> Patch
> 
> OK it looks sane, thanks.
> 
> We don't have any GTK tests bots running Darwin so I don't really care toooo
> much if it's still broken on that platform, but indeed WebAssembly would not
> be available there either, so testing the port would make more sense than
> testing the OS, right?

(In reply to comment #10)
> Comment on attachment 292863 [details]
> Patch
> 
> OK it looks sane, thanks.
> 
> We don't have any GTK tests bots running Darwin so I don't really care toooo
> much if it's still broken on that platform, but indeed WebAssembly would not
> be available there either, so testing the port would make more sense than
> testing the OS, right?

Yeah, I think theoretically we can. But, in reality, I think it constantly causes some build/runtime errors until the implementation becomes ready to be ported.

Anyway, this should be a transient workaround. As JF suggested, I'll open a bug and note this in FIXME.
Comment 12 Yusuke Suzuki 2016-10-26 14:12:35 PDT
Committed r207913: <http://trac.webkit.org/changeset/207913>