Closed Bug 852175 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla28

People

(Reporter: decoder, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update,testComment=8,origRev=59ff3a2a708a])

Attachments

(3 files, 1 obsolete file)

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)(); 
  }  
}
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.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Brian, is the bisect in comment 2 likely to be correct?
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unknown exception (check manually)
Whiteboard: [jsbugmon:] → [jsbugmon:update]
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f20b0ce9e528).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
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.
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).
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; }");
Whiteboard: [jsbugmon:] → [jsbugmon:update,bisect,testComment=8,origRev=59ff3a2a708a]
Whiteboard: [jsbugmon:update,bisect,testComment=8,origRev=59ff3a2a708a] → [jsbugmon:update,testComment=8,origRev=59ff3a2a708a]
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.
Setting needinfo? per comment 9.
Flags: needinfo?(kvijayan)
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.
Attached patch fix-bug-852175.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(kvijayan)
Assignee: general → kvijayan
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.
Attachment #823646 - Flags: review?(jdemooij)
(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?
Good catch.  Updating patch and re-posting.
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.
Attachment #823646 - Attachment is obsolete: true
Attachment #823646 - Flags: review?(jdemooij)
Attachment #824832 - Flags: review?(jdemooij)
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
Attachment #824832 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/c532cabb71ec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 934427
Keywords: verifyme
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?
Flags: needinfo?(kvijayan)
Keywords: verifyme
> - "InternalError: too much recursion" with the 02/04 js shell.

This is also reproducible with the Fx30 02/05 shell.
(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 :)
Flags: needinfo?(kvijayan)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.