The default bug view has changed. See this FAQ.

Assertion failure: fp_->isGlobalFrame() || fp_->isDebuggerFrame(), at vm/ScopeObject.cpp:1231

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: luke)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla15
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The following test asserts on mozilla-central revision d55df2c9c037 (options -m -n -a):


gcparam("maxBytes", gcparam("gcBytes") + 4*1024)     ;   
evaluate("\
test();\
function test() {\
  ( test ('test')     )  ('test');\
    eval('with({}) let y;');\
}\
");
(Assignee)

Comment 1

5 years ago
Created attachment 625544 [details] [diff] [review]
fix and test

This test case exposes a very interesting corner case: if there is an error in a heavyweight function frame before the prologue has completed, then the scope will still be fun->environment().  Fortunately, the ScopeIter abstraction allows us to handle this corner case in one place.  (I will need to re-jigger bug 659577 since HAS_CALL_OBJ is now meaningful, though.)
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #625544 - Flags: review?(jimb)
(Assignee)

Updated

5 years ago
Whiteboard: js-triage-needed → js-triage-done

Comment 2

5 years ago
Comment on attachment 625544 [details] [diff] [review]
fix and test

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

I don't feel quite so bad for having missed this possibility in the review. :)

::: js/src/jit-test/jit_test.py
@@ +243,5 @@
>  
>      if rc != test.expect_status:
>          # Allow a non-zero exit code if we want to allow OOM, but only if we
>          # actually got OOM.
> +        return test.allow_oom and ': out of memory' in err and 'Assertion failure' not in err

Would it be better to simply add 'Assertion failure' to the test in the 'for' loop above?

::: js/src/jsinterpinlines.h
@@ +568,5 @@
>  
>      JS_ASSERT(!fp->hasBlockChain());
>      JSObject &scope = *fp->scopeChain();
>  
> +    if (fp->fun()->isHeavyweight() && fp->hasCallObj())

It'd be nice to have a one-line comment here, since this is a configuration where it's hard to see why the special case is needed.

::: js/src/vm/ScopeObject.cpp
@@ +1184,5 @@
>       * chain. Beware: while ScopeIter iterates over the scopes of a single
>       * frame, the scope chain (pointed to by cur_) continues into the scopes of
>       * enclosing frames. Thus, it is important not to look at cur_ until it is
>       * certain that cur_ points to a scope object in the current frame. In
> +     * particular, there are three tricky corner cases:

NOBODY EXPECTS THE SPANISH INQUISITION

@@ +1214,5 @@
>              hasScopeObject_ = false;
>          } else {
>              fp_ = NULL;
>          }
> +    } else if (fp_->isNonEvalFunctionFrame() && !fp_->hasCallObj()) {

Again, a one-line comment here, "observed before prologue has finished", would be good.
Attachment #625544 - Flags: review?(jimb) → review+
Whiteboard: js-triage-done → [js:t]
(Assignee)

Comment 3

5 years ago
(In reply to Jim Blandy :jimb from comment #2)
> Would it be better to simply add 'Assertion failure' to the test in the
> 'for' loop above?

The bug is triggered by OOM which is an uncatchable exception.

> Again, a one-line comment here, "observed before prologue has finished",
> would be good.

Well, that was the goal of the fat explanatory comment above: to give the cases a cohesive explanation; do you still think there is something more to be said?  I could number the cases in the fat comment and put the numbers on the associated if's...
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15f13527d49
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/f15f13527d49
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.