Last Comment Bug 756919 - Assertion failure: fp_->isGlobalFrame() || fp_->isDebuggerFrame(), at vm/ScopeObject.cpp:1231
: Assertion failure: fp_->isGlobalFrame() || fp_->isDebuggerFrame(), at vm/Scop...
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla15
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: langfuzz
  Show dependency treegraph
Reported: 2012-05-20 16:14 PDT by Christian Holler (:decoder)
Modified: 2012-05-26 05:09 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix and test (4.81 KB, patch)
2012-05-20 19:26 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2012-05-20 16:14:38 PDT
The following test asserts on mozilla-central revision d55df2c9c037 (options -m -n -a):

gcparam("maxBytes", gcparam("gcBytes") + 4*1024)     ;   
function test() {\
  ( test ('test')     )  ('test');\
    eval('with({}) let y;');\
Comment 1 User image Luke Wagner [:luke] 2012-05-20 19:26:22 PDT
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.)
Comment 2 User image Jim Blandy :jimb 2012-05-22 14:00:49 PDT
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/
@@ +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:


@@ +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.
Comment 3 User image Luke Wagner [:luke] 2012-05-23 00:06:45 PDT
(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...
Comment 5 User image Ryan VanderMeulen [:RyanVM] 2012-05-26 05:09:30 PDT

Note You need to log in before you can comment on or make changes to this bug.