591437 - Reflect.parse(): can't reliably use pn_cookie for function args

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Herman [:dherman]]

js> Reflect.parse("function f(a) { function a() { } }")
Assertion failure: !isFree(), at ../../jsscript.h:105
Segmentation fault

Comment 1

[Dave Herman [:dherman]]

Attachment: don't check frameSlot() on formal args, only on destructuring args

Fix attached.

Dave

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 469988 [details] [diff] [review]
don't check frameSlot() on formal args, only on destructuring args

>From: Dave Herman <[e-mail]>
>
>bug 591437, r=?: can't reliably use pn_cookie for function args
>
>diff --git a/js/src/jsreflect.cpp b/js/src/jsreflect.cpp
>--- a/js/src/jsreflect.cpp
>+++ b/js/src/jsreflect.cpp
>@@ -2712,26 +2712,37 @@ ASTSerializer::functionArgs(JSParseNode 
>      * Arguments are found in potentially two different places: 1) the
>      * argsbody sequence (which ends with the body node), or 2) a
>      * destructuring initialization at the beginning of the body. Loop
>      * |arg| through the argsbody and |destruct| through the initial
>      * destructuring assignments, stopping only when we've exhausted
>      * both.
>      */
>     while ((arg && arg != pnbody) || destruct) {
>-        if (arg && arg != pnbody && arg->frameSlot() == i) {
>+        if (destruct && destruct->pn_right->frameSlot() == i) {
>+            if (!pattern(destruct->pn_left, NULL, &node) ||
>+                !args.append(node))
>+                return false;

Nit, pre-existing: house style would put the if condition on one line, but if it didn't fit, then even though the "then" takes one line it would be braced.

>+            destruct = destruct->pn_next;
>+        } else if (arg && arg != pnbody) {
>+            /*
>+             * We don't check that arg->frameSlot() == i since we
>+             * can't call that method if the arg def has been turned
>+             * into a use, e.g.:
>+             *
>+             *     function(a) { function a() { } }
>+             *
>+             * There's no other way to ask a non-destructuring arg its
>+             * index in the formals list, so we rely on the ability to
>+             * ask destructuring args their index above.
>+             */
>             if (!identifier(arg, &node) ||
>                 !args.append(node))
>                 return false;

Same nit.

Looks good to me with nits picked, pre-emptive r+.

/be

Comment 3

[Dave Herman [:dherman]]

http://hg.mozilla.org/tracemonkey/rev/4f0f0d22e3c5

Comment 4

[Mike Shaver (:shaver emeritus)]

http://hg.mozilla.org/mozilla-central/rev/4f0f0d22e3c5