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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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;');\
}\
");

Comment 1

[Luke Wagner [:luke]]

Attachment: 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

[Jim Blandy :jimb]

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.

Comment 3

[Luke Wagner [:luke]]

(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 4

[Luke Wagner [:luke]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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