620315 - "Assertion failure: unexpected constantly false guard detected" with "use strict", 4>this

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

./js -j -m

new Function('"use strict"; for (var j = 0; j < 9; ++j) { 4 > this; }')()

Assertion failure: unexpected constantly false guard detected, at jstracer.cpp:4321

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

The first bad revision is:
changeset:   38cbd4e02afc
user:        Jeff Walden
date:        Tue Oct 12 11:50:03 2010 -0700
summary:     Bug 514570 - 3 - Don't box |this| for strict mode functions. r=jorendorff

Comment 3

[Boris Zbarsky [:bzbarsky]]

I thought we allowed constant-false guards....

Comment 4

[Jesse Ruderman]

bz, see the big comment near http://hg.mozilla.org/tracemonkey/annotate/8a9c6231fd52/js/src/jstracer.cpp#l4307

Comment 5

[Nicholas Nethercote [inactive]]

I think this shouldn't block 2.0.  Constantly false guards can cause sub-optimal performance but they aren't inherently incorrect or dangerous.  

dmandelin, can you mark it blocking2.x?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Why is this a fatal assert if it's not inherently incorrect?

Comment 7

[Jeff Walden [:Waldo]]

new Function functions lack JSFUN_PRIMITIVE_THIS.  You can also see the difference with this wrong behavior, interpreter-only (probably reproducible with methodjit too):

var toString = function() { return 5; };
assertEq(new Function("'use strict'; return this > 4;")(), false);

JSFUN_PRIMITIVE_THIS was misguided.  It was introduced for natives, but natives always get it as I recall, so it's pointless.  Detection based on it really should be for strict mode directly.

This is type confusion (and wrong behavior moreover), so it should block.

Comment 8

[Jeff Walden [:Waldo]]

(The effect is regs.fp->computeThis(cx) actually does computeThis here, when it shouldn't.)

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
>
> Why is this a fatal assert if it's not inherently incorrect?

Because it indicates poor code generation quality, and an assert is the easiest way to get people's attention.  Can you think of a better alternative?


(In reply to comment #7)
> 
> This is type confusion (and wrong behavior moreover), so it should block.

In that case, fair enough.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Can you think of a better alternative?

A debug printf that doesn't kill the browser and hence doesn't interfere with things like fuzz testing, say?

Comment 11

[Nicholas Nethercote [inactive]]

Well, maybe, though as this case shows it can be due to correctness issues as well.  Do debug printfs tend to just get lost/ignored?  That's my impression.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sure; I was just basing my question on the "this is always safe" claims above...

Comment 13

[Jeff Walden [:Waldo]]

Attachment: 1 - Remove JSFUN_PRIMITIVE_THIS

I had both these patches finished a bit ago, left them running tests while I ran out to perform an errand -- might as well post them and get this fixed.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: 2 - Make JSFunction::inStrictMode assume isInterpreted()

Looking at callers, I noticed most to all of them are already called on an interpreted function.  It seems reasonable to push that check out to the two calling locations that explicitly need it, keep things a little bit thinner where we can.

Comment 15

[David Mandelin [:dmandelin]]

What's the risk level of landing these patches? I'm hesitant to move forward on nonblocking bugs at this point in case they cause regressions. 

(Also setting blocking to 2.x per comment 5, which I didn't see earlier.)

Comment 16

[Jeff Walden [:Waldo]]

The former is code *removal* and simplification.  Risk is low to start, and it's even lower because it makes the code smaller and clearer.

The latter is even less risky -- just splitting up one very-trivial inline method into two separate calls at each caller, most of which already checked (or can trivially assume, e.g. that the function in question *being currently traced* must be interpreted) that the first such call already returned true, so need not be performed.

In comment 7 I argue this is type confusion in the tracer and therefore should block.  We only happened to trip over the particular assertion in the original report by dumb luck.  You could easily go astray in other ways than just redundantly guarding here, and I am unwilling to give a limit for just how far astray you'd go.

Comment 17

[Jeff Walden [:Waldo]]

...plus there are also behavioral differences between tracer and interpreter, and even flat-out bugs where we do things wrong regardless whether tracer, interpreter, or methodjit is configured.

Comment 18

[David Mandelin [:dmandelin]]

OK, thanks for the clarification. Will review now.

Comment 19

[David Mandelin [:dmandelin]]

Comment on attachment 499449 [details] [diff] [review]
2 - Make JSFunction::inStrictMode assume isInterpreted()

r+ with this addition:


> inline bool
> JSFunction::inStrictMode() const
> {

      JS_ASSERT(isInterpreted());

>-    return isInterpreted() && u.i.script->strictModeCode;
>+    return script()->strictModeCode;
> }

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> r+ with this addition:
> 
> > inline bool
> > JSFunction::inStrictMode() const
> > {
> 
>       JS_ASSERT(isInterpreted());
> 
> >-    return isInterpreted() && u.i.script->strictModeCode;
> >+    return script()->strictModeCode;
> > }

JSFunction::script() includes that assertion -- seems a nice readability win to rely on this, as I'm pretty sure we do elsewhere.  Make sense?

Comment 21

[David Mandelin [:dmandelin]]

(In reply to comment #20)
> JSFunction::script() includes that assertion -- seems a nice readability win to
> rely on this, as I'm pretty sure we do elsewhere.  Make sense?

Yes. I didn't know that.

Comment 22

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/f4c2edf5362e

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

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

Comment 24

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929