553778 - "Assertion failure: cg->fun->u.i.skipmin <= skip, at ../jsemit.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function () {
    var a = function () {
        (function () {
            a, x::e
        }[0])
        for (var [x] = [];;) {}
    }
}[0])


asserts js debug shell on TM tip without -j at Assertion failure: cg->fun->u.i.skipmin <= skip, at ../jsemit.cpp:2224

autoBisect shows this is probably related to bug 494235:

The first bad revision is:
changeset:   28920:c074f0f0ad2d
user:        Brendan Eich
date:        Thu Jun 04 18:58:47 2009 -0700
summary:     Bug 494235: wrap escaping optimized closures for the debugger API (r=igor/mrbkap).

Comment 1

[Daniel Veditz [:dveditz]]

Gary: why does this bug potentially block a branch release?

Comment 2

[Daniel Veditz [:dveditz]]

marking affected branches for now, re-nom for blocking if there's compelling reasons that it fits the stable branch criteria

Comment 3

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

(In reply to comment #1)
> Gary: why does this bug potentially block a branch release?

I have no idea if this assertion is bad, also, it seems to have regressed since June 2009. I guess I should have nominated first for 1.9.3.

Comment 4

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

<nth12sd> brendan: is bug 553778 worth taking a look?
<brendan> nth12sd: jorendorff can fix it i bet

Comment 5

[Jason Orendorff [:jorendorff]]

E4X is innocent. This asserts the same way:

function f() {
    function g() {
        function h() {
            g; x;
        }
        var [x] = [];
    }
}

In h, skipMin is 2, reflecting that at the time h was parsed, g was 2 frames up (in f) and x was apparently a global.  If I change the declaration on the 6th line to just `var x = [];` then it doesn't assert.

Comment 6

[Damon Sicore (:damons)]

Can we get an owner here, please?

Comment 7

[Jim Blandy :jimb]

Looking at this...

Comment 8

[Jim Blandy :jimb]

The problem here appears to be that destructuring var declarations don't correctly set the cookie:

'var x = []' produces a definition node for x whose upvar cookie is 2:1, whereas 'var [x] = []' produces a definition node for x whose upvar cookie is FREE_VALUE (completely incorrect).

Comment 9

[Jim Blandy :jimb]

FindFunArgs walks the tree of functions and scans each function's upvars (as recorded in the TOK_UPVARS node, if present) to compute the function's skipmin.

In the test case, when we see the first reference to x, we create a placeholder definition node for that to refer to. The placeholder goes in h's upvar table. When we see the declaration of x, we create a new definition node for that, steal the original definition node's uses, and update their lexdef links, as per standard practice. The original definition node is left with an empty use list; it does not refer to our new definition node at all, as there are cases where such placeholder nodes still have uses.

However, we leave the original placeholder definition in h's upvars table. Since we've done nothing to associate this with the new definition node, FindFunArgs doesn't recognize that its upvar has found a definition, and thus computes its skipmin value incorrectly.

Comment 10

[Jim Blandy :jimb]

So, I think I've gotten my head around what's going on in jsparse.cpp:Define, which we call to introduce a new JSDefinition node for some identifier. What's subtle about this function is not just that we may already have a placeholder definition node for our identifier, with a chain of use nodes, but that our new definition may steal none, some portion, or all of that use list, as its own.

Here, the call to Define for the 'let' will steal none of the uses from the var node's use list:

function f() { var x = 3; x; { let x = 4; } return x; }

Here, the call to Define for the 'let' will steal one use off the var node's use list: when we see the 'let', we realize that the second reference to x actually doesn't belong to the 'var' at all:

function f() { var x = 3; x; { x = 4; let x; } return x; }

Here, the call to Define will steal all the uses from the placeholder:

function f() { function g() { x; } let [x] = [1]; }

The nice solution to the problem described in comment 9 would be to simply transform the placeholder node, listed in the inner functions' TOK_UPVARS nodes, into a use of the capturing definition; then the 'resolve' call in FindFunArgs will find the right definition, and compute the function's skipmin correctly.

But that would not be correct if those placeholder node can have *some* uses that the new definition should capture, and others that it must not. Can the placeholder merging done in LeaveFunction construct such use lists?

Here is an example to try in the morning:

function f() { x; { x; function g() { x; } let x; } }

I believe that 1) the first and second uses of x will be on the same placeholder's use list, 2) the lexdep from g will be merged with that placeholder, but then 3) the 'let' will steal the second and third references, but not the first. Thus, the placeholder in g's TOK_UPVAR node cannot be transformed into a use, because that would incorrectly turn the first use of x into a use of the 'let'-bound x.

Comment 11

[Jim Blandy :jimb]

Attachment: Don't orphan placeholder definition nodes when a real definition is found.

Comment 12

[Jim Blandy :jimb]

I think that process of "turn this def into a use of this new def" should be abstracted out into a function (even though we already have a MakeUseIntoDef, which does something else). But I'd appreciate a review just to see if I'm thinking about this in a reasonable way.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> I think that process of "turn this def into a use of this new def" should be
> abstracted out into a function (even though we already have a MakeUseIntoDef,

You mean MakeDefIntoUse?

> which does something else

MakeDefIntoUse does what it said, with a complication but it's not doing something else, AFAIK. What do you mean?

> But I'd appreciate a review just to see if I'm thinking about this in a
> reasonable way.

Looking now.

/be

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #10)
> So, I think I've gotten my head around what's going on in jsparse.cpp:Define,
> which we call to introduce a new JSDefinition node for some identifier. What's
> subtle about this function is not just that we may already have a placeholder
> definition node for our identifier, with a chain of use nodes, but that our new
> definition may steal none, some portion, or all of that use list, as its own.

The joys of hoisting.

> Here, the call to Define for the 'let' will steal one use off the var node's
> use list: when we see the 'let', we realize that the second reference to x
> actually doesn't belong to the 'var' at all:
> 
> function f() { var x = 3; x; { x = 4; let x; } return x; }

BTW, we resolved without yet spec'ing it even informally to change let in Harmony to be like a declaration in C++, or like let* if you will, and not hoist (yay!). I convinced everyone this is what people expect and not var-like hoisting. So we should have a bug to implement this, and get on with it -- and break JS1.[78]* compatibility too.

> Here, the call to Define will steal all the uses from the placeholder:
> 
> function f() { function g() { x; } let [x] = [1]; }
> 
> The nice solution to the problem described in comment 9 would be to simply
> transform the placeholder node, listed in the inner functions' TOK_UPVARS
> nodes, into a use of the capturing definition; then the 'resolve' call in
> FindFunArgs will find the right definition, and compute the function's skipmin
> correctly.
> 
> But that would not be correct if those placeholder node can have *some* uses
> that the new definition should capture, and others that it must not.

Right, that is why the code instead steals uses. But we shouldn't leave an empty placeholder in an exited function's upvars -- this is the bug.

> Can the
> placeholder merging done in LeaveFunction construct such use lists?
> 
> Here is an example to try in the morning:
> 
> function f() { x; { x; function g() { x; } let x; } }
> 
> I believe that 1) the first and second uses of x will be on the same
> placeholder's use list, 2) the lexdep from g will be merged with that
> placeholder, but then 3) the 'let' will steal the second and third references,
> but not the first.

Right.

> Thus, the placeholder in g's TOK_UPVAR node cannot be
> transformed into a use, because that would incorrectly turn the first use of x
> into a use of the 'let'-bound x.

That's right, but if an exited function has an upvar in its TOK_UPVARS set that is a useless placeholder (whew!), then we hit this bug. One fix would be to cope in LeaveFunction. Looks like you attacked the problem in Define. More in a bit, on the patch.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 487722 [details] [diff] [review]
Don't orphan placeholder definition nodes when a real definition is found.

>Bug 553778: Don't orphan placeholder definition nodes when a real definition is found.
>
>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp
>+++ b/js/src/jsparse.cpp
>@@ -1467,8 +1467,33 @@ Define(JSParseNode *pn, JSAtom *atom, JS
>                 pn->dn_uses = dn->dn_uses;
>                 dn->dn_uses = pnu;
> 
>-                if ((!pnu || pnu->pn_blockid < tc->bodyid) && list != &tc->decls)
>-                    list->rawRemove(tc->parser, ale, hep);
>+                /*
>+                 * If we followed the use list all the way back to tc's outermost block,
>+                 * then we must remove it from its list: either we have satisfied all
>+                 * references to that identifier, so it is no longer a lexdep, or
>+                 * we are about to replace the decls entry with our new definition.
>+                 */
>+                if (!pnu || pnu->pn_blockid < tc->bodyid) {
>+                    /* We needn't bother deleting a tc->decls entry; we'll overwrite it below. */
>+                    if (list != &tc->decls) {
>+                        list->rawRemove(tc->parser, ale, hep);

Is the if(||)-if instead of if((||)&&) better? It's more indented :-|.

>+                        /*
>+                         * If we've taken all the definitions from dn, then we must

"all the uses from dn"

>+                         * transform it into a use of our new definition pn: dn may appear
>+                         * in inner functions' TOK_UPVARS tables, and those should resolve
>+                         * to our definition.
>+                         */
>+                        if (!pnu) {
>+                            dn->pn_link = pn->dn_uses;
>+                            pn->dn_uses = dn;
>+                            dn->pn_dflags &= ~PND_PLACEHOLDER;
>+                            pn->pn_dflags |= dn->pn_dflags & PND_USE2DEF_FLAGS;
>+                            dn->pn_defn = false;
>+                            dn->pn_used = true;
>+                            dn->pn_lexdef = (JSDefinition *) pn;

You're right that there is a more primitive MakeDefIntoUse to common out here. I still wonder about making the fix in LeaveFunction instead. Would that be even simpler? It would leave a useless placeholder around for a while.

/be

Comment 16

[Jim Blandy :jimb]

(In reply to comment #15)
> >+                if (!pnu || pnu->pn_blockid < tc->bodyid) {
> >+                    /* We needn't bother deleting a tc->decls entry; we'll overwrite it below. */
> >+                    if (list != &tc->decls) {
> >+                        list->rawRemove(tc->parser, ale, hep);
> 
> Is the if(||)-if instead of if((||)&&) better? It's more indented :-|.

Uh-oh, a ':-|' from Brendan --- that means he's pretty upset!! :)

I could not understand that test until I realized that it was just an optimization --- it's always appropriate to remove the entry, but in the decls case we're going to add it right back. I like it better this way, but I'd be happy enough with a good comment.

> >+                         * If we've taken all the definitions from dn, then we must
> 
> "all the uses from dn"

Gaack.

> You're right that there is a more primitive MakeDefIntoUse to common out here.
> I still wonder about making the fix in LeaveFunction instead. Would that be
> even simpler? It would leave a useless placeholder around for a while.

That could well be --- and seems more "right".  I'll give that a shot tomorrow.

I also have a bunch more test cases to add that helped me understand why the code must be as it is.

Comment 17

[Jim Blandy :jimb]

(In reply to comment #14)
> BTW, we resolved without yet spec'ing it even informally to change let in
> Harmony to be like a declaration in C++, or like let* if you will, and not
> hoist (yay!). I convinced everyone this is what people expect and not var-like
> hoisting. So we should have a bug to implement this, and get on with it -- and
> break JS1.[78]* compatibility too.

That sounds great to me. The behavior in the example is pretty surprising.

> Right, that is why the code instead steals uses. But we shouldn't leave an
> empty placeholder in an exited function's upvars -- this is the bug.

Yep, as stated in comment 9.

Comment 18

[Jim Blandy :jimb]

(In reply to comment #14)
> BTW, we resolved without yet spec'ing it even informally to change let in
> Harmony to be like a declaration in C++, or like let* if you will, and not
> hoist (yay!). I convinced everyone this is what people expect and not var-like
> hoisting. So we should have a bug to implement this, and get on with it -- and
> break JS1.[78]* compatibility too.

Filed as bug 609205.

Comment 19

[Jim Blandy :jimb]

Attachment: Don't orphan placeholder definition nodes when a real definition is found.

Okay, this is revised to do the deed in LeaveFunction. I think this is much nicer; please let me know what you think.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 488007 [details] [diff] [review]
Don't orphan placeholder definition nodes when a real definition is found.

Great, nice attention to detail. Commoning the pn_type and pn_op init wins in saved lines of code and less distributed knowledge. The runt-else for the old if (outer_ale) if going away thanks to MakePlaceholder doing lexdep.add, etc., ditto.

/be

Comment 21

[Jim Blandy :jimb]

Below from the Try server. This seems sufficiently unsimilar to the other intermittent failures in this test (31 assertions?) that I'm going to look into it.

/tests/dom/tests/mochitest/ajax/scriptaculous/test_Scriptaculous.html | testInPlaceEditor - 31 assertions, 2 failures, 0 errors

Comment 22

[Jim Blandy :jimb]

Scriptaculous failures seemed spurious; landed.
http://hg.mozilla.org/tracemonkey/rev/8c59a5bf187b

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8c59a5bf187b

Comment 24

[Christian Holler (:decoder)]

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

Comment 25

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

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.