Last Comment Bug 771039 - assert some invariants in BindNameToSlot
: assert some invariants in BindNameToSlot
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 773927
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-04 21:50 PDT by Luke Wagner [:luke]
Modified: 2012-07-14 01:00 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (applies to cset 07b1a5999430) (6.74 KB, patch)
2012-07-04 21:50 PDT, Luke Wagner [:luke]
gary: feedback+
Details | Diff | Splinter Review
patch (applies to cset 07b1a5999430) (10.77 KB, patch)
2012-07-05 16:21 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (applies to cset 07b1a5999430) (11.90 KB, patch)
2012-07-05 20:32 PDT, Luke Wagner [:luke]
dvander: review+
Details | Diff | Splinter Review
fix genexpr transplanting (7.49 KB, patch)
2012-07-06 17:57 PDT, Luke Wagner [:luke]
dvander: review+
Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset 007003bb82c9) (19.25 KB, patch)
2012-07-06 17:58 PDT, Luke Wagner [:luke]
choller: feedback+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-04 21:50:48 PDT
Created attachment 639215 [details] [diff] [review]
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] 2012-07-04 21:52:38 PDT
Comment on attachment 639215 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

decoder might also be able to help.
Comment 2 Luke Wagner [:luke] 2012-07-04 23:15:15 PDT
woohoo, so far green on try.
Comment 3 Christian Holler (:decoder) 2012-07-05 06:29:47 PDT
I'm on this now :)
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2012-07-05 09:31:17 PDT
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] 2012-07-05 16:21:28 PDT
Created attachment 639507 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

Another key invariant (different static level implies isClosed) and a few more syntactic cleanups.
Comment 6 Luke Wagner [:luke] 2012-07-05 20:32:57 PDT
Created attachment 639568 [details] [diff] [review]
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) 2012-07-06 07:28:09 PDT
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] 2012-07-06 12:15:59 PDT
Which boils down to: ((function() arguments) for (x in []));
Mmm, 'arguments' and generator expressions, what could go wrong?
Comment 9 Luke Wagner [:luke] 2012-07-06 17:57:42 PDT
Created attachment 639871 [details] [diff] [review]
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] 2012-07-06 17:58:53 PDT
Created attachment 639872 [details] [diff] [review]
combined patch for fuzzing (applies to cset 007003bb82c9)
Comment 11 Christian Holler (:decoder) 2012-07-09 05:36:01 PDT
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] 2012-07-11 17:27:42 PDT
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] 2012-07-11 18:29:13 PDT
(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 15 Ed Morley [:emorley] 2012-07-12 05:24:32 PDT
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 17 Luke Wagner [:luke] 2012-07-12 10:58:52 PDT
oops, and:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b790407d394f

Note You need to log in before you can comment on or make changes to this bug.