Summary: | JSTests/exceptionFuzz/earley-boyer.js fails with early exception thrown. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | fpizlo, keith_miller, msaboff, rmorisset, saam, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=212598 | ||||||
Attachments: |
|
Description
Mark Lam
2020-05-30 09:50:43 PDT
I gave a shot investigating this issue. Right now we are hoisting function declarations out of exception handler. It is not clear to me yet if this is the behavior we want to have, but this explains why we are failing with early exception. I created another test case that we can observe the same behavior: ``` try { function foo(){} function bar(){} function baz(){} foo(); } catch(e) { print(e); } ``` This fails early if we run with `--fireExceptionFuzzAt=10` or any number below 10. The bytecode generated for this is: ``` <global>#D6X2C4:[0x10b9bc000->0x10b6d6768, NoneGlobal, 153]: 34 instructions (0 16-bit instructions, 0 32-bit instructions, 14 instructions with metadata); 269 bytes (116 metadata bytes); 1 parameter(s); 18 callee register(s); 6 variable(s); scope at loc4 [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] new_func loc6, loc4, 0 [ 11] resolve_scope loc7, loc4, 0, GlobalProperty, 0 [ 18] mov loc8, loc7 [ 21] put_to_scope loc8, 0, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 29] new_func loc6, loc4, 1 [ 33] put_to_scope loc8, 1, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 41] new_func loc6, loc4, 2 [ 45] put_to_scope loc8, 2, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 53] mov loc6, Undefined(const0) [ 56] mov loc6, Undefined(const0) [ 59] resolve_scope loc8, loc4, 0, GlobalProperty, 0 [ 66] get_from_scope loc7, loc8, 0, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 74] call loc6, loc7, 1, 14 [ 80] jmp 41(->121) [ 82] mov loc4, loc5 [ 85] mov loc8, <JSValue()>(const1) [ 88] mov loc8, loc7 [ 91] mov loc6, Undefined(const0) [ 94] resolve_scope loc12, loc4, 3, GlobalProperty, 0 [ 101] get_from_scope loc9, loc12, 3, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 109] mov loc11, loc8 [ 112] call loc6, loc9, 2, 18 [ 118] mov loc7, Undefined(const0) [ 121] mov loc6, Undefined(const0) [ 124] resolve_scope loc8, loc4, 0, GlobalProperty, 0 [ 131] get_from_scope loc7, loc8, 0, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 139] call loc6, loc7, 1, 14 [ 145] end loc6 [ 147] catch loc8, loc7 [ 151] jmp -69(->82) Identifiers: id0 = foo id1 = bar id2 = baz id3 = print Constants: k0 = Undefined k1 = <JSValue()> Exception Handlers: 1: { start: [ 56] end: [ 82] target: [ 147] } catch ``` Also, FYI this is causing some flaky failures on EWS and also buildbots (e.g https://build.webkit.org/builders/Apple-Catalina-Release-JSC-Tests) I noticed that he following test don't fail: ``` try { (function () { function foo(){} function bar(){} function baz(){} foo(); })(); } catch(e) { print(e); } ``` This avoids that function declarations are hoisted before handler start point. It only fails early with `--fireExceptionFuzzAt=1` because there is an exception check on `op_enter`. I'm not convinced that this is the right solution, but I think it would be good to use it temporarily on `exceptionFuzz` to make tree green while we investigate what's the correct fix. Created attachment 400737 [details]
Patch
Comment on attachment 400737 [details]
Patch
r=me. Please file a new bug for try-catch range not including hoisted function declarations so that we don't just drop it. Please also note that one should undo this workaround in order to reproduce the issue, and that the issue is intermittent. Thanks.
(In reply to Mark Lam from comment #4) > Comment on attachment 400737 [details] > Patch > > r=me. Please file a new bug for try-catch range not including hoisted > function declarations so that we don't just drop it. Please also note that > one should undo this workaround in order to reproduce the issue, and that > the issue is intermittent. Thanks. Thank you very much for the review. I opened https://bugs.webkit.org/show_bug.cgi?id=212598 and related it with this issue. Comment on attachment 400737 [details]
Patch
I was waiting until EWS process it, but it is taking too long due to current flakiness of exceptionFuzz/earley-boyer.js. I'm setting cq+ and watching bots to verify if this cause any regression.
Committed r262383: <https://trac.webkit.org/changeset/262383> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400737 [details]. |