852175 - Assertion failure: cur_ == frame_.fun()->environment(), at vm/ScopeObject.cpp:1046

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision b03bb3ce8cee (no options required):


loadFile('\
gcparam("maxBytes", gcparam("gcBytes") + 4*1024);\
function f() {\
    var inner4 = f("get"),\
    otherGlobalSameCompartment = newGlobal(\'{"a":5,"\', true);\
    eval("");\
}\
assertEq("aaa".replace((/\u004e/ ), f()), "poniesponiesponies");\
');
function loadFile(lfVarx) {
  if (lfVarx.substr(-3) != ".js") {
    new Function(lfVarx)(); 
  }  
}

Comment 1

[Christian Holler (:decoder)]

This is an OOM issue and it has been there for quite a while but I managed to find a somewhat reliable test only now. Hopefully this can be fixed so it stops triggering.

Ccing jandem since it looks like he added the assertion.

Comment 2

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   119344:1591384ce3ad
user:        Brian Hackett
date:        Fri Jan 18 20:35:08 2013 -0700
summary:     Bug 832042 - Reduce cost of exact stack rooting during addition operations, r=terrence.

This iteration took 112.888 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Brian, is the bisect in comment 2 likely to be correct?

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 5

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision f20b0ce9e528).

Comment 6

[Christian Holler (:decoder)]

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

The first good revision is:
changeset:   125319:1255520f471b
user:        Jason Orendorff
date:        Mon Mar 18 15:32:29 2013 -0700
summary:     Bug 848062 - Make arrow functions inherit the value of 'this' from the enclosing scope. r=bhackett.

This iteration took 172.910 seconds to run.

Comment 7

[Christian Holler (:decoder)]

This is unlikely to be fixed as I'm still seeing it on trunk. It would be helpful if a developer could debug this on the original revision to fix it (it's an OOM issue).

Comment 8

[Christian Holler (:decoder)]

I'm still seeing this assert, and here's another test (this also looks like OOM/out-of-stack, mozilla-central rev 59ff3a2a708a, no options required):


var g = newGlobal();
var dbg = new Debugger(g);
g.eval("function f(n) { if (n == 0) debugger; else f(n - 1); }");
g.f("function f() { debugger; }");

Comment 9

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0452b5b504d0
user:        Kannan Vijayan
date:        Tue Sep 10 06:19:30 2013 -0400
summary:     Bug 905523 - On windows, incrementally touch large baseline frames before using them. r=efaust

This iteration took 418.330 seconds to run.

Comment 10

[Jan de Mooij [:jandem]]

Setting needinfo? per comment 9.

Comment 11

[Kannan Vijayan [:djvj]]

Issue is that emitStackCheck moved up before initScopeChain.  If emitStackCheck throws, then when it uses the scope chain iterator, and the iterator tries to settle, it runs across a baseline frame with a NULL scopeChain because it hasn't been initialized yet.

I can't move the scopeChain initialization above the stack check, because that would put it before the locals initialization code.  That's not OK because if we're bailing out into the prologue, we can't re-do the locals slot initialization to undefined (since that'll mess the stack up).

I think the way to fix this is to get stack check not to throw immediately, but wait until after the scope chain has been initialized.  If the stack check fails, then we skip locals initialization, jump directly to scope chain initialization, and then after scope chain initialization, make sure to throw an exception then.

Comment 12

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-852175.patch

Tentative fix.  Passes jit-tests on x86-64.

Summary:
For scripts with more than a threshold number of slots, we call stack check twice: he first time prior to initializing locals slots, and the second time after scope chain initialization.

The first time entry is infallible.  Instead of returning false, it sets a flag on the frame indicating that it has overflown.  On return, we check this flag and if it's set, skip past the locals initialization, straight down to scope chain initialization followed by the second stack check call.

The second stack check call is modified to check if the overrecursed flag is set on the stack frame and if so, return false.  This way, we avoid both the possibility of throwing an exception before the scope chain is initialized, and also initializing the large number of locals that may overflow the stack depth limit.

Comment 13

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 14

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 15

[Kannan Vijayan [:djvj]]

Comment on attachment 823646 [details] [diff] [review]
fix-bug-852175.patch

Passes try: https://tbpl.mozilla.org/?tree=Try&rev=419779a1ccca

See comment 12 for description of patch.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #12)
> The second stack check call is modified to check if the overrecursed flag is
> set on the stack frame and if so, return false.

This is inside the VM function; won't the inline, second check bypass the VM call completely if we don't push the locals?

Comment 17

[Kannan Vijayan [:djvj]]

Good catch.  Updating patch and re-posting.

Comment 18

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-852175.patch

Updated patch.  Changes to previous patch:

1. Adds convenience method to check for whether the baseline script being compiled needs both an early and late stack check.

2. Modifies the late stack check's guard code, in only those situations where both a late and early stack check are emitted, such that the OVER_RECURSED flag is checked for in the jitcode and if true, the VMCall to CheckOverRecursedWithInfo made unconditionally.

3. Adds bug to jit-test test cases.

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 824832 [details] [diff] [review]
fix-bug-852175.patch

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

Nice, r=me with nits addressed.

::: js/src/jit/BaselineCompiler.cpp
@@ +285,5 @@
>          masm.storePtr(ImmPtr(nullptr), frame.addressOfScopeChain());
>      else
>          masm.storePtr(R1.scratchReg(), frame.addressOfScopeChain());
>  
> +    Label earlyStackCheckFailed;

Can you add a comment here like the summary you posted in comment 12? It'd be good to explain here why we have to use two checks and how they work.

@@ +290,5 @@
> +    if (needsEarlyStackCheck()) {
> +        if (!emitStackCheck(/* earlyCheck = */ true))
> +            return false;
> +        masm.branchTest32(Assembler::NonZero,
> +                          frame.addressOfFlags(), 

Nit: trailing whitespace.

@@ +458,5 @@
> +    // VMCall to CheckOverRecursed done unconditionally if it's set.
> +    Label forceCall;
> +    if (!earlyCheck && needsEarlyStackCheck()) {
> +        masm.branchTest32(Assembler::NonZero,
> +                          frame.addressOfFlags(), 

Nit: trailing whitespace.

@@ +467,1 @@
>      masm.branchPtr(Assembler::BelowOrEqual, AbsoluteAddress(limitAddr), R1.scratchReg(),

I think it'd be a bit clearer/more efficient to move this check inside an "else" block, since we don't need it if we did an early check. Then you can also remove the forceCall label and invert the condition and jump to skipCall.

::: js/src/jit/BaselineFrame.h
@@ +311,5 @@
> +    bool overRecursed() const {
> +        return flags_ & OVER_RECURSED;
> +    }
> +
> +    bool setOverRecursed() {

s/bool/void

Comment 20

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c532cabb71ec

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c532cabb71ec

Comment 22

[Ioana (away)]

Tested with the Fx28 10/31 and 02/04 JS shells on Ubuntu 12.10 x86_x64:

The testcase from comment 0 gives "uncaught exception: out of memory" with both js shells.

The testcase from comment 8 gives:
- "Assertion failure: cur_ == frame_.fun()->environment(), at ../../../js/src/vm/ScopeObject.cpp:1058
Segmentation fault (core dumped)" with the 10/31 js shell.
- "InternalError: too much recursion" with the 02/04 js shell.

Is there a chance this error might be hiding a still existing assertion failure?

Comment 23

[Ioana (away)]

> - "InternalError: too much recursion" with the 02/04 js shell.

This is also reproducible with the Fx30 02/05 shell.

Comment 24

[Kannan Vijayan [:djvj]]

(In reply to Ioana Budnar, QA [:ioana] from comment #22)
> Tested with the Fx28 10/31 and 02/04 JS shells on Ubuntu 12.10 x86_x64:
> 
> The testcase from comment 0 gives "uncaught exception: out of memory" with
> both js shells.
> 
> The testcase from comment 8 gives:
> - "Assertion failure: cur_ == frame_.fun()->environment(), at
> ../../../js/src/vm/ScopeObject.cpp:1058
> Segmentation fault (core dumped)" with the 10/31 js shell.
> - "InternalError: too much recursion" with the 02/04 js shell.
> 
> Is there a chance this error might be hiding a still existing assertion
> failure?

This is the correct behaviour.

The test case on comment 8 will loop forever if run correctly, with |f| repeately calling itself with |NaN| until it exhausts the stack.  A recursion exception is appropriate for the engine to throw.

I'd call this verified :)