592202 - Crash [@ CallPropertyOp] or [@ js::SetFlatUpvar] or "Assertion failure: FUN_FLAT_CLOSURE(callee_fun)," or "Assertion failure: fun->isFlatClosure(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

eval("let(y){(function(){let({}=y){(function(){let({}=y=[])(i)})()}})()}")

crashes js opt shell on TM changeset e8ee411dca70 at CallPropertyOp without -j and asserts js debug shell at Assertion failure: FUN_FLAT_CLOSURE(callee_fun),

s-s because this involves a scary address (0x40).

===

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0005890e in CallPropertyOp ()
(gdb) bt
#0  0x0005890e in CallPropertyOp ()
#1  0x00084f65 in js_NativeSet ()
(gdb) x/i $eip
0x5890e <_ZL14CallPropertyOpP9JSContextP8JSObjectiPN2js5ValueE18JSCallPropertyKindi+158>:       mov    %eax,(%ecx,%esi,8)
(gdb) x/b $eax
0x11023b8:      0x40
(gdb) x/b $ecx
0x0:    Cannot access memory at address 0x0
(gdb) x/b $esi
0x0:    Cannot access memory at address 0x0

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Probably related to bug 558451.

Regression window:

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=be9979b4c10b&tochange=f3e58c264932

Comment 2

[Daniel Veditz [:dveditz]]

Is this a (near-)null deref? or just happens to be in this testcase?

Comment 3

[Johnny Stenback (:jst)]

Ping?

Comment 4

[Brendan Eich [:brendan]]

Some kind of regression from the patch stack for bug 558451 (Gary, can you confirm?). It's thus very recent, so no branch worries. I will need more time to dig into this one.

/be

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #4)
> Some kind of regression from the patch stack for bug 558451 (Gary, can you
> confirm?). It's thus very recent, so no branch worries. I will need more time
> to dig into this one.
> 
> /be

Seems to be - the regression window is in comment #1.

Comment 6

[Bob Clary [:bc] (inactive)]

1. http://deklomp.se/
2. Assertion failure: FUN_FLAT_CLOSURE(callee_fun)

This is associated with the socorro signature GetFlatUpvar. Although it only shows Linux (15) and Mac (1) in the last 30 days, I also see this on Windows.

Search goodness: JS_Assert CallPropertyOp GetFlatUpvar js::CallJSPropertyOp js::Shape::get

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: semi reduced testcase

based on deklomp.se page: still loads external scripts. requires meta tag.

Comment 8

[Jesse Ruderman]

Not a null deref for me. I see |array| as (js::Value *) 0xfff2000000000000 when I try the testcase in an opt-vg xpcshell on Linux64.

Comment 9

[Jesse Ruderman]

Attachment: jesse's gdb output

Comment 10

[Johnny Stenback (:jst)]

Brendan, any updates here yet?

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

eval("\
  let(b)((\
    function(){\
      let(d=b)\
      ((function(){\
        b=b\
      })())\
    }\
  )())\
")

asserts similarly and crashes opt shell at js::SetFlatUpvar without -m nor -j.

Program received signal SIGSEGV, Segmentation fault.
0x080b43f8 in js::SetFlatUpvar(JSContext*, JSObject*, int, js::Value*) ()
(gdb) bt
#0  0x080b43f8 in js::SetFlatUpvar(JSContext*, JSObject*, int, js::Value*) ()
#1  0x083396f0 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb) x/i $eip
=> 0x80b43f8 <_ZN2js12SetFlatUpvarEP9JSContextP8JSObjectiPNS_5ValueE+56>:	mov    %esi,(%edx,%eax,8)
(gdb) x/b $esi
0x0:	Cannot access memory at address 0x0
(gdb) x/b $edx
0x0:	Cannot access memory at address 0x0
(gdb) x/b $eax
0x0:	Cannot access memory at address 0x0

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

bug 616762 looks like a dupe of this, though I like the reduced test case there better. At a glance, we could consider just removing MakeUpvarForEval. Chris is already removing most cases of the UPVAR ops in bug 614834, since ICs are faster.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

bug 616762 looks unrelated, after all. The symptoms are very similar but the MakeUpvarForEval path isn't taken.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This still occurs in changeset df86d5999fef.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

#6 and #9 on 4.0b9pre top crash list.

Comment 16

[Johnny Stenback (:jst)]

Bumping this to beta9, Gary believes that fixing this would take care of some other related crashes that appear in crahs-stats as well, and getting this in sooner rather than later would give us more time to react on those crashes if they turn out to not be fixed by this bug.

Comment 18

[Brendan Eich [:brendan]]

Attachment: Jesse's most reduced testcase from bug 621991

Comment 19

[Brendan Eich [:brendan]]

This is dopey, sorry for not looking into it sooner. The bug is an assertion and consequent requirements where there should be a guard with an early return true. The patch I'm about to attach minimizes change to avoid conflicting more than absolutely necessary with Waldo's specialized expansions of CallPropertyOp.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: minimal fix with collected tests

Comment 21

[Brendan Eich [:brendan]]

Removing scopes left compiler-created shape paths held by fun->u.i.names as precomputed scope replacements for all Call objects created for fun. The bug is that all UPVAR locals in a function have GetFlatUpvar and SetFlatUpvar as their getter and setter, to indicate their JSLocalKind. But unless the function is in fact a flat closure, these getter and setter functions should be no-ops.

/be

Comment 24

[Brendan Eich [:brendan]]

There's more here than met my eye last night. The assumption was that we'd never have a Call object on the scope chain for a function that has upvars mapped as shapes if that function is not a flat closure. Such upvar-using lightweights make use of JSOP_*NAME and JSOP_GETUPVAR but do not themselves need Call objects.

This assumption holds in all the tests if I revert the patch for bug 545573 to js_NewFlatClosure:

-JSObject *
+JS_REQUIRES_STACK JSObject *
 js_NewFlatClosure(JSContext *cx, JSFunction *fun)
 {
-    JSObject *closure = js_AllocFlatClosure(cx, fun, cx->fp->scopeChain);
+    /*
+     * Flat closures can be partial, they may need to search enclosing scope
+     * objects via JSOP_NAME, etc.
+     */
+    JSObject *scopeChain = js_GetScopeChain(cx, cx->fp);
+    if (!scopeChain)
+        return NULL;
+
+    JSObject *closure = js_AllocFlatClosure(cx, fun, scopeChain);

I should have reverted this when I fixed bug 554572:

changeset:   40000:77d5a7f48a4e
user:        Brendan Eich <[e-mail]>
date:        Wed Mar 24 13:26:28 2010 -0700
summary:     Disable partial flat closures pending scope chain reconstruction on trace (554572, r=jorendorff; CLOSED TREE push ok'ed by sayrer).

But since js_NewFlatClosure gets the full scope chain, it makes a Call object for any function containing a flat closure, even if that function has upvar shapes in its bindings.

We don't do partial flat closures. That's a FIXME: bug 545759. Therefore when we make a flat closure, we do not need to reify the full scope chain including Call objects induced by block objects (which arise from catch clauses), and indeed we must not create Call objects for enclosing functions that are not themselves flat.

This is all a bit too delicate, but I am testing a patch that should fix all the bugs and speed up flat closure computation.

/be

Comment 25

[Brendan Eich [:brendan]]

> indeed we must not create Call objects for enclosing functions that are not
> themselves flat.

But this is of course impossible to prevent, at least due to the debugger -- see tests/ecma/extensions/trapflatclosure.js for one testcase.

It's impossible to avoid also because we specialize JSOP_NAME to JSOP_GETUPVAR, ditto CALLNAME/CALLUPVAR, but the mutating variants do not have specialized UPVAR forms. If the function is heavyweight, these ops can't be used. If it escapes as a funarg, these two ops won't be emitted either. So it's easy to make tests that have JSOP_GETUPVAR but JSOP_SETNAME (the tests for this bug, Jesse's reduced one in particular) for the same upvar, and the JSOP_SETNAME works on a Call object for the function containing the upvar reference.

But since scope removal such a function containing an upvar ref that's used by one or more JSOP_GETUPVARs will have a Call object property for the same upvar, in effect shadowing the upvar. The getter for this property won't be called (JSOP_GETUPVAR takes a faster, cough, maybe not so much without the display, path). But JSOP_SETNAME will call js::SetFlatUpvar.

The symmetry break where JSOP_GET/CALLUPVAR are specialized but no mutating ops are, falling back on JSOP_SETNAME, etc., combined with independent causes of the function containing the upvar refs to need a Call object (e.g. it contains let or try/catch statements where the lexical scope is captured by a closure -- but not eval or with, those make heavyweights and eliminate the JSOP_GET/CALLUPVAR ops), means the compiler-precomputed upvar shapes -- the ones that were addUpvar'ed to bindings structs -- must shadow their actual upvars along the scope chain.

So the setter, currently named js::SetFlatUpvar, must update the true home of the upvar, in the outer Call or Block (let in global scope) object.

Patch is close.

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: proposed fix, pushed to try

Comment 27

[Brendan Eich [:brendan]]

http://hg.mozilla.org/try/rev/8735d09b21c4 looks good to my eyes -- unstarred win dbg reftest is Number.toLocaleString known badness, right?

/be

Comment 28

[Phil Ringnalda (:philor)]

http://tbpl.mozilla.org/?tree=MozillaTry&rev=8735d09b21c4 looks more than good, it looks jaw-droppingly green. Pushed again with "try: -a" in the commit message and rebased with the .toLocaleString fix in http://tbpl.mozilla.org/?tree=MozillaTry&rev=f9d2a25708b8 since without -a or explicitly asking for it you don't get Talos, and other than a couple of busted slaves it's happy too.

Comment 29

[Brendan Eich [:brendan]]

http://hg.mozilla.org/try/rev/dcc047f8e36e per comment 28.

/be

Comment 31

[Sheila Mooney]

What were the results from the tryserver run? Is it ready to check in?

Comment 32

[Phil Ringnalda (:philor)]

Fine, pretty much the same as when I had already done it, since I was saying "I did" rather than "you should." But it's not ready to check in since it hasn't yet been reviewed.

Comment 33

[Brendan Eich [:brendan]]

Phil, ever seen anything like this?

s: talos-r3-snow-021
TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-290575.js | application timed out after 330 seconds with no output

from my last push to try. Will try to reproduce.

/be

Comment 34

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 36

[LegNeato]

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

Comment 37

[Phil Ringnalda (:philor)]

No, I haven't ever seen regress-290575.js time out (including during the other two times you and I pushed it to try), though bug 613551 did manage to break it today, in a way I didn't understand.

Comment 38

[Sheila Mooney]

I don't know that this characterizes as an unshippable trunk BUT as I understand it our goal for each beta is NOT to regress on crashes. This one is responsible for a bunch of signatures. Any chance we can get it in for beta9?

Comment 39

[Brendan Eich [:brendan]]

I'm waiting for code review. Jason has been sick but I saw mail from him today.

This absolutely should block beta9, time-based release is not an absolute.

/be

Comment 40

[Jason Orendorff [:jorendorff]]

In jsfun.cpp, GetCallUpvar:
>+    /*
>+     * Our callee must be a flat closure. Any other kind of upvar-referencing
>+     * function will use JSOP_GETUPVAR or JSOP_CALLUPVAR, or else won't ever
>+     * have upvar shapes in its compiler-created shape lineage which is shared
>+     * by all Call objects created for activations of the function.
>+     */
>     *vp = obj->getCallObjCallee().getFlatClosureUpvar(i);
>     return true;
> }

To expand the proof a bit:

 1. We only use the GetCallUpvar getter on Call objects for functions that
    qualify for GETUPVAR/CALLUPVAR access to the referenced
    arguments/variables. (In particular, obj->getCallObjCallee() is
    non-escaping w.r.t. the scope of the arg/var.)

 2. These properties are only ever got directly, via bytecode instructions.
    (This assumes the debugger doesn't try to grab arbitrary properties off the
    scope object. I don't remember it being that well-behaved; is it?)

 3. Furthermore, if we add a binding for an upvar, we get the upvar exclusively
    via JSOP_GETUPVAR and CALLUPVAR, except in flat closures. We never emit
    unspecialized NAMEish opcodes that might call the getter. (What about
    INCNAME? What about JS_EvaluateInStackFrame code that doesn't qualify for
    the upvar optimization?)

If 1, 2, and 3 were true, we could conclude that the callee here is a flat
closure. At least INCNAME seems like a hole.

In js_NewFlatClosure:
>+     * FIXME: bug 545759 proposes to enable partial flat closures. Fixing this
>+     * bug requires a GetScopeChainFast call here, along with JS_REQUIRES_STACK
>+     * annotations on this function's prototype and definition.
>      */
>-    JSObject *scopeChain = GetScopeChainFast(cx, cx->fp(), op, oplen);
>-    if (!scopeChain)
>-        return NULL;
>+    JSObject *scopeChain = &cx->fp()->scopeChain();

The call to cx->fp() would run afoul of the static analysis. In this case, you
don't care exactly which stack frame you get, as long as the right global
object is at the end of the scope chain, right? In that case, add:
    VOUCH_DOES_NOT_REQUIRE_STACK();
before that line.

In class Bindings:
>+     * before before any VARIABLE or CONSTANT bindings, which themselves must

"before" is doubled.

Comment 41

[Brendan Eich [:brendan]]

Attachment: remove JSOP_{GET,CALL}UPVAR and code for those ops

That lightened the load!

Trying to test, need orange to go away and then to rebase and push to try. But please feel free to review, this simplifies away the whole problem, and removes a lot of code including my nemesis from the early tofte wars, MakeUpvarForEval.

/be

Comment 42

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function() {
    let(x) function() {
        let(a = x) function() {
            x = x
        } ()
    } ()
} ())

asserts js debug shell at Assertion failure: fun->isFlatClosure(), and crashes opt shell at js::SetFlatUpvar. I haven't tested this against the latest patch, though.

Comment 43

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment 42 occurred on TM changeset ca11457ed5fe  without -m nor -j.

This is another testcase showing similar issues as comment 42:

let(b)((function() {
  let([] = <x/>, x = b, d = b = "")(function() {})
})())

Comment 44

[Brendan Eich [:brendan]]

Gary, there are tons of testcase variations on a theme. No need to put more in this bug, especially when the patch fixes them all.

/be

Comment 45

[Sheila Mooney]

Does that mean we are done? Can we wrap up this one today before the beta9 freeze?

Comment 46

[Brendan Eich [:brendan]]

Morning push to try at 932a856611fb is looking good (I used try: -a), save for the patch exposing two latent bugs in tests:

js1_8_1/extensions/regress-452498-196.js
js1_8_1/regress/regress-452498-119.js

Both relied on eval'ed const redeclaring a local var, naughty. Fixing those to use var in the eval.

/be

Comment 47

[Jesse Ruderman]

"Relied on"? They were tests for assertions and crashes, so replacing "const" with "var" could change what they test.

Please add a new test covering the intentional behavior that affects the old version of those tests.

Comment 48

[Brendan Eich [:brendan]]

I get a clean js/src/tests run in my debug shell with this patch, clean in that only these tests fail:

js1_5/extensions/regress-336409-1.js
js1_6/extensions/regress-456826.js

I think these are known failures. Wish I could be sure without unpatching and rerunning (and hoping they aren't intermittent).

/be

Comment 49

[Brendan Eich [:brendan]]

(In reply to comment #47)
> "Relied on"? They were tests for assertions and crashes, so replacing "const"
> with "var" could change what they test.

The tests say what they were checking, but they cannot pass now as written.

> Please add a new test covering the intentional behavior that affects the old
> version of those tests.

The old versions throw SyntaxErrors. Wrapping the evals in try/catch could work but would change the generated bytecode more fundamentally (catch makes a block scope). Changing to var and avoiding try/catch preserves what these were about.

/be

Comment 50

[Brendan Eich [:brendan]]

(In reply to comment #49)
> (In reply to comment #47)
> > "Relied on"? They were tests for assertions and crashes, so replacing "const"
> > with "var" could change what they test.
> 
> The tests say what they were checking,

IOW, I read the tests and made the right changes. But thanks for the doubt! :-/

/be

Comment 51

[Brendan Eich [:brendan]]

Attachment: proposed fix, rebased and with tests fixed

Comment 52

[Jason Orendorff [:jorendorff]]

Comment on attachment 502134 [details] [diff] [review]
proposed fix, rebased and with tests fixed

In comment 40 I wrote a bit about JS_REQUIRES_STACK. That applies to this patch too.

Also I'd like another follow-up bug to look at the `if (!fn->isFunArg())` block in Parser::setFunctionKinds. I bet those two blocks of code (the if block and the else block) can be combined into something shorter and saner.

>                     if (!lexdep->isFreeVar()) {
>                         JS_ASSERT(lexdep->frameLevel() <= funbox->level);
>                         ++nupvars;
>-                        if (lexdep->isAssigned())
>-                            break;
>                     }

I think this could just say `break;`. The purpose of the loop is to find out if nupvars is zero or not; at this point we know it's not.

r=me.

Comment 53

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #48)
> I get a clean js/src/tests run in my debug shell with this patch, clean in that
> only these tests fail:
> 
> js1_5/extensions/regress-336409-1.js
> js1_6/extensions/regress-456826.js
> 
> I think these are known failures. Wish I could be sure without unpatching and
> rerunning (and hoping they aren't intermittent).

336409 is known to suck, as reported in 618337
456826 is believed to be invalid, per bug 594667.

We should lose both those tests, IMO, but I'll take it to the bugs in question.

Comment 54

[Brendan Eich [:brendan]]

(In reply to comment #52)
> Comment on attachment 502134 [details] [diff] [review]
> proposed fix, rebased and with tests fixed
> 
> In comment 40 I wrote a bit about JS_REQUIRES_STACK. That applies to this patch
> too.

We lost tinderbox coverage of this static analysis, which means it has rotted. Can we get that back? Or 'make check' on Linux with uprev gcc does it for you?

/be

Comment 55

[Brendan Eich [:brendan]]

(In reply to comment #52)
> I'd like another follow-up bug to look at the `if (!fn->isFunArg())` block
> in Parser::setFunctionKinds. I bet those two blocks of code (the if block and
> the else block) can be combined into something shorter and saner.

As mentioned on IRC, that condition is a stricter one than the condition we need now: that the function isn't induced by a hoisted declaration form. We cannot flatten non-lambdas.

> >                     if (!lexdep->isFreeVar()) {
> >                         JS_ASSERT(lexdep->frameLevel() <= funbox->level);
> >                         ++nupvars;
> >-                        if (lexdep->isAssigned())
> >-                            break;
> >                     }
> 
> I think this could just say `break;`. The purpose of the loop is to find out if
> nupvars is zero or not; at this point we know it's not.

Right, fixing.

> r=me.

Thanks!

/be

Comment 56

[Brendan Eich [:brendan]]

Attachment: fix I just pushed

Comment 57

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/a213cb8ca396

/be

Comment 58

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/a213cb8ca396

Comment 59

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 60

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-592202-3.js.