772328 - function statements shouldn't shadow formals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

IIUC, this should return 42:

  function f(x) {
    function x() {}
    arguments[0] = 42;
    return x
  }

The reason is that Parser::functionDef calls addVariable if there is an existing formal binding.

Note: this offers a new opportunity for (undetected) shapes with duplicate ids.

Comment 1

[Luke Wagner [:luke]]

(dmandelin needs [:dmandelin])

Comment 2

[David Mandelin [:dmandelin]]

(In reply to Luke Wagner [:luke] from comment #1)
> (dmandelin needs [:dmandelin])

Heh, I never put one in because I figured my name chars were unique enough, so I was always waiting for someone to ask. :-)

Comment 3

[Nicholas Nethercote [inactive]]

> (dmandelin needs [:dmandelin])

Hear hear!  This has caught me lots of times;  I reflexively put a ':' in front of everybody's names.

Comment 4

[Luke Wagner [:luke]]

In addition to being wrong, the function-shadowing-formals gets in the way of bug 775323.

Comment 5

[Luke Wagner [:luke]]

Attachment: simplify MakeDefIntoUse

As explained in the comment added by this patch, MakeDefIntoUse is called when a function statement redeclares another function.  Currently we handle this by turning the former into a name that uses the latter.  We then hack the ParseNode's op to be JSOP_NOP which is a signal to the emitter not to do anything.  This causes trouble for a later patch.

Comment 6

[Luke Wagner [:luke]]

Attachment: add IsArgOp and IsLocalOp (patch 2)

I'm tired of forgetting whether it's JOF_OPMODE or JOF_OPTYPE...

Comment 7

[Luke Wagner [:luke]]

Attachment: fix and test (patch 3)

This patch fixes the bug by making the op of the PNK_FUNC node tell whether the function is stored in an arg or local.  This patch also cleans up the logic in Parser::functionDef to be more understandable and conducive to the changes coming in bug 775323 (viz., avoiding the querying of bindings).

Comment 8

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies to cset 7ad3878167c1)

These patches assert and depend on several new frontend invariants and change some core frontend bits.  If you guys have time, do you suppose you could point the fuzzers at this patch and see if they find anything?

Comment 9

[Christian Holler (:decoder)]

Comment on attachment 645942 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ad3878167c1)

function F(x, y) {}
function test(N) {
  var src = F.toSource(-1)+"\n";
  src = Array(N + 1).join(src);
  eval(src);
}
for (var i = 0; i != 10; ++i) {
  var time = test(1000 + i * 2000);
}


asserts with

Assertion failure: pnu->isUsed(), at js/src/frontend/Parser.cpp:878

Comment 10

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

Comment on attachment 645942 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ad3878167c1)

I could spare a few mins for this - nothing severe immediately found on my side.

Comment 11

[Luke Wagner [:luke]]

Attachment: simplify MakeDefIntoUse (patch 1)

Phew, thanks!  Here is the fixed patch.

Comment 12

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies to cset 462036803a34)

If you have any more time (no rush), it'd be great to see if this patch holds up

Comment 13

[Christian Holler (:decoder)]

Comment on attachment 645989 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)

(function (call) { 
  function call() { } 
}).abstract.f++;


asserts with:

Assertion failure: *nextpc == JSOP_SETLOCAL || *nextpc == JSOP_SETALIASEDVAR, at js/src/jsopcode.cpp:4796

Comment 14

[Luke Wagner [:luke]]

*shakes fist at decompiler; hasn't Benjamin removed that yet?

Comment 15

[Luke Wagner [:luke]]

Ah, this is just an assertion fix.  decoder, was this the only failure?  If this is fixed, do you think the patch passes fuzzing or did fuzzing not run long before finding this?

Comment 16

[Luke Wagner [:luke]]

Attachment: fix and test (patch 3)

Comment 17

[Luke Wagner [:luke]]

Attachment: combined for fuzzing (applies to cset 462036803a34)

Comment 18

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

Comment on attachment 646255 [details] [diff] [review]
combined for fuzzing (applies to cset 462036803a34)

(function() {
  var x = (function() {})
  function x() {}
  function x() {}
})()

Assertion failure: pnu->isUsed(),

Comment 19

[Luke Wagner [:luke]]

Attachment: simplify MakeDefIntoUse (patch 1)

Phew, painful.  Mutating parse node trees in place is not a good idea and fraught with peril.  Thanks Gary and Christian!

Comment 20

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies to cset 462036803a34)

Comment 21

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

Comment on attachment 646369 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)

A night of fuzzing on my MBP did not throw up anything weird. \o/

Comment 22

[Luke Wagner [:luke]]

Phew, thanks again!

Comment 23

[Luke Wagner [:luke]]

Attachment: simplify ParseNode::resolve (patch 4)

A nice side-effect of the MakeDefIntoUse change is that we don't have this unbounded chain of definitions-converted-into-assignments which allows a nice simplification of ParseNode::resolve.

Comment 24

[Eddy Bruel [:ejpbruel]]

Comment on attachment 646235 [details] [diff] [review]
fix and test (patch 3)

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

This patch looks alright to me, but not 100% sure I didn't miss anything. Double checked some stuff with jorendorff over irc to make sure. Should be ok to r+, since you ran this stuff through the fuzzer.

Comment 25

[Eddy Bruel [:ejpbruel]]

Comment on attachment 646364 [details] [diff] [review]
simplify MakeDefIntoUse (patch 1)

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

I got nothing

Comment 26

[Eddy Bruel [:ejpbruel]]

Comment on attachment 649856 [details] [diff] [review]
simplify ParseNode::resolve (patch 4)

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

Looks good. Glad to see this being simplified. Thanks Luke!

Comment 27

[Luke Wagner [:luke]]

Thank you

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb94aecac98f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8153d805296
https://hg.mozilla.org/integration/mozilla-inbound/rev/441cf1a50af8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea43af33ec9

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb94aecac98f
https://hg.mozilla.org/mozilla-central/rev/e8153d805296
https://hg.mozilla.org/mozilla-central/rev/441cf1a50af8
https://hg.mozilla.org/mozilla-central/rev/5ea43af33ec9

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 646235 [details] [diff] [review]
fix and test (patch 3)

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1202,5 @@
>  static bool
>  BindNameToSlot(JSContext *cx, BytecodeEmitter *bce, ParseNode *pn)
>  {
>      JS_ASSERT(pn->isKind(PNK_NAME));
> +    JS_ASSERT_IF(pn->isKind(PNK_FUNCTION), pn->isBound());

Can pn->isKind(PNK_FUNCTION) be true if the assert above this passes?

Comment 30

[Luke Wagner [:luke]]

Good catch!  That is a left-over from earlier iterations of the patch.  I'll remove that in bug 775323.