Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla2.0

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: gkw, Assigned: brendan)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla2.0
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta9+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [ccbr][sg:critical][hardblocker] fixed-in-tracemonkey, crash signature)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
(Reporter)

Comment 1

7 years ago
Probably related to bug 558451.

Regression window:

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=be9979b4c10b&tochange=f3e58c264932
Blocks: 558451
Is this a (near-)null deref? or just happens to be in this testcase?
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected

Updated

7 years ago
blocking2.0: ? → betaN+
(Assignee)

Updated

7 years ago
Assignee: general → brendan
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

7 years ago
Target Milestone: --- → mozilla2.0
Ping?
(Assignee)

Comment 4

7 years ago
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
(Reporter)

Comment 5

7 years ago
(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.
Whiteboard: [ccbr] → [ccbr][sg:dos]

Comment 6

7 years ago
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
Blocks: 532972

Comment 7

7 years ago
Created attachment 489814 [details]
semi reduced testcase

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

Comment 8

7 years ago
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.
Whiteboard: [ccbr][sg:dos] → [ccbr][sg:critical]

Comment 9

7 years ago
Created attachment 491788 [details]
jesse's gdb output
Brendan, any updates here yet?
(Reporter)

Comment 11

7 years ago
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
Summary: Crash [@ CallPropertyOp] or "Assertion failure: FUN_FLAT_CLOSURE(callee_fun)," → Crash [@ CallPropertyOp] or [@ js::SetFlatUpvar] or "Assertion failure: FUN_FLAT_CLOSURE(callee_fun),"
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.
bug 616762 looks unrelated, after all. The symptoms are very similar but the MakeUpvarForEval path isn't taken.
(Reporter)

Comment 14

7 years ago
This still occurs in changeset df86d5999fef.
(Reporter)

Comment 15

7 years ago
#6 and #9 on 4.0b9pre top crash list.
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.
blocking2.0: betaN+ → beta9+
Blocks: 618558
Blocks: 620099
(Assignee)

Updated

7 years ago
Duplicate of this bug: 621991
(Assignee)

Comment 18

7 years ago
Created attachment 500315 [details]
Jesse's most reduced testcase from bug 621991
(Assignee)

Comment 19

7 years ago
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
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 20

7 years ago
Created attachment 500317 [details] [diff] [review]
minimal fix with collected tests
Attachment #500317 - Flags: review?(jorendorff)
(Assignee)

Comment 21

7 years ago
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
(Assignee)

Updated

7 years ago
Duplicate of this bug: 620099
(Assignee)

Updated

7 years ago
No longer blocks: 618558
Duplicate of this bug: 618558
(Assignee)

Comment 24

7 years ago
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 <brendan@mozilla.org>
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
(Assignee)

Updated

7 years ago
Blocks: 494637
(Assignee)

Comment 25

7 years ago
> 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
(Assignee)

Comment 26

7 years ago
Created attachment 500493 [details] [diff] [review]
proposed fix, pushed to try
Attachment #500317 - Attachment is obsolete: true
Attachment #500493 - Flags: review?(jorendorff)
Attachment #500317 - Flags: review?(jorendorff)
(Assignee)

Comment 27

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

/be
(Assignee)

Updated

7 years ago
No longer blocks: 620099
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.
(Assignee)

Comment 29

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

/be
(Assignee)

Updated

7 years ago
Duplicate of this bug: 622041

Comment 31

7 years ago
What were the results from the tryserver run? Is it ready to check in?
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.
(Assignee)

Comment 33

7 years ago
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
Whiteboard: [ccbr][sg:critical] → [ccbr][sg:critical][hardblocker]

Comment 34

7 years ago
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)
No longer blocks: 349611, 494637, 532972, 558451
blocking2.0: beta9+ → betaN+
Keywords: assertion, crash, regression, testcase
(Assignee)

Updated

7 years ago
Duplicate of this bug: 621939

Comment 36

7 years ago
Fixing fields my automated script accidentally blanked. Apologies for the bugspam
Keywords: assertion, crash, regression, testcase

Updated

7 years ago
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

7 years ago
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?
(Assignee)

Comment 39

7 years ago
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
blocking2.0: betaN+ → beta9+
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.
Blocks: 618549
(Assignee)

Comment 41

7 years ago
Created attachment 501898 [details] [diff] [review]
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
Attachment #500493 - Attachment is obsolete: true
Attachment #501898 - Flags: review?(jorendorff)
Attachment #500493 - Flags: review?(jorendorff)
(Reporter)

Comment 42

7 years ago
(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.
Summary: Crash [@ CallPropertyOp] or [@ js::SetFlatUpvar] or "Assertion failure: FUN_FLAT_CLOSURE(callee_fun)," → Crash [@ CallPropertyOp] or [@ js::SetFlatUpvar] or "Assertion failure: FUN_FLAT_CLOSURE(callee_fun)," or "Assertion failure: fun->isFlatClosure(),"
(Reporter)

Comment 43

7 years ago
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() {})
})())
(Assignee)

Comment 44

7 years ago
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

7 years ago
Does that mean we are done? Can we wrap up this one today before the beta9 freeze?
(Assignee)

Comment 46

7 years ago
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

7 years ago
"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.
(Assignee)

Comment 48

7 years ago
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
(Assignee)

Comment 49

7 years ago
(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
(Assignee)

Comment 50

7 years ago
(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
(Assignee)

Comment 51

7 years ago
Created attachment 502134 [details] [diff] [review]
proposed fix, rebased and with tests fixed
Attachment #501898 - Attachment is obsolete: true
Attachment #502134 - Flags: review?(jorendorff)
Attachment #501898 - Flags: review?(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.
Attachment #502134 - Flags: review?(jorendorff) → review+
(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.
(Assignee)

Comment 54

7 years ago
(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
(Assignee)

Comment 55

7 years ago
(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
(Assignee)

Comment 56

7 years ago
Created attachment 502177 [details] [diff] [review]
fix I just pushed
Attachment #502134 - Attachment is obsolete: true
Attachment #502177 - Flags: review+
(Assignee)

Comment 57

7 years ago
http://hg.mozilla.org/tracemonkey/rev/a213cb8ca396

/be
Whiteboard: [ccbr][sg:critical][hardblocker] → [ccbr][sg:critical][hardblocker] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/a213cb8ca396
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Crash Signature: [@ CallPropertyOp] [@ js::SetFlatUpvar]
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Group: core-security
Crash Signature: [@ CallPropertyOp] [@ js::SetFlatUpvar] → [@ CallPropertyOp] [@ js::SetFlatUpvar]
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-592202-3.js.
Flags: in-testsuite+

Updated

2 years ago
Duplicate of this bug: 564390
You need to log in before you can comment on or make changes to this bug.