771039 - assert some invariants in BindNameToSlot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch (applies to cset 07b1a5999430)

The patch adds a couple asserts/comments that (I think) hold and are needed for bug 753158.  Some fuzzing would be appreciated and increase confidence.

Comment 1

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

Comment on attachment 639215 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

decoder might also be able to help.

Comment 2

[Luke Wagner [:luke]]

woohoo, so far green on try.

Comment 3

[Christian Holler (:decoder)]

I'm on this now :)

Comment 4

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

Comment on attachment 639215 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

Green for me after a night of overnight fuzzing on Mac 10.7 as well.

Comment 5

[Luke Wagner [:luke]]

Attachment: patch (applies to cset 07b1a5999430)

Another key invariant (different static level implies isClosed) and a few more syntactic cleanups.

Comment 6

[Luke Wagner [:luke]]

Attachment: patch (applies to cset 07b1a5999430)

... and empowered by these assertions, I can see that this atomIndex-abusing abomination is dead.

Comment 7

[Christian Holler (:decoder)]

Not going to let you get away so easily ^^ (run with -n -m):


const cases = [{ expr: "((function() arguments) for (x in []))" }];
function expectError1(err, ctx, msg) {}
function expectError(expr, call, wrapCtx, expect, msg) {
    expectError1(eval(wrapCtx(call)), exps.call, 'call argument in ' + msg);
}
function atTop(str) { return str; }
  for (let i = 0, len = cases.length; i < len; i++) {
    let {expr, top, fun, gen, desc} = cases[i];
    let call = (expr[0] === "(") ? ("print" + expr) : null;
      expectError(null, call, atTop);
}


Assertion failure: dn->isClosed(), at js/src/frontend/BytecodeEmitter.cpp:1274

Comment 8

[Luke Wagner [:luke]]

Which boils down to: ((function() arguments) for (x in []));
Mmm, 'arguments' and generator expressions, what could go wrong?

Comment 9

[Luke Wagner [:luke]]

Attachment: fix genexpr transplanting

Ah ha, this is exactly what I was hoping to flush out.  There is a subtle interaction whereby implicit arguments gets bound at one static level, and then later, when we realize we are in a generator expression, we try to fix up all the parse nodes.  However, since there was no explicit definition of the arguments, CompExprTransplanter::transplant doesn't update the static level of the implicit arguments.  This the fix is pretty simple, but regrettably adds a PND flag.

Comment 10

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies to cset 007003bb82c9)

Comment 11

[Christian Holler (:decoder)]

Comment on attachment 639872 [details] [diff] [review]
combined patch for fuzzing (applies to cset 007003bb82c9)

No further bugs found so far, feedback+.

Comment 12

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

Comment on attachment 639871 [details] [diff] [review]
fix genexpr transplanting

Review of attachment 639871 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +5102,5 @@
> +                    if (genexp && !visitedImplicitArguments.has(dn)) {
> +                        if (!BumpStaticLevel(dn, tc))
> +                            return false;
> +                        AdjustBlockId(dn, adjust, tc);
> +                        if (!visitedImplicitArguments.put(dn))

Out of curiosity, would it make sense to just strip the PND_IMPLICITARGUMENTS flag so you don't visit it again?

Comment 13

[Luke Wagner [:luke]]

(In reply to David Anderson [:dvander] from comment #12)
Ah, yes, that would work.  The reason I'd prefer the clunkier HashSet is that if we unset the bit, it would nuance the official meaning of PND_IMPLICITARGUMENTS from "is an implicitly-declared arguments" to "is a bit set that CompExprTransplanter uses to x y z..." which, in some sense, let's CompExprTransplanter spread it's grossness.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41d5c8529748
https://hg.mozilla.org/integration/mozilla-inbound/rev/0253f34f6bc2

Comment 15

[Ed Morley [:emorley]]

Unfortunately bug 772303 (https://hg.mozilla.org/integration/mozilla-inbound/rev/fad7d06d7dd5) had to be backed out for winxp pgo-only jsreftest failures.

This bug (amongst others) either caused unresolvable (for someone who doesn't work on the JS engine at least) conflicts with the backout of fad7d06d7dd5, or else bugzilla dependencies indicated that one of it's dependants had now been backed out. 

Since there was no one in #developers that could resolve the conflicts, unfortunately this bug has been backed out:

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

Comment 16

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15409e3b3bdc

Comment 17

[Luke Wagner [:luke]]

oops, and:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b790407d394f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15409e3b3bdc
https://hg.mozilla.org/mozilla-central/rev/b790407d394f