JIT bug - fails when inspector closed, works when open

[Jordan Harband]

When accessing http://rawgit.com/es-shims/es6-shim/master/test/index.html on a current Nightly revision, it will fail with a bunch of `obj.__flags` errors (something in mocha/chai). However, when the dev inspector is *open*, they all/mostly pass as expected (some failures in older revisions are fine).

This appears to have been introduced by r182007, r182008, or r182009 - http://trac.webkit.org/changeset/182009 looks related perhaps?

Obviously this is super critical, and since I have no idea how it's broken I have no idea how many things this could break across the web :-)

[Jordan Harband]

(thanks to yusuke for confirming it's a JIT bug, and yusuke and twitter.com/rosskukulinski for helping me narrow down the problem revisions)

[Yusuke Suzuki]

hm, at r182007, it already fails.
So there's issue before r182007.

The issue looks like that.
In chai.js, we call

function Assertion(...) {
    flag(this, '...', ...);
    flag(this, '...', ...);
    flag(this, '...', ...);
}

function flag(obj, ...) {
    accessing obj.__flags become error (obj is undefined)
}

function expect(...) {
    return new Assertion(...);
}

I guess
1. accidentally accessing incorrect location of arguments (`obj`).
2. accidentally `this` becomes undefined.

[Jordan Harband]

Hm, then it must be between r181893 and r182007 ?

[Yusuke Suzuki]

r181993 seems the cause of this failure.
In r181993, I've made sure that this issue occurs.
http://trac.webkit.org/changeset/181993

[Radar WebKit Bug Importer]

<rdar://problem/21061293>

[Michael Saboff]

The failure is manifest in the following code:

function Assertion (obj, msg, stack) {
    flag(this, 'ssfi', stack  || arguments.callee);  // Eliminate arguments.callee and the bug goes away
    flag(this, 'object', obj);   // The first argument, "this", is undefined in "flag()"
    flag(this, 'message', msg);
}

function flag(obj, key, value) {
    var flags = obj.__flags || (obj.__flags = Object.create(null));
    if (arguments.length === 3) {
        flags[key] = value;
    } else {
        return flags[key];
    }
}

The bytecode for Assertion is:
Assertion#BsmUKj:[0x10b7dc720->0x10a04ab00, NoneFunctionConstruct, 140]: 140 m_instructions; 1120 bytes; 4 parameter(s); 14 callee register(s); 3 variable(s)
[   0] enter
[   1] get_scope         loc0
[   3] create_direct_arguments loc1
[   5] create_this       this, this, 0, 0
[  10] mov               loc2, loc1
[  13] resolve_scope     loc8, loc0, flag(@id0), 1<ThrowIfNotFound|GlobalVar>, 0
[  20] get_from_scope    loc3, loc8, flag(@id0), 1<ThrowIfNotFound|GlobalVar>, 201294216    predicting None
[  28] mov               loc7, this
[  31] mov               loc6, String (atomic) (identifier): ssfi, ID: 4(const0)
[  34] get_from_arguments loc5, loc1, 2    predicting None
[  39] jtrue             loc5, 12(->51)
[  42] get_by_id         loc5, loc2, callee(@id1)    predicting None
[  51] call              loc3, loc3, 4, 14 status(Could Take Slow Path, maxNumArguments = 1)    Original; predicting None
[  60] resolve_scope     loc8, loc0, flag(@id0), 1<ThrowIfNotFound|GlobalVar>, 0
[  67] get_from_scope    loc3, loc8, flag(@id0), 1<ThrowIfNotFound|GlobalVar>, 201294216    predicting None
[  75] mov               loc7, this
[  78] mov               loc6, String (atomic) (identifier): object, ID: 4(const1)
[  81] get_from_arguments loc5, loc1, 0    predicting None
[  86] call              loc3, loc3, 4, 14 status(Could Take Slow Path, maxNumArguments = 1)    Original; predicting None
[  95] resolve_scope     loc8, loc0, flag(@id0), 1<ThrowIfNotFound|GlobalVar>, 0
[ 102] get_from_scope    loc3, loc8, flag(@id0), 1<ThrowIfNotFound|GlobalVar>, 201294216    predicting None
[ 110] mov               loc7, this
[ 113] mov               loc6, String (atomic) (identifier): message, ID: 4(const2)
[ 116] get_from_arguments loc5, loc1, 1    predicting None
[ 121] call              loc3, loc3, 4, 14 status(Could Take Slow Path, maxNumArguments = 1)    Original; predicting None
[ 130] is_object         loc3, this
[ 133] jtrue             loc3, 5(->138)
[ 136] ret               this
[ 138] ret               this

The bytecode for flag is:
flag#D14VaO:[0x10b7dc980->0x10a04aa00, NoneFunctionCall, 132]: 132 m_instructions; 1056 bytes; 4 parameter(s); 14 callee register(s); 4 variable(s)
[   0] enter
[   1] get_scope         loc0
[   3] create_direct_arguments loc1
[   5] mov               loc3, loc1
[   8] get_from_arguments loc5, loc1, 0    predicting None
[  13] get_by_id         loc4, loc5, __flags(@id0)    predicting None
[  22] jtrue             loc4, 56(->78)
[  25] get_from_arguments loc5, loc1, 0    predicting None
[  30] resolve_scope     loc8, loc0, Object(@id1), 0<ThrowIfNotFound|GlobalProperty>, 0
[  37] get_from_scope    loc8, loc8, Object(@id1), 0<ThrowIfNotFound|GlobalProperty>, 101    predicting None
[  45] get_by_id         loc6, loc8, create(@id2)    predicting None
[  54] mov               loc7, Null(const0)
[  57] call              loc6, loc6, 2, 14 status(Could Take Slow Path, maxNumArguments = 1)    Original; predicting None
[  66] put_by_id         loc5, __flags(@id0), loc6
[  75] mov               loc4, loc6
[  78] mov               loc2, loc4
[  81] get_by_id         loc4, loc3, length(@id3)    predicting None
[  90] stricteq          loc4, loc4, Int32: 3(const1)
[  94] jfalse            loc4, 23(->117)
[  97] mov               loc4, loc2
[ 100] get_from_arguments loc5, loc1, 1    predicting None
[ 105] get_from_arguments loc6, loc1, 2    predicting None
[ 110] put_by_val        loc4, loc5, loc6    Original
[ 115] jmp               15(->130)
[ 117] get_from_arguments loc4, loc1, 1    predicting None
[ 122] get_by_val        loc4, loc2, loc4    Original; predicting None
[ 128] ret               loc4
[ 130] ret               Undefined(const2)

All three instances of "flag()" are inlined.  The problem is during the second call to flag(), we get "undefined" for the base object when executing bc#13, get_by_id.  The reason is during local CSE, we replace object we use as the base.  Before local CSE:
   189:< 1:->   CreateDirectArguments(JS|UseAsOther, Directarguments, R:Stack,HeapObjectCount, W:HeapObjectCount, bc#3)
...
   194:< 1:->   GetFromArguments(Check:KnownCell:@189, JS|UseAsOther, Final, capturedArgument0, R:DirectArgumentsProperties(0), bc#8)  predicting Final
...
   197:<!0:->   GetById(Check:Cell:@194, JS|MustGen|UseAsOther, Final, id2{__flags}, R:World, W:Heap, bc#13)  predicting Final

After local CSE:
 171:<!0:->     GetLocal(@457, JS|MustGen|UseAsOther, Directarguments, loc1(I<DirectArguments>/FlushedCell), R:Stack(-2), bc#81)  predicting Directarguments
 172:< 1:->     GetFromArguments(Check:KnownCell:@171, JS|UseAsOther, Other, capturedArgument0, R:DirectArgumentsProperties(0), bc#81)  predicting Other
   197:<!0:->   GetById(Check:Cell:@172, JS|MustGen|UseAsOther, Final, id2{__flags}, R:World, W:Heap, bc#13)  predicting Final

Where 172 is the GetFromArguments in Assertion.

The problem is in DFGClobberize::cloberize() and its handling of GetFromArguments.

[Michael Saboff]

Created attachment 254893 [details]
Patch

[Michael Saboff]

Committed r185566: <http://trac.webkit.org/changeset/185566>

[Filip Pizlo]

Comment on attachment 254893 [details]
Patch

r=me too

[Jordan Harband]

Thanks, this looks great in the latest nightly build!