467495 - Crash [@ js_Interpret]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

Attachment: crash log

(function() { x = 0; function x() 4; var x; const y = 1; })();

crashes js shell (TM not necessary) at null at js_Interpret, both opt and debug. Looks fairly simple for it to jump to real-life so I nominate it for blocking-1.9.1.

Comment 1

[Brendan Eich [:brendan]]

Regression since fx3 -- please bisect and blame the cset that broke code generation here (we should be emitting SETLOCAL not SETCONST). Probably one of mine, but I have no time today or tomorrow to bisect.

/be

Comment 2

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

Many thanks to Jesse for helping to reduce this testcase as well. :)

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

This is fallout from bug 453133.

Comment 4

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

Another testcase with identical outcomes:

(function(){const y = 1; function x() '' let functional, x})()

Comment 5

[Igor Bukanov]

Attachment: v1

As a consequence of my patch for bug 453133 the TCF_FUN_CLOSURE_VS_VAR poison has spread to top-level function statements. It resulted in disabling slot binding "optimizations" in BindNameToSlot from js/src/jsemit.cpp when a function has a name clash between a variable and a top-level function. But these are not optimizations per see, for a lightweight function the binding is a must. In particular, the code relies on the fact that [setconst] would turn into [setlocal] which that patch disabled.

For the fix I simply remove TCF_FUN_CLOSURE_VS_VAR - AFAICS the name clash is harmless.

Comment 6

[Igor Bukanov]

Attachment: diff -b for v1

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 359070 [details] [diff] [review]
v1

TCF_FUN_CLOSURE_VS_VAR is necessary -- see bug 65308.

1.9.0 js shell session:

js> function f(x){var g;print(typeof g);if (x) function g(){}; print(g)}
js> f(0)
undefined
undefined
js> f(1)
undefined
function g() {
}

tracemonkey with this patch applied:

js> function f(x){var g;print(typeof g);if (x) function g(){}; print(g)}
js> f(0)
undefined
undefined
js> f(1)
undefined
undefined

>+        /*
>+         * We can't optimize if we are compiling a with statement and its
>+         * body, or we're in a catch block whose exception variable has the
>+         * same name as this node. FIXME: we should be able to optimize catch
>+         * vars to be block-locals.

This patch moves this comment, but the comment is already stale. For a new patch (I've fixed this in the patch for bug 452498) the ", or we're in a catch block ..." all the way to end of comment should be removed.

I'd love to get rid of function sub-statements, but not in 1.9.1.

/be

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> (From update of attachment 359070 [details] [diff] [review])
> TCF_FUN_CLOSURE_VS_VAR is necessary

Right, how can I forget that [deffun] uses OBJ_DEFINE_PROPERTY which *unconditionally* replaces the existing property, that is the one with the getter that stores g from the example in the frame's slot. I will try to come up tomorrow with a fix that properly deal with that.

Comment 9

[Brendan Eich [:brendan]]

Why not restore the not-top-level logic that set TCF_FUN_CLOSURE_VS_VAR? Minimal fix for function sub-statements seems better than anything that might slow down runtime for top-level function binding.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Minimal fix for function sub-statements seems better than anything that might
> slow down runtime for top-level function binding.

There is no need to touch [deffun] - the solution is to use [deflocalfun] for closures clashing with local variables.

Comment 11

[Igor Bukanov]

It turned 1.9.0 has a mild form of this bug. Consider:

function g() {
    if (1)
        function x() {};
    var x;
    const y = 1;
}

g();

Here both 1.9.0 and the trunk give:

TypeError: redeclaration of const y

Comment 12

[Igor Bukanov]

Here is a variation of the example from the comment 7 that shows yet another bug that exists on 1.9.0 and the trunk:

@watson~> cat ~/s/x.js
function g(x) { if (1) function x() {} return x; }
print(g(1));
@watson~> ~/m/30-ff/mozilla/js/src/Linux_All_DBG.OBJ/js ~/s/x.js
1

Comment 13

[Igor Bukanov]

Attachment: v2 (untested)

My initial plan to use [deflocalfun] for closures that clashes with local names does not work. Although it would fix the regression from the bug, it would not fix the issue from the comment 11. As [deflocalfun] can only store the function in a variable slot, it can not be used to deal with closure/argument name slashes.

So to deal with this bug and to address closure/argument clashes, the new patch does change [deffun]. But it optimizes for the common case when [deffun] introduces a new name in a variable object avoiding the call to js_CheckRedeclaration and extra locking/scope searches. After some testing I will ask for a review.

Comment 14

[Brendan Eich [:brendan]]

This patch and the patch for bug 452498 will conflict. I wonder if it is worth the trouble to keep closure vs. var "working" as before. Could we simply make all functions that contain function sub-statements heavyweight, therefore not emit JSOP_DEFLOCALFUN for such sub-statements?

/be

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> This patch and the patch for bug 452498 will conflict.

I am not sure about it - the coming fix will simply modify JSOP_DEFFUN to do special actions if the property exists (like keeping the native getters/setters for Call objects).

> Could we simply make all functions that contain function sub-statements heavyweight,

Although this will fix the original 1.9.1 regression leading to this bug, it would not help with other issues. A heavyweight function still uses getarg etc. bytecodes, so JSOP_DEFVAR still has to be fixed not use OBJ_DEFINE_PROPERTY for var and arg properties of the Call object as OBJ_DEFINE_PROPERTY will remove native getter/setters accessing stack frame slots.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (In reply to comment #14)
> > This patch and the patch for bug 452498 will conflict.
> 
> I am not sure about it - the coming fix will simply modify JSOP_DEFFUN to do
> special actions if the property exists (like keeping the native getters/setters
> for Call objects).

Yet we do not want JSOP_DEFFUN to do this when it is implementing a top-level function definition (top level in another function or program). There ES1-3+ and compatibility require blowing away any pre-existing property.

Since we are moving toward function sub-statements being legal only in blocks (directly) and binding block-scoped names, we should not overload JSOP_DEFFUN if we can help it. Also, time is short for 3.1 -- and this patch does indeed (and may inevitably, I'm not sure it's a problem, just a reason to minimize) conflict with the upvar patch.
 
> > Could we simply make all functions that contain function sub-statements heavyweight,
> 
> Although this will fix the original 1.9.1 regression leading to this bug, it
> would not help with other issues. A heavyweight function still uses getarg etc.
> bytecodes, so JSOP_DEFVAR still has to be fixed not use OBJ_DEFINE_PROPERTY for
> var and arg properties of the Call object as OBJ_DEFINE_PROPERTY will remove
> native getter/setters accessing stack frame slots.

JSOP_DEFVAR (always, again part of ES1 and predating it) defines only if the property does not already exist; otherwise it just checks. What am I missing?

I suggest minimizing fixing here to address the crash and (if possible) remove the TCF_FUN_CLOSURE_VS_VAR flag and logic.

/be

Comment 17

[Brendan Eich [:brendan]]

If we could remove function sub-statements, I would. We can't, not without breaking too much content that uses the narrow intersection of IE/Opera/Safar/Fx semantics. But we can certainly keep shipping bugs that were there for years. Not this crash bug, though ;-).

It would help the upvar patch to get rid of TCF_FUN_CLOSURE_VS_VAR.

/be

Comment 18

[Igor Bukanov]

This goes back to the pool.

Comment 19

[Igor Bukanov]

Attachment: v3

Here is the patch that removes TCF_FUN_CLOSURE_VS_VAR and makes sure that JSOP_DEFFUN preserves the native getter and setter for Call object properties and stores the closures in argument or variable slots of the current function frame.

In the patch I also added missing JSPROP_ENUMERATE to the set of attributes of Call object properties corresponding to arguments and variables. Although this is not observable in scripts, it is required by Ecma 262 and simplifies logic in JSOP_DEFUN.

Comment 20

[Igor Bukanov]

Here is a test case to check that non-top-level function statements overwrites existing argument and variables:

function f(x)
{
    var y = 1;
    if (Math)
        function x() { }
    if (Math)
        function y() { }
    return [x, y];
}

var r = f(0);
if (typeof(r[0]) != "function")
    throw "Bad";
if (typeof(r[1]) != "function")
    throw "Bad";

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 360530 [details] [diff] [review]
v3

r=me on the jsemit.{cpp,h} and jsparse.cpp changes -- wish those could land today, I'm almost ready with teh big upvar patch and I need those parts of this bug's patch.

>@@ -879,17 +879,17 @@ call_resolve(JSContext *cx, JSObject *ob
>         return JS_TRUE;
> 
>     if (!js_ValueToStringId(cx, idval, &id))
>         return JS_FALSE;
> 
>     localKind = js_LookupLocal(cx, fun, JSID_TO_ATOM(id), &slot);
>     if (localKind != JSLOCAL_NONE) {
>         JS_ASSERT((uint16) slot == slot);
>-        attrs = JSPROP_PERMANENT | JSPROP_SHARED;
>+        attrs = JSPROP_ENUMERATE | JSPROP_PERMANENT | JSPROP_SHARED;

Is this a necessary change? Only for the new test in JSOP_DEFFUN's interpreter case that you're adding (more about that below)?

In any event a comment would help. It may not hurt to add JSPROP_ENUMERATE -- it should not since Call objects are censored -- but it seems non-minimal.

>+          BEGIN_CASE(JSOP_DEFFUN) {

Nit: usual style is left brace on its own line, underhanging the B in BEGIN_CASE.

>+            JSBool doSet;

Could use bool, we're ready. If there's no API issue and we want truthy values to coerce to 1, then bool is better.

Could use cond instead of declaring an opcode-local boolean variable, but that's a little less clear due to the vague/generic name "cond".

>+            JSObject *pobj;
>+            JSProperty *prop;

Use obj2 and prop from js_Interpret top-level scope, no need for these.

>+            prop = NULL;
>+            ok = js_CheckRedeclaration(cx, parent, id, attrs, &pobj, &prop);
>+            if (!ok)
>+                goto restore_scope;
>+            doSet = (attrs == JSPROP_ENUMERATE);

This is setting doSet to true if we are in an eval frame -- but I don't see how that can be correct. ES1-3 require eval to create deleteable bindings, but the specs do not change how a function declaration that is eval'ed must blow away any pre-existing property. In a heavyweight function, we need to preserve the Call specific getter and setter as you note. But in eval called from global code we need to OBJ_DEFINE_PROPERTY, not OBJ_SET_PROPERTY, or we could run some global getter/setter pair.

It seems there is an ambiguity between call hooks we want to preserve, and other getters and setters (possibly user-defined) that we must blow away in global code. So should the initialization of doSet take into account the class of the parent == pobj object?

>+            JS_ASSERT_IF(doSet, fp->flags & JSFRAME_EVAL);
>+            if (prop) {
>+                /*
>+                 * We must not use OBJ_DEFINE_PROPERTY for the existing
>+                 * properties of Call objects with matching attributes to
>+                 * preserve the native getters and setters that store the
>+                 * value of the property in the interpreter frame, see
>+                 * bug 467495.
>+                 */
>+                if (parent == pobj &&
>+                    OBJ_GET_CLASS(cx, parent) == &js_CallClass) {
>+                    JSScopeProperty *sprop = (JSScopeProperty *) prop;

Use top-level sprop here, no need to shadow with a block-local.

>+
>+                    if ((sprop->attrs & (JSPROP_GETTER | JSPROP_SETTER))
>+                        == 0) {

This would be more readable if you violate the 80th column. Or perhaps use !(sprop->attrs & ...).

>+                        /*
>+                         * js_CheckRedeclaration must reject attempts to
>+                         * add a getter or setter to an existing property
>+                         * without a getter or setter.
>+                         */
>+                        JS_ASSERT((attrs &
>+                                   ~(JSPROP_ENUMERATE | JSPROP_PERMANENT))
>+                                  == 0);
>+                        JS_ASSERT((sprop->attrs & JSPROP_READONLY) == 0);
>+
>+                        if ((sprop->attrs &
>+                             (JSPROP_ENUMERATE | JSPROP_PERMANENT)) ==
>+                            attrs) {
>+                            doSet = JS_TRUE;
>+                        }

Aha, so this is why the call_resolve change was needed. This part makes sense but it's overwrapped. Any way to reduce indentation, or else allow for a few chars beyond column 80?

/be

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> Could use cond instead of declaring an opcode-local boolean variable, but
> that's a little less clear due to the vague/generic name "cond".
> 
> >+            JSObject *pobj;
> >+            JSProperty *prop;
> 
> Use obj2 and prop from js_Interpret top-level scope, no need for these.

I already did it once and then spend spent some time figuring out why the test suite crashes - obj2 is already used by JSOP_DEFFUN. But OK, I renames that usage into savedChain keeping changing all obj2 and prop opcode-local for another patch. Since the &obj2 is passed to external functions, the compiler must assume that &obj2 can be stored on a heap or in a global preventing reuse of the native stack slot that stores obj2 for other local values at different opcodes.

> This is setting doSet to true if we are in an eval frame -- but I don't see how that can be correct. ES1-3 require eval to create deleteable bindings, but the specs do not change how a function declaration that is eval'ed must blow away any pre-existing property.

Currently SM does OBJ_SERT_PROPERTY under eval instead of replacing the existing property with OBJ_DEFINE_PROPERTY. The patch preserves this. This violates Ecma but ensures permanent properties stays permanent. Otherwise just doing eval(function x() { }); delete x; will delete the declared variable or argument. IIRC there was a discussion about it on ES lists. 

I will add a comment about this deviation.

> So should the initialization of doSet take into account the class
> of the parent == pobj object?

I prefer to keep semantic changes with this bug minimal. But IMO it is better to fix Ecma spec to require that a function statement inherits the existing attributes of a property if other JS implementations do not allow to delete vars and arguments.

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> (In reply to comment #21)
> > Could use cond instead of declaring an opcode-local boolean variable, but
> > that's a little less clear due to the vague/generic name "cond".
> > 
> > >+            JSObject *pobj;
> > >+            JSProperty *prop;
> > 
> > Use obj2 and prop from js_Interpret top-level scope, no need for these.
> 
> I already did it once and then spend spent some time figuring out why the test
> suite crashes - obj2 is already used by JSOP_DEFFUN.

D'oh -- never mind, pobj local is fine. Sorry, should have looked closer.

> Currently SM does OBJ_SERT_PROPERTY under eval instead of replacing the
> existing property with OBJ_DEFINE_PROPERTY. The patch preserves this. This
> violates Ecma but ensures permanent properties stays permanent. Otherwise just
> doing eval(function x() { }); delete x; will delete the declared variable or
> argument. IIRC there was a discussion about it on ES lists. 

Thanks for reminding me. It's a hole in ES1-3 -- did it get fixed in ES3.1? I'll have to check when after sleep.

/be

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 360530 [details] [diff] [review]
v3

r=me with comment on intentional spec deviation. Thanks again,

/be

Comment 25

[Igor Bukanov]

Attachment: v4

The new version fixes the nits and compresses the code for easier following.

Comment 26

[Igor Bukanov]

landed to tm - http://hg.mozilla.org/tracemonkey/rev/d4ed482363b2

Comment 27

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

Nominating blocking-1.9.0.8 due to comment #11 and comment #12 and the fact that this is a P1 Critical bug on the trunk.

Comment 28

[Igor Bukanov]

(In reply to comment #27)
> Nominating blocking-1.9.0.8 due to comment #11 and comment #12 and the fact
> that this is a P1 Critical bug on the trunk.

I am not sure about 1.9.0 - there the bug does not lead AFAICS to any crashes and the only problem is semantic correctness in the code that relies on SM-only extensions to JS. So at best this could be 1.9.0 wanted.

Comment 29

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

(In reply to comment #28)
> (In reply to comment #27)
> > Nominating blocking-1.9.0.8 due to comment #11 and comment #12 and the fact
> > that this is a P1 Critical bug on the trunk.
> 
> I am not sure about 1.9.0 - there the bug does not lead AFAICS to any crashes
> and the only problem is semantic correctness in the code that relies on SM-only
> extensions to JS. So at best this could be 1.9.0 wanted.

Done, thanks, Igor!

Comment 30

[Igor Bukanov]

The patch triggered mochi test failure on Linux unit test tinderbox, http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1233839695.1233845759.30358.gz . I presume that this is that random timeout bug. But if it persists, do not hesitate to back out the patch.

Comment 31

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d4ed482363b2

Comment 32

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d711ec66549

Comment 33

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/03d6ca9b38b7
/cvsroot/mozilla/js/tests/js1_8/regress/regress-467495-01.js,v  <--  regress-467495-01.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-467495-02.js,v  <--  regress-467495-02.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-467495-03.js,v  <--  regress-467495-03.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-467495-04.js,v  <--  regress-467495-04.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-467495-05.js,v  <--  regress-467495-05.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-467495-06.js,v  <--  regress-467495-06.js
initial revision: 1.1

Comment 34

[Bob Clary [:bc] (inactive)]

verified 1.9.1, 1.9.2