Bug 165429 – We should be able to throw exceptions from Wasm code and when Wasm frames are on the stack

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 296254 [details]
WIP

Most things are in place now, I just need to teach StackVisitor about wasm.

[Saam Barati]

Created attachment 296323 [details]
WIP

Just threw a wasm exception:


TypeError: Out of bounds memory access
------------------------
<wasm>@[wasm code]
<wasm>@[wasm code]
testWasmModuleFunctions@[native code]
module code@/Volumes/Data/WK/a/OpenSource/JSTests/wasm/function-tests/i32-load.js:27:32
evaluate@[native code]
moduleEvaluation@[native code]
[native code]
promiseReactionJob@[native code]
------------------------

[Saam Barati]

Created attachment 296760 [details]
WIP

might be done. I need to review the code and write a changelog.

[Saam Barati]

Created attachment 296765 [details]
patch

[Saam Barati]

Created attachment 296767 [details]
patch

rebased

[WebKit Commit Bot]

Attachment 296767 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 296768 [details]
patch

fix merge conflict compile error.

[WebKit Commit Bot]

Attachment 296768 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

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

lgtm, but I'm not super familiar with this code so it would be good to have another look.

> Source/JavaScriptCore/API/JSContextRef.cpp:276
> +            if (visitor->hasLineAndColumnInfo()) {

Should we have a fix to collect wasm function number and index?

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:162
> +#if ENABLE(WEBASSEMBLY)

Do you need these #if here and elsewhere? You're not using anything which isn't available if not wasm.

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:329
> +        // FIXME:

This seems fine, why FIXME?

> Source/JavaScriptCore/runtime/JSCell.cpp:301
> +    return inherits(JSWebAssemblyCallee::info()) || inherits(WebAssemblyToJSCallee::info());

I added a function which does this in Repatch.cpp, we should share it.

[Saam Barati]

Looks like WebCore uses StackVisitor.

[Keith Miller]

Comment on attachment 296768 [details]
patch

I think this seems reasonable assuming you fix the WebCore build issues. Although, I don't fully understand why we would export StackVisitor to WebCore. It also, looks like you need to fix the Wasm-less builds too.

[Saam Barati]

Created attachment 296788 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296788 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 296791 [details]
patch for landing

[Saam Barati]

Created attachment 296792 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296792 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 296794 [details]
patch for landing

[Saam Barati]

Created attachment 296795 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296795 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 296796 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296796 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 296798 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296798 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 296798 [details]
patch for landing

Attachment 296798 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2687180

New failing tests:
inspector/codemirror/prettyprinting-css-rules.html

[Build Bot]

Created attachment 296799 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296798 [details]
patch for landing

Attachment 296798 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2687207

New failing tests:
imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html
imported/w3c/web-platform-tests/streams/readable-streams/general.https.html
js/dom/Promise-static-all.html
imported/w3c/web-platform-tests/streams/byte-length-queuing-strategy.https.html
streams/readable-stream-pipeThrough.html
imported/w3c/web-platform-tests/streams/count-queuing-strategy.https.html
imported/w3c/web-platform-tests/streams/readable-streams/pipe-through.https.html
js/dom/Promise-static-race.html

[Build Bot]

Created attachment 296800 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

Created attachment 296822 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296822 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 296822 [details]
patch for landing

Attachment 296822 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2692102

New failing tests:
inspector/codemirror/prettyprinting-javascript.html

[Build Bot]

Created attachment 296834 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

(In reply to comment #31)
> Created attachment 296834 [details]
> Archive of layout-test-results from ews103 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

Hmmm, the crashes here don't look related to my patch. I'm going to rebase the patch and re-upload for EWS.

[Saam Barati]

Created attachment 296874 [details]
patch for landing

[WebKit Commit Bot]

Attachment 296874 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 296874 [details]
patch for landing

Clearing flags on attachment: 296874

Committed r209696: <http://trac.webkit.org/changeset/209696>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #35)
> Comment on attachment 296874 [details]
> patch for landing
> 
> Clearing flags on attachment: 296874
> 
> Committed r209696: <http://trac.webkit.org/changeset/209696>

It broke the Linux builds, see build.webkit.org for details.

[Saam Barati]

(In reply to comment #37)
> (In reply to comment #35)
> > Comment on attachment 296874 [details]
> > patch for landing
> > 
> > Clearing flags on attachment: 296874
> > 
> > Committed r209696: <http://trac.webkit.org/changeset/209696>
> 
> It broke the Linux builds, see build.webkit.org for details.

About to land a speculative fix

[Saam Barati]

Tried to fix the build in:
https://trac.webkit.org/changeset/209702

[Csaba Osztrogonác]

cloop build is still broken:
https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/22207

[Saam Barati]

(In reply to comment #40)
> cloop build is still broken:
> https://build.webkit.org/builders/
> Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/22207

Fix in:
https://trac.webkit.org/changeset/209722