Bug 117602 – Going to google.com/trends causes a crash

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[jacoco]

Going to google.com/trends force closes the browser

[Alexey Proskuryakov]

<rdar://problem/13910521>

[Oliver Hunt]

function g() { f() }
 function f() {  doStuff(); arguments = undefined;  }
 function doStuff() { throw {} }

 for (var i = 0; i < 100; i++) { try { g() } catch (e) {} }

[Oliver Hunt]

Okay, so if we have:
function g() { f() }
function f() {  doStuff(); arguments;  }
function doStuff() { throw {} }

for (var i = 0; i < 100; i++) { try { g() } catch (e) {} }



The initial graph is:
DFG for g#BBXOF8:[0x7fe6ad00f000->0x10935fd70, DFGFunctionCall]:
  Fixpoint state: BeforeFixpoint; Form: LoadStore; Unification state: LocallyUnified; Ref count state: EverythingIsLive
  ArgumentPosition size: 3
    #0: 
    #1: 
    #2: 
Block #0 (bc#0):  (OSR target)
  Predecessors:
  Phi Nodes:
  vars before: <empty>
  var links: arg0:- : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
   0:           < 1:->	SetArgument(arg0(a), bc#0)  predicting None
   1:           < 1:->	JSConstant(JS|PureInt, $0 = Undefined, bc#1)
   2:           <!0:->	GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
   3:           < 1:->	WeakJSConstant(JS|PureInt, 0x1092decb0, bc#1)
   4:           < 1:->	SetLocal(@1, CanExit|NodeExitsForward, r1(B~), bc#1)  predicting None
   5:           < 1:->	SetLocal(@3, CanExit|NodeExitsForward, r0(C~), bc#1)  predicting None
   6:           < 1:->	SetLocal(@1, CanExit|NodeExitsForward, r1(D~), bc#1)  predicting None
   7:           < 1:->	SetLocal(@3, CanExit|NodeExitsForward, r0(E~), bc#1)  predicting None
   8:           <!0:->	Phantom(@3, @1, MustGen|CanExit, bc#7)
  --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
     9:           <!0:->	InlineStart(MustGen, bc#0)
    10:           < 1:->	SetLocal(@1, CanExit|NodeExitsForward, r8(F*), bc#0)  predicting None
    11:           < 1:->	SetLocal(@1, CanExit|NodeExitsForward, r9(G*), bc#0)  predicting None
    12:           < 1:->	JSConstant(JS|PureInt, $1 = <JSValue()>, bc#1)
    13:           <!0:->	Flush(MustGen, r9(G*), bc#1)  predicting None
    14:           < 1:->	SetLocal(@12, CanExit|NodeExitsForward, r9(H*), bc#1)  predicting None
    15:           <!0:->	Flush(MustGen, r8(F*), bc#3)  predicting None
    16:           < 1:->	SetLocal(@12, CanExit|NodeExitsForward, r8(I*), bc#3)  predicting None
    17:           <!0:->	GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
    18:           < 1:->	WeakJSConstant(JS|PureInt, 0x1092dec70, bc#5)
    19:           < 1:->	SetLocal(@1, CanExit|NodeExitsForward, r11(J~), bc#5)  predicting None
    20:           < 1:->	SetLocal(@18, CanExit|NodeExitsForward, r10(K~), bc#5)  predicting None
    21:           < 1:->	SetLocal(@1, CanExit|NodeExitsForward, r11(L~), bc#5)  predicting None
    22:           < 1:->	SetLocal(@18, CanExit|NodeExitsForward, r10(M~), bc#5)  predicting None
    23:           <!0:->	Phantom(@18, @1, MustGen|CanExit, bc#11)
    --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
      24:           <!0:->	InlineStart(MustGen, bc#0)
      25:           < 1:->	NewObject(JS|PureInt|CanExit, struct(0x105ddb6c8: NonArray), bc#1)
      26:           < 1:->	SetLocal(@25, CanExit|NodeExitsForward, r18(N~), bc#1)  predicting None
      27:           <!0:->	Throw(@25, MustGen|CanExit, bc#5)


Which eventually becomes:
  var links: arg0:@0 : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
   0:           < 1:->	SetArgument(arg0(a), bc#0)  predicting Other
   1:           < 1:->	JSConstant(JS|UseAsOther, $0 = Undefined, bc#1)
   2:           <!0:->	GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
   3:           < 1:->	WeakJSConstant(JS|UseAsOther, 0x1092decb0, bc#1)
   4:           < 1:->	SetLocal(@1<Other>, CanExit|NodeExitsForward, r1(B~<Other>), bc#1)  predicting Other
   5:           < 1:->	SetLocal(@3<Function>, CanExit|NodeExitsForward, r0(C~<Function>), bc#1)  predicting Function
   6:           < 1:->	SetLocal(@1<Other>, CanExit|NodeExitsForward, r1(D~<Other>), bc#1)  predicting Other
   7:           < 1:->	SetLocal(@3<Function>, CanExit|NodeExitsForward, r0(E~<Function>), bc#1)  predicting Function
   8:           <!0:->	Phantom(@3<Function>, @1<Other>, MustGen|CanExit, bc#7)
  --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
     9:           <!0:->	InlineStart(MustGen, bc#0)
    10:           < 1:->	SetLocal(@1<Other>, CanExit|NodeExitsForward, r8(F*<Other>), bc#0)  predicting Other
    11:           < 1:->	SetLocal(@1<Other>, CanExit|NodeExitsForward, r9(G*<Other>), bc#0)  predicting Other
    12:           < 1:->	JSConstant(JS|PureInt, $1 = <JSValue()>, bc#1)
    13:           <!0:->	Flush(@11, MustGen, r9(G*<Other>), bc#1)  predicting Other
    14:           < 1:->	SetLocal(@12, CanExit|NodeExitsForward, r9(H*), bc#1)  predicting Empty
    15:           <!0:->	Flush(@10, MustGen, r8(F*<Other>), bc#3)  predicting Other
    16:           < 1:->	SetLocal(@12, CanExit|NodeExitsForward, r8(I*), bc#3)  predicting Empty
    17:           <!0:->	GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
    18:           < 1:->	WeakJSConstant(JS|UseAsOther, 0x1092dec70, bc#5)
    19:           < 1:->	SetLocal(@1<Other>, CanExit|NodeExitsForward, r11(J~<Other>), bc#5)  predicting Other
    20:           < 1:->	SetLocal(@18<Function>, CanExit|NodeExitsForward, r10(K~<Function>), bc#5)  predicting Function
    21:           < 1:->	SetLocal(@1<Other>, CanExit|NodeExitsForward, r11(L~<Other>), bc#5)  predicting Other
    22:           < 1:->	SetLocal(@18<Function>, CanExit|NodeExitsForward, r10(M~<Function>), bc#5)  predicting Function
    23:           <!0:->	Phantom(@18<Function>, @1<Other>, MustGen|CanExit, bc#11)
    --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
      24:           <!0:->	InlineStart(MustGen, bc#0)
      25:           < 1:->	NewObject(JS|UseAsOther|CanExit, struct(0x105ddb6c8: NonArray), bc#1)
      26:           < 1:->	SetLocal(@25<Final>, CanExit|NodeExitsForward, r18(N~<Final>), bc#1)  predicting Final
      27:           <!0:->	Throw(@25<Final>, MustGen|CanExit, bc#5)


And then after DCE
  var links: arg0:@0 : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
   0:  skipped  < 0:->	SetArgument(arg0(a), bc#0)
   1:           < 4:->	JSConstant(JS|UseAsOther, $0 = Undefined, bc#1)
   2:           <!0:->	GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
   3:           < 1:->	WeakJSConstant(JS|UseAsOther, 0x1092decb0, bc#1)
   4:  skipped  < 0:->	MovHint(@1<Other>, r1(B~<Other>), bc#1)
   5:  skipped  < 0:->	MovHint(@3<Function>, r0(C~<Function>), bc#1)
   6:  skipped  < 0:->	MovHint(@1<Other>, r1(D~<Other>), bc#1)
   7:  skipped  < 0:->	MovHint(@3<Function>, r0(E~<Function>), bc#1)
   8:           <!0:->	Phantom(@3<Function>, @1<Other>, MustGen, bc#7)
  --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
     9:           <!0:->	InlineStart(MustGen, bc#0)
    10:  skipped  < 0:->	MovHint(@1<Other>, r8(F*<Other>), bc#0)
    11:  skipped  < 0:->	MovHint(@1<Other>, r9(G*<Other>), bc#0)
    12:           <!0:->	Phantom(MustGen|CanExit, bc#1)
    13:           <!0:->	Phantom(@1<Other>, MustGen, bc#1)
    14:  skipped  < 0:->	ZombieHint(r9(H*), bc#1)
    15:           <!0:->	Phantom(@1<Other>, MustGen, bc#3)
    16:  skipped  < 0:->	ZombieHint(r8(I*), bc#3)
    17:           <!0:->	GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
    18:           < 1:->	WeakJSConstant(JS|UseAsOther, 0x1092dec70, bc#5)
    19:  skipped  < 0:->	MovHint(@1<Other>, r11(J~<Other>), bc#5)
    20:  skipped  < 0:->	MovHint(@18<Function>, r10(K~<Function>), bc#5)
    21:  skipped  < 0:->	MovHint(@1<Other>, r11(L~<Other>), bc#5)
    22:  skipped  < 0:->	MovHint(@18<Function>, r10(M~<Function>), bc#5)
    23:           <!0:->	Phantom(@18<Function>, @1<Other>, MustGen, bc#11)
    --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
      24:           <!0:->	InlineStart(MustGen, bc#0)
      25:           < 1:->	NewObject(JS|UseAsOther, struct(0x105ddb6c8: NonArray), bc#1)
      26:  skipped  < 0:->	MovHint(@25<Final>, r18(N~<Final>), bc#1)
      27:           <!0:->	Throw(@25<Final>, MustGen|CanExit, bc#5)


Nodes @14 and @16 were responsible for initializing the lazy argument slots, but they have been elided, despite being necessary for correct behavior.

I can add 
            addToGraph(Phantom, get(currentInstruction[1].u.operand));

To init_lazy_reg, and that makes this crash go away, but it feels jacky, and looking at the output graph it is not obvious _why_ we end up with the correct behavior.

[Filip Pizlo]

(In reply to comment #3)
> Okay, so if we have:
> function g() { f() }
> function f() {  doStuff(); arguments;  }
> function doStuff() { throw {} }
> 
> for (var i = 0; i < 100; i++) { try { g() } catch (e) {} }
> 
> 
> 
> The initial graph is:
> DFG for g#BBXOF8:[0x7fe6ad00f000->0x10935fd70, DFGFunctionCall]:
>   Fixpoint state: BeforeFixpoint; Form: LoadStore; Unification state: LocallyUnified; Ref count state: EverythingIsLive
>   ArgumentPosition size: 3
>     #0: 
>     #1: 
>     #2: 
> Block #0 (bc#0):  (OSR target)
>   Predecessors:
>   Phi Nodes:
>   vars before: <empty>
>   var links: arg0:- : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
>    0:           < 1:->    SetArgument(arg0(a), bc#0)  predicting None
>    1:           < 1:->    JSConstant(JS|PureInt, $0 = Undefined, bc#1)
>    2:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
>    3:           < 1:->    WeakJSConstant(JS|PureInt, 0x1092decb0, bc#1)
>    4:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r1(B~), bc#1)  predicting None
>    5:           < 1:->    SetLocal(@3, CanExit|NodeExitsForward, r0(C~), bc#1)  predicting None
>    6:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r1(D~), bc#1)  predicting None
>    7:           < 1:->    SetLocal(@3, CanExit|NodeExitsForward, r0(E~), bc#1)  predicting None
>    8:           <!0:->    Phantom(@3, @1, MustGen|CanExit, bc#7)
>   --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
>      9:           <!0:->    InlineStart(MustGen, bc#0)
>     10:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r8(F*), bc#0)  predicting None
>     11:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r9(G*), bc#0)  predicting None
>     12:           < 1:->    JSConstant(JS|PureInt, $1 = <JSValue()>, bc#1)
>     13:           <!0:->    Flush(MustGen, r9(G*), bc#1)  predicting None
>     14:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r9(H*), bc#1)  predicting None
>     15:           <!0:->    Flush(MustGen, r8(F*), bc#3)  predicting None
>     16:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r8(I*), bc#3)  predicting None

Are 14 and 16 flushed?

>     17:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
>     18:           < 1:->    WeakJSConstant(JS|PureInt, 0x1092dec70, bc#5)
>     19:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r11(J~), bc#5)  predicting None
>     20:           < 1:->    SetLocal(@18, CanExit|NodeExitsForward, r10(K~), bc#5)  predicting None
>     21:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r11(L~), bc#5)  predicting None
>     22:           < 1:->    SetLocal(@18, CanExit|NodeExitsForward, r10(M~), bc#5)  predicting None
>     23:           <!0:->    Phantom(@18, @1, MustGen|CanExit, bc#11)
>     --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
>       24:           <!0:->    InlineStart(MustGen, bc#0)
>       25:           < 1:->    NewObject(JS|PureInt|CanExit, struct(0x105ddb6c8: NonArray), bc#1)
>       26:           < 1:->    SetLocal(@25, CanExit|NodeExitsForward, r18(N~), bc#1)  predicting None
>       27:           <!0:->    Throw(@25, MustGen|CanExit, bc#5)
> 
> 
> Which eventually becomes:
>   var links: arg0:@0 : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
>    0:           < 1:->    SetArgument(arg0(a), bc#0)  predicting Other
>    1:           < 1:->    JSConstant(JS|UseAsOther, $0 = Undefined, bc#1)
>    2:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
>    3:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092decb0, bc#1)
>    4:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r1(B~<Other>), bc#1)  predicting Other
>    5:           < 1:->    SetLocal(@3<Function>, CanExit|NodeExitsForward, r0(C~<Function>), bc#1)  predicting Function
>    6:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r1(D~<Other>), bc#1)  predicting Other
>    7:           < 1:->    SetLocal(@3<Function>, CanExit|NodeExitsForward, r0(E~<Function>), bc#1)  predicting Function
>    8:           <!0:->    Phantom(@3<Function>, @1<Other>, MustGen|CanExit, bc#7)
>   --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
>      9:           <!0:->    InlineStart(MustGen, bc#0)
>     10:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r8(F*<Other>), bc#0)  predicting Other
>     11:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r9(G*<Other>), bc#0)  predicting Other
>     12:           < 1:->    JSConstant(JS|PureInt, $1 = <JSValue()>, bc#1)
>     13:           <!0:->    Flush(@11, MustGen, r9(G*<Other>), bc#1)  predicting Other
>     14:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r9(H*), bc#1)  predicting Empty
>     15:           <!0:->    Flush(@10, MustGen, r8(F*<Other>), bc#3)  predicting Other
>     16:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r8(I*), bc#3)  predicting Empty
>     17:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
>     18:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092dec70, bc#5)
>     19:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r11(J~<Other>), bc#5)  predicting Other
>     20:           < 1:->    SetLocal(@18<Function>, CanExit|NodeExitsForward, r10(K~<Function>), bc#5)  predicting Function
>     21:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r11(L~<Other>), bc#5)  predicting Other
>     22:           < 1:->    SetLocal(@18<Function>, CanExit|NodeExitsForward, r10(M~<Function>), bc#5)  predicting Function
>     23:           <!0:->    Phantom(@18<Function>, @1<Other>, MustGen|CanExit, bc#11)
>     --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
>       24:           <!0:->    InlineStart(MustGen, bc#0)
>       25:           < 1:->    NewObject(JS|UseAsOther|CanExit, struct(0x105ddb6c8: NonArray), bc#1)
>       26:           < 1:->    SetLocal(@25<Final>, CanExit|NodeExitsForward, r18(N~<Final>), bc#1)  predicting Final
>       27:           <!0:->    Throw(@25<Final>, MustGen|CanExit, bc#5)
> 
> 
> And then after DCE
>   var links: arg0:@0 : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
>    0:  skipped  < 0:->    SetArgument(arg0(a), bc#0)
>    1:           < 4:->    JSConstant(JS|UseAsOther, $0 = Undefined, bc#1)
>    2:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
>    3:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092decb0, bc#1)
>    4:  skipped  < 0:->    MovHint(@1<Other>, r1(B~<Other>), bc#1)
>    5:  skipped  < 0:->    MovHint(@3<Function>, r0(C~<Function>), bc#1)
>    6:  skipped  < 0:->    MovHint(@1<Other>, r1(D~<Other>), bc#1)
>    7:  skipped  < 0:->    MovHint(@3<Function>, r0(E~<Function>), bc#1)
>    8:           <!0:->    Phantom(@3<Function>, @1<Other>, MustGen, bc#7)
>   --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
>      9:           <!0:->    InlineStart(MustGen, bc#0)
>     10:  skipped  < 0:->    MovHint(@1<Other>, r8(F*<Other>), bc#0)
>     11:  skipped  < 0:->    MovHint(@1<Other>, r9(G*<Other>), bc#0)
>     12:           <!0:->    Phantom(MustGen|CanExit, bc#1)
>     13:           <!0:->    Phantom(@1<Other>, MustGen, bc#1)
>     14:  skipped  < 0:->    ZombieHint(r9(H*), bc#1)
>     15:           <!0:->    Phantom(@1<Other>, MustGen, bc#3)
>     16:  skipped  < 0:->    ZombieHint(r8(I*), bc#3)
>     17:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
>     18:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092dec70, bc#5)
>     19:  skipped  < 0:->    MovHint(@1<Other>, r11(J~<Other>), bc#5)
>     20:  skipped  < 0:->    MovHint(@18<Function>, r10(K~<Function>), bc#5)
>     21:  skipped  < 0:->    MovHint(@1<Other>, r11(L~<Other>), bc#5)
>     22:  skipped  < 0:->    MovHint(@18<Function>, r10(M~<Function>), bc#5)
>     23:           <!0:->    Phantom(@18<Function>, @1<Other>, MustGen, bc#11)
>     --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
>       24:           <!0:->    InlineStart(MustGen, bc#0)
>       25:           < 1:->    NewObject(JS|UseAsOther, struct(0x105ddb6c8: NonArray), bc#1)
>       26:  skipped  < 0:->    MovHint(@25<Final>, r18(N~<Final>), bc#1)
>       27:           <!0:->    Throw(@25<Final>, MustGen|CanExit, bc#5)
> 
> 
> Nodes @14 and @16 were responsible for initializing the lazy argument slots, but they have been elided, despite being necessary for correct behavior.
> 
> I can add 
>             addToGraph(Phantom, get(currentInstruction[1].u.operand));
> 
> To init_lazy_reg, and that makes this crash go away, but it feels jacky, and looking at the output graph it is not obvious _why_ we end up with the correct behavior.

No that is definitely not the right solution. Why aren't those two nodes flushed?  Does the inkined functional ways end in Throw?  If so maybe it's that op_throw (and ThrowReferenceError) aren't doing the Flushing that op_ret and op_end do?

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #3)
> > Okay, so if we have:
> > function g() { f() }
> > function f() {  doStuff(); arguments;  }
> > function doStuff() { throw {} }
> > 
> > for (var i = 0; i < 100; i++) { try { g() } catch (e) {} }
> > 
> > 
> > 
> > The initial graph is:
> > DFG for g#BBXOF8:[0x7fe6ad00f000->0x10935fd70, DFGFunctionCall]:
> >   Fixpoint state: BeforeFixpoint; Form: LoadStore; Unification state: LocallyUnified; Ref count state: EverythingIsLive
> >   ArgumentPosition size: 3
> >     #0: 
> >     #1: 
> >     #2: 
> > Block #0 (bc#0):  (OSR target)
> >   Predecessors:
> >   Phi Nodes:
> >   vars before: <empty>
> >   var links: arg0:- : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
> >    0:           < 1:->    SetArgument(arg0(a), bc#0)  predicting None
> >    1:           < 1:->    JSConstant(JS|PureInt, $0 = Undefined, bc#1)
> >    2:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
> >    3:           < 1:->    WeakJSConstant(JS|PureInt, 0x1092decb0, bc#1)
> >    4:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r1(B~), bc#1)  predicting None
> >    5:           < 1:->    SetLocal(@3, CanExit|NodeExitsForward, r0(C~), bc#1)  predicting None
> >    6:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r1(D~), bc#1)  predicting None
> >    7:           < 1:->    SetLocal(@3, CanExit|NodeExitsForward, r0(E~), bc#1)  predicting None
> >    8:           <!0:->    Phantom(@3, @1, MustGen|CanExit, bc#7)
> >   --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
> >      9:           <!0:->    InlineStart(MustGen, bc#0)
> >     10:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r8(F*), bc#0)  predicting None
> >     11:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r9(G*), bc#0)  predicting None
> >     12:           < 1:->    JSConstant(JS|PureInt, $1 = <JSValue()>, bc#1)
> >     13:           <!0:->    Flush(MustGen, r9(G*), bc#1)  predicting None
> >     14:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r9(H*), bc#1)  predicting None
> >     15:           <!0:->    Flush(MustGen, r8(F*), bc#3)  predicting None
> >     16:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r8(I*), bc#3)  predicting None
> 
> Are 14 and 16 flushed?
> 
> >     17:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
> >     18:           < 1:->    WeakJSConstant(JS|PureInt, 0x1092dec70, bc#5)
> >     19:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r11(J~), bc#5)  predicting None
> >     20:           < 1:->    SetLocal(@18, CanExit|NodeExitsForward, r10(K~), bc#5)  predicting None
> >     21:           < 1:->    SetLocal(@1, CanExit|NodeExitsForward, r11(L~), bc#5)  predicting None
> >     22:           < 1:->    SetLocal(@18, CanExit|NodeExitsForward, r10(M~), bc#5)  predicting None
> >     23:           <!0:->    Phantom(@18, @1, MustGen|CanExit, bc#11)
> >     --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
> >       24:           <!0:->    InlineStart(MustGen, bc#0)
> >       25:           < 1:->    NewObject(JS|PureInt|CanExit, struct(0x105ddb6c8: NonArray), bc#1)
> >       26:           < 1:->    SetLocal(@25, CanExit|NodeExitsForward, r18(N~), bc#1)  predicting None
> >       27:           <!0:->    Throw(@25, MustGen|CanExit, bc#5)
> > 
> > 
> > Which eventually becomes:
> >   var links: arg0:@0 : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
> >    0:           < 1:->    SetArgument(arg0(a), bc#0)  predicting Other
> >    1:           < 1:->    JSConstant(JS|UseAsOther, $0 = Undefined, bc#1)
> >    2:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
> >    3:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092decb0, bc#1)
> >    4:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r1(B~<Other>), bc#1)  predicting Other
> >    5:           < 1:->    SetLocal(@3<Function>, CanExit|NodeExitsForward, r0(C~<Function>), bc#1)  predicting Function
> >    6:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r1(D~<Other>), bc#1)  predicting Other
> >    7:           < 1:->    SetLocal(@3<Function>, CanExit|NodeExitsForward, r0(E~<Function>), bc#1)  predicting Function
> >    8:           <!0:->    Phantom(@3<Function>, @1<Other>, MustGen|CanExit, bc#7)
> >   --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
> >      9:           <!0:->    InlineStart(MustGen, bc#0)
> >     10:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r8(F*<Other>), bc#0)  predicting Other
> >     11:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r9(G*<Other>), bc#0)  predicting Other
> >     12:           < 1:->    JSConstant(JS|PureInt, $1 = <JSValue()>, bc#1)
> >     13:           <!0:->    Flush(@11, MustGen, r9(G*<Other>), bc#1)  predicting Other
> >     14:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r9(H*), bc#1)  predicting Empty
> >     15:           <!0:->    Flush(@10, MustGen, r8(F*<Other>), bc#3)  predicting Other
> >     16:           < 1:->    SetLocal(@12, CanExit|NodeExitsForward, r8(I*), bc#3)  predicting Empty
> >     17:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
> >     18:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092dec70, bc#5)
> >     19:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r11(J~<Other>), bc#5)  predicting Other
> >     20:           < 1:->    SetLocal(@18<Function>, CanExit|NodeExitsForward, r10(K~<Function>), bc#5)  predicting Function
> >     21:           < 1:->    SetLocal(@1<Other>, CanExit|NodeExitsForward, r11(L~<Other>), bc#5)  predicting Other
> >     22:           < 1:->    SetLocal(@18<Function>, CanExit|NodeExitsForward, r10(M~<Function>), bc#5)  predicting Function
> >     23:           <!0:->    Phantom(@18<Function>, @1<Other>, MustGen|CanExit, bc#11)
> >     --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
> >       24:           <!0:->    InlineStart(MustGen, bc#0)
> >       25:           < 1:->    NewObject(JS|UseAsOther|CanExit, struct(0x105ddb6c8: NonArray), bc#1)
> >       26:           < 1:->    SetLocal(@25<Final>, CanExit|NodeExitsForward, r18(N~<Final>), bc#1)  predicting Final
> >       27:           <!0:->    Throw(@25<Final>, MustGen|CanExit, bc#5)
> > 
> > 
> > And then after DCE
> >   var links: arg0:@0 : r0:- r1:- r2:- r3:- r4:- r5:- r6:- r7:- r8:- r9:- r10:- r11:- r12:- r13:- r14:- r15:- r16:- r17:- r18:-
> >    0:  skipped  < 0:->    SetArgument(arg0(a), bc#0)
> >    1:           < 4:->    JSConstant(JS|UseAsOther, $0 = Undefined, bc#1)
> >    2:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global4(0x105dff9c8), bc#1)
> >    3:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092decb0, bc#1)
> >    4:  skipped  < 0:->    MovHint(@1<Other>, r1(B~<Other>), bc#1)
> >    5:  skipped  < 0:->    MovHint(@3<Function>, r0(C~<Function>), bc#1)
> >    6:  skipped  < 0:->    MovHint(@1<Other>, r1(D~<Other>), bc#1)
> >    7:  skipped  < 0:->    MovHint(@3<Function>, r0(E~<Function>), bc#1)
> >    8:           <!0:->    Phantom(@3<Function>, @1<Other>, MustGen, bc#7)
> >   --> f#AmUsAH:<0x10935fc70, bc#7, Call, known callee: Cell: 0x1092decb0 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r8>
> >      9:           <!0:->    InlineStart(MustGen, bc#0)
> >     10:  skipped  < 0:->    MovHint(@1<Other>, r8(F*<Other>), bc#0)
> >     11:  skipped  < 0:->    MovHint(@1<Other>, r9(G*<Other>), bc#0)
> >     12:           <!0:->    Phantom(MustGen|CanExit, bc#1)
> >     13:           <!0:->    Phantom(@1<Other>, MustGen, bc#1)
> >     14:  skipped  < 0:->    ZombieHint(r9(H*), bc#1)
> >     15:           <!0:->    Phantom(@1<Other>, MustGen, bc#3)
> >     16:  skipped  < 0:->    ZombieHint(r8(I*), bc#3)
> >     17:           <!0:->    GlobalVarWatchpoint(MustGen|CanExit, global5(0x105dff9d0), bc#5)
> >     18:           < 1:->    WeakJSConstant(JS|UseAsOther, 0x1092dec70, bc#5)
> >     19:  skipped  < 0:->    MovHint(@1<Other>, r11(J~<Other>), bc#5)
> >     20:  skipped  < 0:->    MovHint(@18<Function>, r10(K~<Function>), bc#5)
> >     21:  skipped  < 0:->    MovHint(@1<Other>, r11(L~<Other>), bc#5)
> >     22:  skipped  < 0:->    MovHint(@18<Function>, r10(M~<Function>), bc#5)
> >     23:           <!0:->    Phantom(@18<Function>, @1<Other>, MustGen, bc#11)
> >     --> doStuff#ASE1VK:<0x10935fb70, bc#11, Call, known callee: Cell: 0x1092dec70 (0x105ddf420: Function, NonArray), numArgs+this = 1, stack >= r18>
> >       24:           <!0:->    InlineStart(MustGen, bc#0)
> >       25:           < 1:->    NewObject(JS|UseAsOther, struct(0x105ddb6c8: NonArray), bc#1)
> >       26:  skipped  < 0:->    MovHint(@25<Final>, r18(N~<Final>), bc#1)
> >       27:           <!0:->    Throw(@25<Final>, MustGen|CanExit, bc#5)
> > 
> > 
> > Nodes @14 and @16 were responsible for initializing the lazy argument slots, but they have been elided, despite being necessary for correct behavior.
> > 
> > I can add 
> >             addToGraph(Phantom, get(currentInstruction[1].u.operand));
> > 
> > To init_lazy_reg, and that makes this crash go away, but it feels jacky, and looking at the output graph it is not obvious _why_ we end up with the correct behavior.
> 
> No that is definitely not the right solution. Why aren't those two nodes flushed?  Does the inkined functional ways end in Throw?  If so maybe it's that op_throw (and ThrowReferenceError) aren't doing the Flushing that op_ret and op_end do?

Just reread the original bug code. Yeah, the bug is that op_throw doesn't do the flushing that op_ret does. This can be fixed by either copying or abstracting the code in ByteCodeParser for flushing in return, so that throwing also does it.

[Oliver Hunt]

> Just reread the original bug code. Yeah, the bug is that op_throw doesn't do the flushing that op_ret does. This can be fixed by either copying or abstracting the code in ByteCodeParser for flushing in return, so that throwing also does it.

Yeah, i was coming to the conclusion as well (there's another bug i cc'd you on which I think is just another symptom of this)

randomly should op_throw be flagged as clobbers the world?

[Oliver Hunt]

(In reply to comment #6)
> > Just reread the original bug code. Yeah, the bug is that op_throw doesn't do the flushing that op_ret does. This can be fixed by either copying or abstracting the code in ByteCodeParser for flushing in return, so that throwing also does it.
> 
> Yeah, i was coming to the conclusion as well (there's another bug i cc'd you on which I think is just another symptom of this)
> 
> randomly should op_throw be flagged as clobbers the world?

Although my reading of the code implies that throw, etc think that they _are_ flushing everything

[Filip Pizlo]

(In reply to comment #6)
> > Just reread the original bug code. Yeah, the bug is that op_throw doesn't do the flushing that op_ret does. This can be fixed by either copying or abstracting the code in ByteCodeParser for flushing in return, so that throwing also does it.
> 
> Yeah, i was coming to the conclusion as well (there's another bug i cc'd you on which I think is just another symptom of this)
> 
> randomly should op_throw be flagged as clobbers the world?

No, throwing doesn't clobber, for the same reason that return doesn't: it's a terminal.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > > Just reread the original bug code. Yeah, the bug is that op_throw doesn't do the flushing that op_ret does. This can be fixed by either copying or abstracting the code in ByteCodeParser for flushing in return, so that throwing also does it.
> > 
> > Yeah, i was coming to the conclusion as well (there's another bug i cc'd you on which I think is just another symptom of this)
> > 
> > randomly should op_throw be flagged as clobbers the world?
> 
> Although my reading of the code implies that throw, etc think that they _are_ flushing everything

Lololololo. 

I see the bug and its awesome. 

Throw is doing the right thing, if throw behaved like return. Return just "pops" the function you're returning from. But throw pops all functions we've inkined: it is a hard terminal. So throw must flush eveything but it only flushes things from inline stack top. That's why the bug manifests if you throw from something that is inlined into an arguments-using function; presumably throwing directly won't break it. 

Just make throw flush things for all inline stack entries and not just the top one.

[Oliver Hunt]

*** Bug 117749 has been marked as a duplicate of this bug. ***

[Oliver Hunt]

Okay I believe i have this fixed

[Oliver Hunt]

Created attachment 204956 [details]
Patch

[Geoffrey Garen]

Comment on attachment 204956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204956&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:452
> +        for (InlineStackEntry* stack = m_inlineStackTop; stack; stack = stack->m_caller) {

I'd call this variable "entry", not "stack", since it's an entry in the stack, and not the whole stack.

[Oliver Hunt]

Created attachment 204958 [details]
Patch

[Geoffrey Garen]

Comment on attachment 204958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204958&action=review

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:458
> +    void flushInlineStackEntry(InlineStackEntry* inlineStackEntry)

You can just call this "flush", since the type disambiguates in C++.

[Oliver Hunt]

Committed r151709: <http://trac.webkit.org/changeset/151709>