1233117 - Assertion failure: iter.isFunctionFrame() || iter.isGlobalFrame(), at js/src/vm/Stack.cpp:65

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 749f9328dd76 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

let moduleRepo = new Map();
let d = moduleRepo['d'] = parseModule('var x = "eval"; eval("x")');
d.declarationInstantiation();
d.evaluation();


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000adf09c in js::InterpreterFrame::initExecuteFrame (this=this@entry=0x7ffff4671228, cx=cx@entry=0x7ffff6907400, script=script@entry=..., evalInFramePrev=..., newTargetValue=..., scopeChain=..., scopeChain@entry=..., type=type@entry=js::EXECUTE_DIRECT_EVAL) at js/src/vm/Stack.cpp:65
#0  0x0000000000adf09c in js::InterpreterFrame::initExecuteFrame (this=this@entry=0x7ffff4671228, cx=cx@entry=0x7ffff6907400, script=script@entry=..., evalInFramePrev=..., newTargetValue=..., scopeChain=..., scopeChain@entry=..., type=type@entry=js::EXECUTE_DIRECT_EVAL) at js/src/vm/Stack.cpp:65
#1  0x0000000000adf30a in js::InterpreterStack::pushExecuteFrame (this=<optimized out>, cx=0x7ffff6907400, script=..., newTargetValue=..., scopeChain=..., type=js::EXECUTE_DIRECT_EVAL, evalInFrame=...) at js/src/vm/Stack.cpp:525
#2  0x0000000000a437d2 in js::ExecuteState::pushInterpreterFrame (this=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter.cpp:345
#3  0x0000000000a656cd in Interpret (cx=cx@entry=0x7ffff6907400, state=...) at js/src/vm/Interpreter.cpp:1601
#4  0x0000000000a758c7 in js::RunScript (cx=cx@entry=0x7ffff6907400, state=...) at js/src/vm/Interpreter.cpp:391
#5  0x0000000000a7b281 in js::ExecuteKernel (cx=cx@entry=0x7ffff6907400, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., type=type@entry=js::EXECUTE_DIRECT_EVAL, evalInFrame=..., evalInFrame@entry=..., result=0x7ffff4671200) at js/src/vm/Interpreter.cpp:650
#6  0x00000000005bf504 in EvalKernel (cx=cx@entry=0x7ffff6907400, args=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., scopeobj=..., scopeobj@entry=..., pc=<optimized out>) at js/src/builtin/Eval.cpp:332
#7  0x00000000005bfc8d in js::DirectEval (cx=cx@entry=0x7ffff6907400, args=...) at js/src/builtin/Eval.cpp:440
#8  0x0000000000a68358 in Interpret (cx=cx@entry=0x7ffff6907400, state=...) at js/src/vm/Interpreter.cpp:2682
#9  0x0000000000a758c7 in js::RunScript (cx=cx@entry=0x7ffff6907400, state=...) at js/src/vm/Interpreter.cpp:391
#10 0x0000000000a7b281 in js::ExecuteKernel (cx=cx@entry=0x7ffff6907400, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., type=<optimized out>, evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7ffff4671160) at js/src/vm/Interpreter.cpp:650
#11 0x0000000000a7b50e in js::Execute (cx=cx@entry=0x7ffff6907400, script=script@entry=..., scopeChainArg=..., rval=rval@entry=0x7ffff4671160) at js/src/vm/Interpreter.cpp:685
#12 0x0000000000816885 in js::ModuleObject::evaluate (cx=cx@entry=0x7ffff6907400, self=..., self@entry=..., rval=rval@entry=...) at js/src/builtin/ModuleObject.cpp:817
#13 0x0000000000ad3360 in intrinsic_EvaluateModule (cx=0x7ffff6907400, argc=<optimized out>, vp=0x7ffff4671160) at js/src/vm/SelfHosting.cpp:1344
#14 0x0000000000a7d572 in js::CallJSNative (cx=0x7ffff6907400, native=0xad32f0 <intrinsic_EvaluateModule(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#29 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6877
rax	0x0	0
rbx	0x7ffff4671228	140737293783592
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffac80	140737488333952
rsp	0x7fffffffa7d0	140737488332752
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffa590	140737488332176
r11	0x7ffff6c27960	140737333328224
r12	0x7ffff6907400	140737330050048
r13	0x7fffffffa7e0	140737488332768
r14	0x7fffffffb438	140737488335928
r15	0x7fffffffb400	140737488335872
rip	0xadf09c <js::InterpreterFrame::initExecuteFrame(JSContext*, JS::Handle<JSScript*>, js::AbstractFramePtr, JS::Value const&, JS::Handle<JSObject*>, js::ExecuteType)+812>
=> 0xadf09c <js::InterpreterFrame::initExecuteFrame(JSContext*, JS::Handle<JSScript*>, js::AbstractFramePtr, JS::Value const&, JS::Handle<JSObject*>, js::ExecuteType)+812>:	movl   $0x41,0x0
   0xadf0a7 <js::InterpreterFrame::initExecuteFrame(JSContext*, JS::Handle<JSScript*>, js::AbstractFramePtr, JS::Value const&, JS::Handle<JSObject*>, js::ExecuteType)+823>:	callq  0x4a3db0 <abort()>

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/db4c17553be9
user:        Jon Coppeard
date:        Wed Sep 23 15:47:40 2015 +0100
summary:     Bug 930414 - Implement ModuleEvaluation method r=shu

This iteration took 247.354 seconds to run.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jon, is bug 930414 a likely regressor?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1233117-module-eval

Patch to fix the assertion here to allow the interpreter to execute eval inside a module.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8700036 [details] [diff] [review]
bug1233117-module-eval

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

r=me with question below addressed.

::: js/src/vm/Stack.cpp
@@ +69,5 @@
>                      newTarget = iter.newTarget();
>                  callee = iter.callee(cx);
>                  flags_ |= FUNCTION;
>              } else {
>                  flags_ |= GLOBAL;

So it's okay to set the GLOBAL flag?

Each eval frame currently is also either a global frame or a function frame, depending on the outer scope. I wonder if eval inside a module should be treated similarly as "module frame" instead of "global frame".

Looking at the (few) callers of frame->isModuleFrame(), this is probably fine though.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1233117-module-eval v2

Not sure what I was thinking when I wrote that patch.  Making eval inside a module a module frame makes much more sense.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8701025 [details] [diff] [review]
bug1233117-module-eval v2

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

::: js/src/jsscript.h
@@ +1624,5 @@
>      }
>      inline void setModule(js::ModuleObject* module);
>  
> +    bool isGlobalCode() const {
> +        return !function_ && !module_;

Should we add && !isForEval()? Else we will return true for eval scripts inside functions.

::: js/src/vm/Stack.cpp
@@ +70,5 @@
>                      newTarget = iter.newTarget();
>                  callee = iter.callee(cx);
>                  flags_ |= FUNCTION;
> +            } else if (iter.isModuleFrame()) {
> +                flags_ |= MODULE;

BaselineFrame::isModuleFrame() currently looks at the callee token. To be consistent with this code, we should change jit::EnterBaselineAtBranch and jit::SetEnterJitData to use the module script as CalleeToken (similar to how eval-inside-function has the function as CalleeToken).

Where do we rely on frame->isModuleFrame() (and can't use script->module() instead)? I wonder if we should just remove it and treat these frames as global frames. If not, we should be able to write a test for it.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Where do we rely on frame->isModuleFrame() (and can't use script->module()
> instead)? I wonder if we should just remove it and treat these frames as
> global frames. If not, we should be able to write a test for it.

Looking at the callers of isModuleFrame(), I think we could treat module frames as global frames (with non-null script->module()), so that might be nice to avoid the extra frame bookkeeping.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug1233117-module-eval-v3

How about this?  I left isModuleFrame() and isGlobalFrame() in a couple of places since they are used a few times.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8703608 [details] [diff] [review]
bug1233117-module-eval-v3

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

Clever, thanks! Sorry for the delay.

::: js/src/jsscript.h
@@ +1628,5 @@
>      inline void setModule(js::ModuleObject* module);
>  
> +    bool isGlobalCode() const {
> +        return !function_ && !module_ && !isForEval();
> +    }

(Bug 1233109 also added isGlobalCode() but that version doesn't have the !isForEval check.)

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #10)

> (Bug 1233109 also added isGlobalCode() but that version doesn't have the
> !isForEval check.)

The patch in bug 1233109 depends on isGlobalCode() not having the eval check.  Maybe it should be called isTopLevelCode() instead?

Where it's called in this patch we already assert !isForEval() beforehand.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/624b95b674e6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/624b95b674e6