Bug 212569 – JSTests/exceptionFuzz/earley-boyer.js fails with early exception thrown.

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

[Mark Lam]

Here's how I run the test:

$ VM=WebKitBuild/Release && DYLD_FRAMEWORK_PATH=$VM lldb $VM/jsc -- --dumpGeneratedBytecodes=1 --useBaselineJIT=0 --fireExceptionFuzzAt=782 --useDollarVM\=true --useExceptionFuzz\=true JSTests/exceptionFuzz/earley-boyer.js

We get an uncaught exception (which should not happen):

JSC EXCEPTION FUZZ: Throwing fuzz exception with call frame 0x1079fab68, seen in LLIntSlowPaths and return address 0x1078abc71.
Exception: Error: Exception Fuzz
global code@JSTests/exceptionFuzz/earley-boyer.js:46:20
JSC EXCEPTION FUZZ: encountered 782 checks.

This uncaught exception should not happen because the entire source of JSTests/exceptionFuzz/earley-boyer.js is wrapped in a try catch statement.

From the bytecode dump,

Exception Handlers:
	 1: { start: [4031] end: [109677] target: [109745] } catch

The bytecode around 4031 are:

[3953] *new_func          loc6, loc4, 292
[3961] *put_to_scope      loc8, 292, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0
[3977] *new_func          loc6, loc4, 293
[3985] *put_to_scope      loc8, 293, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0
[4001] *new_func          loc6, loc4, 294
[4009] *put_to_scope      loc8, 294, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0
[4025] mov                loc6, Undefined(const0)
[4028] mov                loc6, Undefined(const0)

[4031] *resolve_scope     loc7, loc4, 295, GlobalProperty, 0

[4045] *put_to_scope      loc7, 295, this, 1048576<DoNotThrowIfNotFound|GlobalProperty|Initialization|NotStrictMode>, 0, 0
[4061] *resolve_scope     loc7, loc4, 296, GlobalProperty, 0
[4075] *put_to_scope      loc7, 296, Int32: -1(const1), 1048576<DoNotThrowIfNotFound|GlobalProperty|Initialization|NotStrictMode>, 0, 0

Note all the new_func opcodes that came before 4031.  There's definitely instructions that can throw generated before beginning of the try block.  Need to investigate further which bytecode --fireExceptionFuzzAt=782 corresponds to, but it looks like there maybe a bug here.

[Caio Lima]

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)

[Caio Lima]

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.

[Caio Lima]

Created attachment 400737 [details]
Patch

[Mark Lam]

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.

[Caio Lima]

(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.

[Caio Lima]

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.

[EWS]

Committed r262383: <https://trac.webkit.org/changeset/262383>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400737 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63834898>