486713 - "Assertion failure: cg->treeContext.flags & TCF_HAS_SHARPS, at ../jsemit.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

({a:#3#})([#1#] for each (x in []))

Assertion failure: cg->treeContext.flags & TCF_HAS_SHARPS, at ../jsemit.cpp:6237

(debug js shell without -j)

Comment 1

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

autoBisect shows this is probably related to bug 485164 or http://hg.mozilla.org/tracemonkey/rev/382c1065eab7 :

The first bad revision is:
changeset:   26767:382c1065eab7
user:        Igor Bukanov
date:        Fri Apr 03 09:53:02 2009 +0200
summary:     bug 485164 - fixing sharp semantic regressions. r=mrbkap

Comment 2

[Igor Bukanov]

In the patch for the bug 485164 I have added TCF_HAS_SHARPS flags to mark function or script containing sharp variables as unsuitable for JSOP_NEWARRAY optimized bytecode. I assumed that once set the flag would always end up in the function node since the flag is in TCF_FUN_FLAGS.

But the generator expression code defeated this assumption due to the following code:

    lambda->pn_flags = TCF_FUN_IS_GENERATOR | TCF_GENEXP_LAMBDA |
                       ((oldflags ^ tc->flags) & TCF_FUN_FLAGS);
...
    tc->flags = oldflags;

Here oldflags are the tc flags before the left-hand-side of the generator expression was parsed. The problem here is that if oldfalgs has TCF_HAS_SHARPS and the left-hand-side would also add TCF_HAS_SHARPS, then this would cancel the flags.

This cancellation affects not only the newly introduced TCF_HAS_SHARPS but also TCF_FUN_HEAVYWEIGHT. And that allows to remove TCF_FUN_HEAVYWEIGHT from the generator function. Here is an example that demonstrates this:

@watson~> cat ~/s/x2.js
function f()
{
    eval("1");
    return ((arguments=Math, arguments.sin(1)) for each (x in [1]));
}

var actual = f().next();
if (actual != Math.sin(1))
    throw "Bad";
@watson~> ~/build/js32.tm.dbg/js ~/s/x2.js
/home/igor/s/x2.js:4: TypeError: arguments.sin is not a function

I suppose this is bug is critical as a missed TCF_FUN_HEAVYWEIGHT flag can be exploited. Also this bug is not a regression from the bug 485164. That one just exposed the problem in another way.

Comment 3

[Igor Bukanov]

On 1.9.0 the bug translates into a missing heavyweight flag on the generator expression function. Since the left-hand-side of the generator expression is also an expression, the missing flag can only affect the following things.

1. Anonymous functions. If such function gets a heavyweight flag itself or accesses non-local variables, then its parent must also be a heavyweight. But the compiler performs such propagation of the flag during code generation. Thus such propagated flag would not be affected by this bug. 

One corner case here would happen if the left-hand-side in turn contains nested generator expressions. In such case one can get a chain of heavy nested functions all missing the flag. But since the generator expression function always starts with JSOP_ENTERBLOCK, any use of these nested function expressions would force creation of the call object and proper scope chain on the parent completely neutralizing bug's consequences.

2. Manipulations of the argument object like assignments to it. For such operations the compiler generates the code as for any non-local name assuming that the call object created for the function would take care of it. But in this case there would be non call object so assignments like arguments = would affect the parent scope. Besides unexpected semantics of such code this is harmless.

3. Using __parent__ or __proto__ as names. As with the argument name, this bug leads to the names accessing or manipulating the wrong object. So again, we only have surprising semantic, nothing else.

4. eval calls. The bug translates into having a direct eval in a not-heavy function. Since the eval always makes sure that its caller gets a call object, this bug translates again into the code not observing manipulations with arguments name.

Now, on 1.9.1 the situation is more tricky. Due to bug 485164 and bug 452498 we now have at least two more missing flags, TCF_HAS_SHARPS and TCF_FUN_USES_ARGUMENTS. Now, the landed patch for bug 452498 fixed TCF_HAS_SHARPS issue. But TCF_FUN_HEAVYWEIGHT and TCF_FUN_USES_ARGUMENTS are still not there. Right now I do not know if a missing TCF_FUN_USES_ARGUMENTS could do more harm than missing TCF_FUN_HEAVYWEIGHT.

The bottom line is that on 1.9.0 the bug is not critical and just deserves wanted1.9.0 with a safe one-liner fix. On 1.9.1 the situation is less clear so 1.9.1 blocking is deserved.

Comment 4

[Igor Bukanov]

Attachment: v2

The patch makes sure that all relevant deoptimization flags are propagated to the function representing the generator expression.

Comment 5

[Igor Bukanov]

recording proper regression dependency on bug 380237

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 371041 [details] [diff] [review]
v2

Thanks, I lost track of this but was worrying about the bug.

/be

Comment 7

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/914e444ed166

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/914e444ed166

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/49234f4a69fb

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-486713.js

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with testcase given in comment 0 on trunk and 1.9.1 with the
following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090422 Minefield/3.6a1pre ID:20090422224452

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043

Comment 12

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.