741241 - IonMonkey: segmentation fault after calling new on local function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

The following testcase segment faults on IonMonkey (tip, 32-debug/32-opt, --ion -n).

function test() {
  function Array() {}
  new Array()
}
for (var i=0; i<100; i++) {
    test()
}

Backtrace
#0  0x0839358c in js::HeapPtr<js::ion::IonCode, unsigned int>::operator js::ion::IonCode* (this=0x0) at ../gc/Barrier.h:214
#1  0x0838f2cf in js::ion::IonScript::method (this=0x0) at ../ion/IonCode.h:270
#2  0x0838e1ed in js::ion::Cannon (cx=0x8702e40, fp=0xf7557070, newType=false) at /home/h4writer/Build/ionmonkey/js/src/ion/Ion.cpp:997
#3  0x081364c6 in js::RunScript (cx=0x8702e40, script=0xf7306128, fp=0xf7557070) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:465
#4  0x0813695e in js::InvokeKernel (cx=0x8702e40, args=..., construct=js::CONSTRUCT) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:539
#5  0x08136cd7 in js::InvokeConstructorKernel (cx=0x8702e40, argsRef=...) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:602
#6  0x08088fcf in js::InvokeConstructor (cx=0x8702e40, args=...) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.h:206
#7  0x08136e36 in js::InvokeConstructor (cx=0x8702e40, fval=..., argc=0, argv=0xffffc724, rval=0xffffc700) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:628
#8  0x084972a6 in js::ion::InvokeConstructorFunction (cx=0x8702e40, fun=0xf730dde0, argc=0, argv=0xffffc71c, rval=0xffffc700) at /home/h4writer/Build/ionmonkey/js/src/ion/VMFunctions.cpp:79
#9  0xf7547743 in ?? ()
#10 0x0838df6f in EnterIon (cx=0x8702e40, fp=0xf7557020, jitcode=0xf75474d5) at /home/h4writer/Build/ionmonkey/js/src/ion/Ion.cpp:964
#11 0x0838e2bb in js::ion::SideCannon (cx=0x8702e40, fp=0xf7557020, pc=0x8711058  <incomplete sequence \344\232>) at /home/h4writer/Build/ionmonkey/js/src/ion/Ion.cpp:1013
#12 0x0813b4ce in js::Interpret (cx=0x8702e40, entryFrame=0xf7557020, interpMode=js::JSINTERP_NORMAL) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:1793
#13 0x08136552 in js::RunScript (cx=0x8702e40, script=0xf7306258, fp=0xf7557020) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:480
#14 0x081370d9 in js::ExecuteKernel (cx=0x8702e40, script=0xf7306258, scopeChain=..., thisv=..., type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x0)
    at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:678
#15 0x081372dc in js::Execute (cx=0x8702e40, script=0xf7306258, scopeChainArg=..., rval=0x0) at /home/h4writer/Build/ionmonkey/js/src/jsinterp.cpp:720
#16 0x08081f48 in JS_ExecuteScript (cx=0x8702e40, obj=0xf7303040, script=0xf7306258, rval=0x0) at /home/h4writer/Build/ionmonkey/js/src/jsapi.cpp:5243
#17 0x0804c2dc in Process (cx=0x8702e40, obj=0xf7303040, filename=0xffffd48a "/tmp/test3.js", forceTTY=false) at /home/h4writer/Build/ionmonkey/js/src/shell/js.cpp:479
#18 0x08056c57 in ProcessArgs (cx=0x8702e40, obj=0xf7303040, op=0xffffd194) at /home/h4writer/Build/ionmonkey/js/src/shell/js.cpp:4798
#19 0x08056e90 in Shell (cx=0x8702e40, op=0xffffd194, envp=0xffffd2c8) at /home/h4writer/Build/ionmonkey/js/src/shell/js.cpp:4881
#20 0x0805783b in main (argc=4, argv=0xffffd2b4, envp=0xffffd2c8) at /home/h4writer/Build/ionmonkey/js/src/shell/js.cpp:5111

Comment 1

[Nicolas B. Pierron [:nbp]]

This error can be reproduce with --ion --ion-eager -n and the following test case:

function f(){}
new f()

In which case we do not even enter jit code and fail by invalidating the newly compiled code by calling js_CreateThis… in ion::Cannon.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Avoid invalidation between compilation and EnterIon

Move js_CreateThis… from ion::Cannon to ion::CanEnter, such that invalidation  does not garbage the not-yet entered ion code.

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 612293 [details] [diff] [review]
Avoid invalidation between compilation and EnterIon

Review of attachment 612293 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/Ion.cpp
@@ +911,5 @@
>      // Skip if the script has been disabled.
>      if (script->ion == ION_DISABLED_SCRIPT)
>          return Method_Skipped;
>  
> +    // If constructing, allocate a new |this| object before entering Ion.

This comment should be expanded to explain why constructing is done in this function.

@@ +912,5 @@
>      if (script->ion == ION_DISABLED_SCRIPT)
>          return Method_Skipped;
>  
> +    // If constructing, allocate a new |this| object before entering Ion.
> +    if (fp->isConstructing() && fp->functionThis().isPrimitive()) {

This step must be performed after Compile() returns. We should not be optimizing for the --ion-eager case.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Sean Stangl from comment #3)
> @@ +912,5 @@
> >      if (script->ion == ION_DISABLED_SCRIPT)
> >          return Method_Skipped;
> >  
> > +    // If constructing, allocate a new |this| object before entering Ion.
> > +    if (fp->isConstructing() && fp->functionThis().isPrimitive()) {
> 
> This step must be performed after Compile() returns. We should not be
> optimizing for the --ion-eager case.

We cannot do it after the return of Compile(), because Compile set script->ion, and the setThis function which is which is called by js_CreateThis will invalidate the result of the compilation and then cause script->ion to be set to NULL even if we succeed at compiling it, which cause the SEGV reported in the description of this bug (even without --ion-eager).

Comment 5

[Sean Stangl [:sstangl]]

(In reply to Nicolas B. Pierron [:pierron] from comment #4)
> We cannot do it after the return of Compile(), because Compile set
> script->ion, and the setThis function which is which is called by
> js_CreateThis will invalidate the result of the compilation and then cause
> script->ion to be set to NULL even if we succeed at compiling it, which
> cause the SEGV reported in the description of this bug (even without
> --ion-eager).

Line 927, right at the end of CanEnter(), checks whether script->ion is still valid.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Sean Stangl from comment #5)
> (In reply to Nicolas B. Pierron [:pierron] from comment #4)
> > We cannot do it after the return of Compile(), because Compile set
> > script->ion, and the setThis function which is which is called by
> > js_CreateThis will invalidate the result of the compilation and then cause
> > script->ion to be set to NULL even if we succeed at compiling it, which
> > cause the SEGV reported in the description of this bug (even without
> > --ion-eager).
> 
> Line 927, right at the end of CanEnter(), checks whether script->ion is
> still valid.

Doing so implies compiling the inner function twice before executing the jitted code.

function test() {
  function inner() {
    … huge …
  }
  new inner()
}
for (var i=0; i<100; i++) {
    test()
}

Updating This before starting the compilation should only impact the performances for the case where the compilation fails, which is still higher than updating This.  Knowing that we register the failure in script->ion, and we check for failure before updating This again, this should not be a problem. (small cost on failure)

Updating This after the compilation is finished should only impact the performances of compiling functions where This is not registered anywhere else than in the function calling it with a new for the first time.  This implies that we have to discard the changes of the compilation and start again the compilation with the new information.  The cost of a compilation overcome the cost of the update and we want to increase the number of successful compilations. (Huge cost on successful compilation)

I don't think this is a good idea to move this check after the Compile function is executed or to add a late failure in CanEnter.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/a9a18824b4c1

Comment 8

[Paul Wright]

Wrong bug # (741271) referenced in commit message.