Bug 186540

Summary: ShadowChicken crashes with stack overflow in the LLInt
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-feeder, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, rniwa, saam
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews112 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews116 for mac-sierra
none
Archive of layout-test-results from ews200 for win-future
none
Patch
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews202 for win-future
none
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Patch none

Description Tadeu Zagallo 2018-06-11 14:30:46 PDT
The following crashes on ShadowChicken when running with the JIT disabled and debugging opcodes enabled:

```
function foo() { foo() }
foo();
```
Comment 1 Tadeu Zagallo 2018-06-11 14:31:40 PDT
<rdar://problem/39682133>
Comment 2 Tadeu Zagallo 2018-06-11 14:34:56 PDT
Created attachment 342465 [details]
Patch
Comment 3 Saam Barati 2018-06-11 15:35:30 PDT
Comment on attachment 342465 [details]
Patch

Please add a test for this that runs with LLInt only and shadow chicken enabled
Comment 4 EWS Watchlist 2018-06-11 16:28:33 PDT
Comment on attachment 342465 [details]
Patch

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

New failing tests:
http/tests/misc/large-js-program.php
Comment 5 EWS Watchlist 2018-06-11 16:28:34 PDT
Created attachment 342478 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 Tadeu Zagallo 2018-06-11 16:42:54 PDT
Created attachment 342480 [details]
Patch
Comment 7 EWS Watchlist 2018-06-11 18:25:56 PDT
Comment on attachment 342480 [details]
Patch

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

New failing tests:
http/tests/misc/large-js-program.php
Comment 8 EWS Watchlist 2018-06-11 18:25:57 PDT
Created attachment 342496 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 EWS Watchlist 2018-06-11 20:19:00 PDT
Comment on attachment 342480 [details]
Patch

Attachment 342480 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8141064

New failing tests:
js/regress-139548.html
fast/dom/console-log-stack-overflow.html
js/dom/line-column-numbers.html
js/regress-141098.html
js/dom/stack-trace.html
js/stack-overflow-catch.html
js/kde/crash-2.html
fast/workers/use-machine-stack.html
js/dom/deep-recursion-test.html
js/stack-overflow-arrity-catch.html
fast/dom/error-to-string-stack-overflow.html
js/dom/global-recursion-on-full-stack.html
js/function-apply-aliased.html
Comment 10 EWS Watchlist 2018-06-11 20:19:11 PDT
Created attachment 342507 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 Tadeu Zagallo 2018-06-12 11:25:34 PDT
Created attachment 342565 [details]
Patch

Fix CLoop
Comment 12 EWS Watchlist 2018-06-12 14:38:34 PDT
Comment on attachment 342565 [details]
Patch

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

New failing tests:
js/dom/JSON-stringify.html
http/tests/misc/large-js-program.php
Comment 13 EWS Watchlist 2018-06-12 14:38:35 PDT
Created attachment 342595 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 EWS Watchlist 2018-06-12 22:54:16 PDT
Comment on attachment 342565 [details]
Patch

Attachment 342565 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8156939

New failing tests:
http/tests/preload/onload_event.html
Comment 15 EWS Watchlist 2018-06-12 22:54:28 PDT
Created attachment 342634 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 16 Saam Barati 2018-06-14 11:06:08 PDT
Comment on attachment 342565 [details]
Patch

LGTM. R=me
Comment 17 EWS 2018-06-15 05:25:31 PDT
Comment on attachment 342565 [details]
Patch

Rejecting attachment 342565 [details] from commit-queue.

tzagallo@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 18 Tadeu Zagallo 2018-06-15 06:23:37 PDT
Created attachment 342805 [details]
Patch
Comment 19 Tadeu Zagallo 2018-06-15 06:32:59 PDT
Created attachment 342806 [details]
Patch
Comment 20 EWS Watchlist 2018-06-15 08:09:12 PDT
Comment on attachment 342806 [details]
Patch

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

New failing tests:
http/tests/misc/large-js-program.php
Comment 21 EWS Watchlist 2018-06-15 08:09:13 PDT
Created attachment 342813 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 22 Tadeu Zagallo 2018-06-18 15:15:51 PDT
Created attachment 342976 [details]
Patch

Fix crash when overflowing in the first JS frame
Comment 23 Saam Barati 2018-06-18 16:08:50 PDT
Comment on attachment 342976 [details]
Patch

r=me still
Comment 24 EWS Watchlist 2018-06-18 16:22:15 PDT
Comment on attachment 342976 [details]
Patch

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

New failing tests:
js/reentrant-caching.html
Comment 25 EWS Watchlist 2018-06-18 16:22:16 PDT
Created attachment 342990 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 26 EWS Watchlist 2018-06-18 16:46:01 PDT
Comment on attachment 342976 [details]
Patch

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

New failing tests:
jsc-layout-tests.yaml/js/script-tests/reentrant-caching.js.layout-ftl-eager-no-cjit
wasm.yaml/wasm/js-api/promise-stack-overflow.js.wasm-no-tls-context
jsc-layout-tests.yaml/js/script-tests/regress-139548.js.layout-ftl-no-cjit
wasm.yaml/wasm/js-api/promise-stack-overflow.js.wasm-no-cjit-yes-tls-context
stress/regress-179355.js.ftl-no-cjit-small-pool
wasm.yaml/wasm/js-api/promise-stack-overflow.js.wasm-eager-jettison
wasm.yaml/wasm/js-api/promise-stack-overflow.js.wasm-no-call-ic
jsc-layout-tests.yaml/js/script-tests/regress-139548.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/regress-139548.js.layout
jsc-layout-tests.yaml/js/script-tests/regress-139548.js.layout-no-cjit
stress/regress-179355.js.ftl-no-cjit-no-inline-validate
wasm.yaml/wasm/js-api/promise-stack-overflow.js.wasm-slow-memory
wasm.yaml/wasm/js-api/promise-stack-overflow.js.default-wasm
wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-call-ic
jsc-layout-tests.yaml/js/script-tests/reentrant-caching.js.layout-dfg-eager-no-cjit
apiTests
Comment 27 EWS Watchlist 2018-06-18 16:46:22 PDT
Comment on attachment 342976 [details]
Patch

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

New failing tests:
js/regress-139548.html
Comment 28 EWS Watchlist 2018-06-18 16:46:23 PDT
Created attachment 342992 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 29 EWS Watchlist 2018-06-18 17:04:20 PDT
Comment on attachment 342976 [details]
Patch

Attachment 342976 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8237948

New failing tests:
js/dom/string-replace-exception-crash.html
js/regress-139548.html
Comment 30 EWS Watchlist 2018-06-18 17:04:22 PDT
Created attachment 342993 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 31 EWS Watchlist 2018-06-18 17:30:56 PDT
Comment on attachment 342976 [details]
Patch

Attachment 342976 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8238384

New failing tests:
js/regress-139548.html
js/regress-141098.html
Comment 32 EWS Watchlist 2018-06-18 17:31:08 PDT
Created attachment 342995 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 33 Tadeu Zagallo 2018-06-19 13:49:11 PDT
Created attachment 343093 [details]
Patch

try a different approach
Comment 34 Saam Barati 2018-06-19 14:02:19 PDT
Comment on attachment 343093 [details]
Patch

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

> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:303
> +            JSValue scopeValue = callFrame->bytecodeOffset() && codeBlock && codeBlock->scopeRegister().isValid()

This feels very precarious. Are we just assuming that the second byte code will always be getScope?

What was wrong with the previous approach?
Comment 35 Saam Barati 2018-06-19 14:02:56 PDT
Comment on attachment 343093 [details]
Patch

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

> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:306
> +            if (scopeValue.isUndefined() && codeBlock->wasCompiledWithDebuggingOpcodes() && !scopeValue.isUndefined()) {

this code is wrong. It can't both be undefined and not undefined.
Comment 36 Saam Barati 2018-06-19 14:03:44 PDT
Comment on attachment 343093 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        initialize it to undefined).

I see. This may be an OK thing to rely on. But I wonder if we still have bugs w.r.t how we handled stack overflow in the LLInt.
Comment 37 Tadeu Zagallo 2018-06-19 14:06:32 PDT
Created attachment 343096 [details]
Patch
Comment 38 Saam Barati 2018-06-19 14:09:12 PDT
Comment on attachment 343096 [details]
Patch

r=me

What was wrong with the previous approach? I wonder if we still have subtle bugs in the LLInt's stack overflow code
Comment 39 Tadeu Zagallo 2018-06-19 14:11:38 PDT
I talked with Phil today, and it seems that it might be better to always handle the stack overflows from the callee instead of from the caller, given that the caller frame may not always be what we need. I will add a follow up bug to update the JIT too.
Comment 40 WebKit Commit Bot 2018-06-19 14:27:12 PDT
Comment on attachment 343096 [details]
Patch

Clearing flags on attachment: 343096

Committed r232983: <https://trac.webkit.org/changeset/232983>
Comment 41 WebKit Commit Bot 2018-06-19 14:27:14 PDT
All reviewed patches have been landed.  Closing bug.