javascript jit bug affecting Google Maps

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryan Sturgell]

Created attachment 269762 [details]
Jit bug repro, prints FAILED for incorrect results

A couple weeks ago we pushed a new version of Google Maps, and Safari users started seeing rendering bugs (missing water features and parks) after loading a few viewports. We were able to work around the issue by rolling back a (seemingly innocuous) change.

I've managed to reduce the repro to a simple case, see attached.

The test calls function calc() 20k times. The function should always return 1. If it successfully returns 1 on every call, the tests shows "PASSED". If it ever returns something other than 1, the test prints FAILED and the iteration number, and exits.

In Safari and WebkitNightly Version 9.0.2 (11601.3.9, r195530) it returns 0 after roughly 10k iterations:

FAILED! Got result 0 at iteration 10486

Note that the test passes if the web inspector is open, and it also seems to pass on the very first load of a freshly started browser (but will consistently repro thereafter on a reload or new tab).

[Radar WebKit Bug Importer]

<rdar://problem/24337479>

[Ryan Sturgell]

Were you able to reproduce this problem?

I spent quite a lot of time creating this reduced repro, it would be nice to know that this report hasn't gone into a black hole...

Thanks,
Ryan

[Saam Barati]

(In reply to comment #2)
> Were you able to reproduce this problem?
> 
> I spent quite a lot of time creating this reduced repro, it would be nice to
> know that this report hasn't gone into a black hole...
> 
> Thanks,
> Ryan

Thanks Ryan. I'll look into this when I'm in the office (I'm on vacation today).

[Mark Lam]

Ryan, thanks for the repro case.  I can reproduce the bug.  It appears to only manifest with the FTL JIT.

[Ryan Sturgell]

Any update on this?

The workaround in my code is quite fragile, and I'm not at all confident that we don't have other instances of this problem that I just haven't detected. The style of code in this repro is very common throughout our code.

If you do not plan to fix the bug any time soon, can you give me some insight into what the exact trigger is, so that I may be able to check our code for other cases?

[BJ Burg]

The bug is still active, but there's a lot going on. FTL JIT has a new backend, so it is probably helpful if you could confirm that it reproduces with a recent nightly.

[Ryan Sturgell]

(In reply to comment #6)
> The bug is still active, but there's a lot going on. FTL JIT has a new
> backend, so it is probably helpful if you could confirm that it reproduces
> with a recent nightly.

Yes, the attached test still fails in today's nightly (Version 9.0.3 (11601.4.4, r197659)).

[Mark Lam]

Here's minimum repro case that can be run from the jsc shell:
=== BEGIN ===
function badFunc(tt, operandA, m) { 
  // This variable re-use seems important - rename it and the bug goes away.
  operandA = tt[operandA];

  if (false) {
    // If this (unreachable) block is removed, the bug goes away!!
  } else 
  {
    m[0] = operandA;
  }
}
noInline(badFunc);

function run() {
  for (var i = 0; i < 10000; i++) {
    var tt = new Uint32Array([0x80000000,1]); // Needs to be an Uint32Array.
    var failed = false;

    var m = [];

    badFunc(tt, 0, m);
    badFunc(tt, 1, m);
    var result = m[0];
    if (result != tt[1]) {
      print('FAILED! Got result ' + result + ' for tt[1] at iteration ' + i);
      return;
    }
  }
  print('PASSED'); 
}

run();
=== END ===

Here's how I run the jsc shell on this test:
$ JSC_useConcurrentJIT=0 JSC_maximumInliningDepth=1 $VM/jsc test.js

[Mark Lam]

Here's the generated code for the badFunc function with my annotations:

Generated JIT code for FTL B3 code for badFunc#AhFADt:[0x116ba3400->0x116ba3840->0x116bcd280, FTLFunctionCall, 28 (NeverInline)]:
    Code at [0x378d824037a0, 0x378d82403920):
      0x378d824037a0: push %rbp
      0x378d824037a1: mov %rsp, %rbp

      0x378d824037a4: add $0xffffffffffffffd0, %rsp         // sp = sp - 40
      0x378d824037a8: mov $0x116ba3400, %rax                // $rax = 0x116ba3400 // cb
      0x378d824037b2: mov %rax, 0x10(%rbp)                  // cfr[2 * 8] = 0x116ba3400 i.e. cfr.codeBlock = 0x116ba3400.
      0x378d824037b6: mov 0x40(%rbp), %rdi                  // $rdi = cfr[5 * 8] i.e. $rdi = arg3 i.e. m.

      0x378d824037ba: mov $0xffff000000000002, %rax         // $rax = Not object mask.
      0x378d824037c4: test %rdi, %rax                       // if (!@isObject(m))
      0x378d824037c7: jnz 0x378d8240388a                    //     goto error

      0x378d824037cd: mov 0x38(%rbp), %rcx                  // $rcx = cfr[7 * 8] i.e. arg2 i.e. operandA
      0x378d824037d1: mov 0x30(%rbp), %rdx                  // $rdx = cfr[6 * 8] i.e. arg1 i.e. tt

      0x378d824037d5: test %rdx, %rax                       // if (!@isObject(tt))
      0x378d824037d8: jnz 0x378d82403894                    //     goto error

      0x378d824037de: cmp $0x60, (%rdx)                     // if (tt.structureID != expected(tt))
      0x378d824037e1: jnz 0x378d8240389e                    //     specCheckfail

      0x378d824037e7: cmp $0x7d, (%rdi)                     // if (m.structureID != expected(m))
      0x378d824037ea: jnz 0x378d824038a8                    //     specCheckFail

      0x378d824037f0: mov 0x10(%rdx), %rax                  // $rax = tt.m_vector
      0x378d824037f4: mov 0x18(%rdx), %esi                  // $esi = tt.m_length

      0x378d824037f7: mov $0xffff000000000000, %rdx         // $rdx = int mask / TagTypeNumber
      0x378d82403801: cmp %rdx, %rcx                        // if (!@isInt(operandA)
      0x378d82403804: jb 0x378d824038b2                     //     specCheckFail.

      0x378d8240380a: cmp %esi, %ecx                        // if (int32(tt.m_length) < int32(operandA))
      0x378d8240380c: jae 0x378d824038bc                    //     error

      0x378d82403812: mov %ecx, %ecx                        // operandA = int32(operandA)
      0x378d82403814: mov (%rax,%rcx,4), %esi               // $esi = tt.m_vector[operandA]
      0x378d82403817: movsxd %esi, %rax                     // $rax = signExtend($esi)

      0x378d8240381a: cmp %rax, %rsi                        // if (signExtend($esi) == $rsi) i.e. if (highBitNotSet($esi))
      0x378d8240381d: jz 0x378d82403881                     //     goto 0x378d82403881

      0x378d82403823: cvtsi2sd %rsi, %xmm0                  // $xmm0 = double($rsi)
      0x378d82403828: movq %xmm0, %rax                      // $rax = doubleAsIntBits($xmm0)
      0x378d8240382d: mov $0x1 0000 0000 0000, %rcx            // $rcx = DoubleEncodeOffset
      0x378d82403837: add %rax, %rcx                        // $rcx = boxedDouble($rsi)

      // Got result of tt[operandA].  Now continue
      0x378d8240383a: mov 0x8(%rdi), %rax                   // $rax = m.butterfly     <=================================== Jumps here after the int boxing below.  Bad!!!!

      0x378d8240383e: add %rcx, %rdx                        // $rdx = unbox result in $rcx expected to be a double    <=== This is good for doubles, bad for ints.
      0x378d82403841: movq %rdx, %xmm0                      // Move unboxed bits to $xmm0

      0x378d82403846: cmp $0x0, -0x8(%rax)                  // if (0 <= m.butterfly.indexingHeader.publicLength)
      0x378d8240384a: jbe 0x378d82403860                    //      goto ...

      0x378d82403850: movsd %xmm0, (%rax)                   // m[0] = result bits from xmm0

      0x378d82403854: mov $0xa, %rax                        // $rax = undefined
      0x378d8240385b: mov %rbp, %rsp                        // return.
      0x378d8240385e: pop %rbp
      0x378d8240385f: ret 

      // case: (0 <= m.butterfly.length)
      0x378d82403860: cmp $0x0, -0x4(%rax)                  // if (0 <= m.butterfly.indexingHeader.vectorLength)
      0x378d82403864: jbe 0x378d824038c6                    //     error ???

      0x378d8240386a: mov $0x1, -0x8(%rax)                  // m.butterfly.indexingHeader.publicLength = 1
      0x378d82403871: movsd %xmm0, (%rax)                   // m[0] = result bits from xmm0

      0x378d82403875: mov $0xa, %rax                        // $rax = undefined
      0x378d8240387c: mov %rbp, %rsp                        // return.
      0x378d8240387f: pop %rbp
      0x378d82403880: ret 

      // case: highBitNotSet($esi) ...
      0x378d82403881: lea (%rsi,%rdx), %rcx                 // $rcx = $rsi + int mask i.e. boxedInt($rsi)            <===== Boxing the Int here and jumps above for badness.
      0x378d82403885: jmp 0x378d8240383a                    // Jumps 

      0x378d8240388a: push $0x0
      0x378d8240388f: jmp 0x378d824039a0


      0x378d82403894: push $0x1
      0x378d82403899: jmp 0x378d824039a0
      0x378d8240389e: push $0x2
      0x378d824038a3: jmp 0x378d824039a0
      0x378d824038a8: push $0x3
      0x378d824038ad: jmp 0x378d824039a0
      0x378d824038b2: push $0x4
      0x378d824038b7: jmp 0x378d824039a0
      0x378d824038bc: push $0x5
      0x378d824038c1: jmp 0x378d824039a0

      0x378d824038c6: push $0x6
      0x378d824038cb: jmp 0x378d824039a0
      0x378d824038d0: mov $0x1133fe2e0, %r9
      0x378d824038da: mov %rbx, (%r9)
      0x378d824038dd: mov %r12, 0x8(%r9)
      0x378d824038e1: mov %r13, 0x10(%r9)
      0x378d824038e5: mov %r14, 0x18(%r9)
      0x378d824038e9: mov %r15, 0x20(%r9)
      0x378d824038ed: mov $0x1133f8000, %rdi
      0x378d824038f7: mov %rbp, %rsi
      0x378d824038fa: mov $0x1103d90e0, %r11
      0x378d82403904: call *%r11
      0x378d82403907: mov $0x1133fe358, %rsi
      0x378d82403911: mov (%rsi), %rsi
      0x378d82403914: jmp *%rsi

[Mark Lam]

So, result array "m" is of DoubleShape.  When the result to be written is an Int52 which gets converted to a double, all is well.  When the result is an Int32, the generated FTL code mistakenly assumes the result is a double, and does a double style unboxing on it.  The resultant "unboxed" value is an impure NaN (0xfffe000000000001).

I'm not sure why that gets read as a hole or undefined yet when we print the result later.

[Geoffrey Garen]

> I'm not sure why that gets read as a hole or undefined yet when we print the
> result later.

Double arrays use NaN to represent hole (and hole converts to undefined). That's likely why impure NaN sometimes becomes hole or undefined.

But that's sort of a red herring -- by the time we have an impure NaN, all bets are off for any future behavior.

[Mark Lam]

(In reply to comment #11)
> > I'm not sure why that gets read as a hole or undefined yet when we print the
> > result later.
> 
> Double arrays use NaN to represent hole (and hole converts to undefined).
> That's likely why impure NaN sometimes becomes hole or undefined.

I know we store pure NaN to represent holes in arrays with double indexing types.  I mistakenly thought that we did that to allow impureNaNs to represent NaN values in the array.  Having thought about it more, that didn't make sense.  And I also confirmed that the isHole() check (in ArrayPrototype.cpp) checks for any NaNs, not only pure NaNs.  That explains why the wrong value gets misinterpreted as a hole and produced an undefined result.

Onwards with finding out what went wrong in the FTL that resulted in the bad "unboxing".

[Mark Lam]

DFG graph just before FTL lowering (with my annotations) says:

                // @59: Int52(@25:tt @78 [ operandA @79 ])
      59:< 1:->	ValueRep(Int52Rep:Kill:@25<Int52>, JS|PureInt, Boolint32Nonboolint32Int52asdouble, bc#6, exit: bc#12)
      91:<!0:->	KillStack(MustGen, loc6, W:SideState, ClobbersExit, bc#12, ExitInvalid)
      32:<!0:->	ZombieHint(MustGen, loc6, W:SideState, ClobbersExit, bc#12, ExitInvalid)

                // @58: butterfly for @80:m
      58:< 1:->	GetButterfly(Cell:@80, Storage|PureInt, R:JSObject_butterfly, Exits, bc#21)

                // @60: Double(@59:Int52(@25:tt @78 [ operandA @79 ]))
      60:< 1:->	DoubleRep(Number:Kill:@59, Double|PureInt, Bytecodedouble, bc#21)

                // @80:m [ @30:0 ] = @60:Double(@59:Int52(@25:tt @78 [ operandA @79 ]))
      39:<!0:->	PutByVal(KnownCell:Kill:@80, Int32:Kill:@30, DoubleRepReal:Kill:@60<Double>, Untyped:Kill:@58, MustGen|VarArgs, DoubleOriginalArrayToHoleAsIs, R:Butterfly_publicLength,Butterfly_vectorLength,IndexedDoubleProperties, W:Butterfly_publicLength,IndexedDoubleProperties, Exits, ClobbersExit, bc#21)

i.e. the DFG graph says that whatever value we got from node @59 will be converted to a double at node @60, and put in the results array "m" at node @39.

The initial lowered B3 (with my annotations) says:

        Int64 @65 = ZExt32(@62, DFG:@25<Int52>)                                 // @65 = zeroExtended int32(operandA)
        Int64 @66 = Const64(DFG:@25<Int52>, 2)                                  // @66 = 2
        Int32 @67 = Trunc($2(@66), DFG:@25<Int52>)                              // @67 = 2
        Int64 @68 = Shl(@65, @67, DFG:@25<Int52>)                               // @68 = zeroExtended int32(operandA) * 4
        Int64 @69 = Add(@56, @68, DFG:@25<Int52>)                               // @69 = &tt[operandA]

        Int32 @70 = Load(@69, DFG:@25<Int52>, ControlDependent|Reads:0...1)     // @70 = tt[operandA]

        Int64 @71 = ZExt32(@70, DFG:@25<Int52>)                                 // @71 = zeroExtended(tt[operandA])
        Int32 @72 = Trunc(@71, DFG:@59)                                         // @72 = int32(@71)
        Int64 @73 = SExt32(@72, DFG:@59)                                        // @73 = signedExtended(tt[operandA])
        Int32 @74 = Equal(@73, @71, DFG:@59)                                    // @74 = (tt[operandA] is Int32)          <========= checks if result is Int32
        Void @75 = Branch(@74, DFG:@59, #5, #6, Terminal)                       // @75: branch #5 if Int32 else #6.

        // is Int32
    BB#5: ; frequency = 1.000000
        Int64 @76 = ZExt32(@72, DFG:@59)                                        // @76 = zeroExtended(tt[operandA])
        Int64 @77 = Add(@76, $-281474976710656(@13), DFG:@59)                   // @77 = boxedInt(tt[operandA])
        Void @78 = Upsilon(@77, DFG:@59, ^85, WritesLocalState)
        Void @79 = Jump(DFG:@59, #7, Terminal)

        // not Int32
    BB#6: ; frequency = 1.000000
        Double @80 = IToD(@71, DFG:@59)                                         // @80 = double(tt[operandA])
        Int64 @81 = BitwiseCast(@80, DFG:@59)                                   // @81 = double(tt[operandA]).bitsAsInt64()
        Int64 @82 = Sub(@81, $-281474976710656(@13), DFG:@59)                   // @82 = boxedDouble(tt[operandA])
        Void @83 = Upsilon(@82, DFG:@59, ^85, WritesLocalState)
        Void @84 = Jump(DFG:@59, #7, Terminal)

        // Got result of tt[operandA].  Now continue
    BB#7: ; frequency = 1.000000
        Int64 @85 = Phi(DFG:@59, ReadsLocalState)
        Int64 @86 = Const64(DFG:@58, 8)                                         // @86 = 8
        Int64 @87 = Add(@29, $8(@86), DFG:@58)                                  // @87 = &m[8] = &m.butterfly
        Int64 @88 = Load(@87, DFG:@58, ControlDependent|Reads:23...24)          // @88 = m.butterfly
        Void @89 = Branch($1(@1), DFG:@60<Double>, #9, #8, Terminal)            // @89: if (1) goto #9 else #8.

        ...
    BB#10: ; frequency = 1.000000
        Int64 @95 = Add(@85, $-281474976710656(@13), DFG:@60<Double>)           // @95 = result + NumberTag <================== trying to unbox a double. Bad !!!!!!!
        Double @96 = BitwiseCast(@95, DFG:@60<Double>)                          // @96 = bitwise value of "unboxed" double result.
        Void @97 = Upsilon(@96, DFG:@60<Double>, ^100, WritesLocalState)
        Void @98 = Jump(DFG:@60<Double>, #12, Terminal)

Hence, the bug lies in the lowering from DFG to B3.  Diving in there next.

[Filip Pizlo]

I think I know what is going on!

Can you show the graph before DFG node @59?

[Mark Lam]

The complete DFG graph before lowering is as follows:

    Block #0 (bc#0): (OSR target)
      Execution count: 1.000000
      Predecessors:
      Successors:
      Dominated by: #0
      Dominates: #0
      Dominance Frontier: 
      Iterated Dominance Frontier: 
      Pre/Post Numbering: 0/0
      States: StructuresAreWatched
      Availability: {locals = arg3:arg3:FlushedCell/Unavailable arg2:arg2:FlushedJSValue/Unavailable arg1:arg1:FlushedCell/Unavailable arg0:this:FlushedJSValue/Unavailable; heap = }
      Live: 
      Values: 
       4:< 1:->	JSConstant(JS|PureInt, Other, Undefined, bc#0)
      30:< 1:->	JSConstant(JS|UseAsOther, Boolint32, Int32: 0, bc#0)
      81:<!0:->	ExitOK(MustGen, W:SideState, bc#0)

                // @78: tt
      78:< 4:->	GetStack(JS|PureInt, Uint32array, arg1, machine:arg1, FlushedCell, R:Stack(6), bc#0)

      62:<!0:->	CheckStructure(Cell:@78, MustGen, [%Ak:Uint32Array], R:JSCell_structureID, Exits, bc#0)

                // @79: operandA
      79:< 2:->	GetStack(JS|PureInt, Boolint32, arg2, machine:arg2, FlushedJSValue, R:Stack(7), bc#0)
                // @80: m
      80:< 3:->	GetStack(JS|PureInt, Array, arg3, machine:arg3, FlushedCell, R:Stack(8), bc#0)
                // Ensure m is a DoubleIndexing array.
      64:<!0:->	CheckStructure(Cell:@80, MustGen, [%AR:Array], R:JSCell_structureID, Exits, bc#0)

      82:<!0:->	KillStack(MustGen, loc0, W:SideState, ClobbersExit, bc#0)
       5:<!0:->	ZombieHint(MustGen, loc0, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      83:<!0:->	KillStack(MustGen, loc1, W:SideState, ClobbersExit, bc#0, ExitInvalid)
       7:<!0:->	ZombieHint(MustGen, loc1, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      84:<!0:->	KillStack(MustGen, loc2, W:SideState, ClobbersExit, bc#0, ExitInvalid)
       9:<!0:->	ZombieHint(MustGen, loc2, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      85:<!0:->	KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      11:<!0:->	ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      86:<!0:->	KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      13:<!0:->	ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      87:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      15:<!0:->	ZombieHint(MustGen, loc5, W:SideState, ClobbersExit, bc#0, ExitInvalid)
      88:<!0:->	KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#1)
      19:<!0:->	ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#1, ExitInvalid)
      89:<!0:->	KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#3)
      21:<!0:->	ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#3, ExitInvalid)

                // @56: storage for tt @78
      56:< 1:->	GetIndexedPropertyStorage(KnownCell:@78, Storage|PureInt, Uint32ArrayOriginalNonArrayInBoundsAsIs, R:MiscFields, Exits, bc#6)
  
      92:< 1:->	GetArrayLength(KnownCell:@78, Int32|PureInt, Int32, Uint32ArrayOriginalNonArrayInBoundsAsIs, R:MiscFields, Exits, bc#6)
      93:<!0:->	CheckInBounds(Check:Int32:@79, KnownInt32:Kill:@92, MustGen, Int32, Exits, bc#6)

                // @25: tt @78 [ operandA @79 ]
      25:<!2:->	GetByVal(KnownCell:Kill:@78, Int32:Kill:@79, Untyped:Kill:@56, Int52|MustGen|UseAsOther, Boolint32Nonboolint32Int52, Uint32ArrayOriginalNonArrayInBoundsAsIs, R:TypedArrayProperties,MiscFields, Exits, bc#6)  predicting Int52asdouble

      90:<!0:->	KillStack(MustGen, arg2, W:SideState, ClobbersExit, bc#6, ExitInvalid)
      26:<!0:->	MovHint(Int52Rep:@25<Int52>, MustGen, arg2, W:SideState, ClobbersExit, bc#6, ExitInvalid)

                // @59: Int52(@25:tt @78 [ operandA @79 ])
      59:< 1:->	ValueRep(Int52Rep:Kill:@25<Int52>, JS|PureInt, Boolint32Nonboolint32Int52asdouble, bc#6, exit: bc#12)
      91:<!0:->	KillStack(MustGen, loc6, W:SideState, ClobbersExit, bc#12, ExitInvalid)
      32:<!0:->	ZombieHint(MustGen, loc6, W:SideState, ClobbersExit, bc#12, ExitInvalid)

                // @58: butterfly for @80:m
      58:< 1:->	GetButterfly(Cell:@80, Storage|PureInt, R:JSObject_butterfly, Exits, bc#21)

                // @60: Double(@59:Int52(@25:tt @78 [ operandA @79 ]))
      60:< 1:->	DoubleRep(Number:Kill:@59, Double|PureInt, Bytecodedouble, bc#21)

                // @80:m [ @30:0 ] = @60:Double(@59:Int52(@25:tt @78 [ operandA @79 ]))
      39:<!0:->	PutByVal(KnownCell:Kill:@80, Int32:Kill:@30, DoubleRepReal:Kill:@60<Double>, Untyped:Kill:@58, MustGen|VarArgs, DoubleOriginalArrayToHoleAsIs, R:Butterfly_publicLength,Butterfly_vectorLength,IndexedDoubleProperties, W:Butterfly_publicLength,IndexedDoubleProperties, Exits, ClobbersExit, bc#21)

                // return undefined.
      42:<!0:->	Return(Untyped:Kill:@4, MustGen, W:SideState, Exits, bc#26)

[Mark Lam]

Created attachment 277115 [details]
proposed patch.

[Mark Lam]

Landed in r199935: <http://trac.webkit.org/r199935>.