Bug 186540 – ShadowChicken crashes with stack overflow in the LLInt

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

[Tadeu Zagallo]

The following crashes on ShadowChicken when running with the JIT disabled and debugging opcodes enabled:

```
function foo() { foo() }
foo();
```

[Tadeu Zagallo]

<rdar://problem/39682133>

[Tadeu Zagallo]

Created attachment 342465 [details]
Patch

[Saam Barati]

Comment on attachment 342465 [details]
Patch

Please add a test for this that runs with LLInt only and shadow chicken enabled

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tadeu Zagallo]

Created attachment 342480 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tadeu Zagallo]

Created attachment 342565 [details]
Patch

Fix CLoop

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

Comment on attachment 342565 [details]
Patch

LGTM. R=me

[EWS]

Comment on attachment 342565 [details]
Patch

Rejecting attachment 342565 [details] from commit-queue.

[e-mail] 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.

[Tadeu Zagallo]

Created attachment 342805 [details]
Patch

[Tadeu Zagallo]

Created attachment 342806 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tadeu Zagallo]

Created attachment 342976 [details]
Patch

Fix crash when overflowing in the first JS frame

[Saam Barati]

Comment on attachment 342976 [details]
Patch

r=me still

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tadeu Zagallo]

Created attachment 343093 [details]
Patch

try a different approach

[Saam Barati]

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?

[Saam Barati]

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.

[Saam Barati]

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.

[Tadeu Zagallo]

Created attachment 343096 [details]
Patch

[Saam Barati]

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

[Tadeu Zagallo]

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.

[WebKit Commit Bot]

Comment on attachment 343096 [details]
Patch

Clearing flags on attachment: 343096

Committed r232983: <https://trac.webkit.org/changeset/232983>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.