495962 - TM: "Assertion failed: p->isQuad()" with "for each" assigning to const (hidden by "with")

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

(function () {
    const c = 0;
    with ({}) {
        for each (c in [{}, {}]) {
            "" + c;
        }
    }
})();

Assertion failed: p->isQuad() (../nanojit/Nativei386.cpp:1348)

The "with" prevents the static error "SyntaxError: invalid for/in left-hand side" from occurring (which is what now happens to the testcase in bug 465443), but "c" still refers to the constant.

Comment 1

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

autoBisect shows this is probably related to bug 481989 :

The first bad revision is:
changeset:   25779:4c66b8ccb1b8
user:        Andreas Gal
date:        Sat Mar 07 01:25:37 2009 -0800
summary:     Check for non-stub getters/setters in SETNAME and SETPROP and invoke SetPropHit after setting the property in INITPROP (481989, r=brendan).

Comment 2

[Jesse Ruderman]

This can also lead to incorrect numerical results in a debug build, without asserting, fwiw.

Comment 3

[Jesse Ruderman]

And it can lead to

Assertion failure: !JSVAL_IS_PRIMITIVE(v), at ../jsnum.cpp:921

Comment 4

[Andreas Gal :gal]

Why didn't we block on this? (just curious, probably too late now). I will try to take a look over the weekend.

Comment 5

[Brendan Eich [:brendan]]

This should get taken if it has a safe, tested patch and there's a respin or RC2.

/be

Comment 6

[Robert Sayre]

we didn't block on it because it requires "const" and "for each". not likely to be hit on web code.

Comment 7

[Andreas Gal :gal]

Needs const in global scope but not for each. Bad bug. Easy fix. Gary just filed a likely dup. Allows address forging. Will upload patch if I find inet.

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 383128 [details] [diff] [review]
patch

Parenthesize & against && and other ops, always (same for |, ^, <<, >> but not unary ops :-P).

/be

Comment 10

[Andreas Gal :gal]

Pushed to TM.

http://hg.mozilla.org/tracemonkey/rev/0abfb7de22d2

Comment 12

[Andreas Gal :gal]

Removed dependency. Bug 481989 made us trace here, but the actual problem is unrelated.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking but tagging with [3.5RC2?] as per comment 5 and comment 6. Let's get this over to trunk, bien sur.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

We're considering taking this as a ridealong for RC, but before we do, need to know what the breadth of effect is here. How much code touches this path? Would we be comfortable shipping this without a full week long RC evaluation period to spot any additional regression? Is there a set of testing we can do to give us additional confidence?

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0abfb7de22d2

Comment 16

[Brendan Eich [:brendan]]

This is a safe and minimal fix, based on identical code nearby that was not factored or copied (as this bug's patch did) to the needed place. This code path is specific to functions containing with, eval, and other such functions, and it also needs a const binding on the activation (Call in SpiderMonkey jargon) object to bite.

So the patch is not going to break any var or other binding, and it does fix the const case.

/be

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 383128 [details] [diff] [review]
patch

OK, let's take it. Please land on mozilla-1.9.1 asap.

a=beltzner

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

No, really, please land on mozilla-1.9.1 asap :) just use CLOSED TREE to get past the closed tree.

Comment 19

[(not currently active) Ted Mielczarek]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3554916b3a56

Comment 20

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

Jessy, can you verify this bug and that it doesn't occur anymore in RC3? I have problems to reproduce it. Thanks.

Comment 21

[Jesse Ruderman]

verified1.9.1 using a 1.9.1 branch shell.

If you want to test it in a browser you'll have to use
<script type="text/javascript;version=1.8">

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 23

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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