453249 - TM: "Assertion failure: s0->isQuad()"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

./js -j
js> this.__proto__.a = 3; for (var j = 0; j < 4; ++j) { [a]; }

Assertion failure: s0->isQuad(), at jstracer.cpp:460

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix?

This seems to fix the assertion. I'm not exactly sure if it's always right though.

Comment 2

[Andreas Gal :gal]

Comment on attachment 337540 [details] [diff] [review]
Fix?

insCall modifies args, so don't put the lir call into there. Also, this is incorrect since box_jsval should get in a double, not an int. Try to find out where the int here comes from and do i2f there, not in box_jsval.

Comment 3

[Andreas Gal :gal]

This fails:

this.__proto__.a = 3; for (var j = 0; j < 4; ++j) { [a]; }

This works:

this.a = 3; for (var j = 0; j < 4; ++j) { [a]; }

My best guess is that the property cache lookup doesn't guard that the hit is in the object itself and not in the prototype.

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

No, we guard just fine on the proto/scope coordinates but then test_property_cache_direct_slot forgot to insist on a direct slot for "get" ops such as name. We cannot handle a prototype property of the global object. I don't think this is a problem for benchmarks or real-world code. This patch makes us abort trace-recording cleanly.

/be

Comment 5

[Andreas Gal :gal]

Comment on attachment 338780 [details] [diff] [review]
fix

This is what I was suspecting. Pushing.

Comment 6

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/165b7a7b2e45

Comment 7

[Brendan Eich [:brendan]]

Oh, you meant *that* kind of "guard" :-P.

Cool, the layering and logic check out now: only TraceRecorder::name calls TraceRecorder::test_property_cache_direct_slot, so all unqualified name refs (simple "get" uses as in this test, or a++, --a, and so on) require direct slots. Thanks for pushing.

Are we closing now when changes hit tracemonkey? Probably ok so long as we sync with mozilla-central quickly.

/be

Comment 8

[Andreas Gal :gal]

We keep forgetting to close bugs otherwise.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow the trace-test here.  Specifically, why is the function even needed?  Just to give the test a name?

Comment 10

[Andreas Gal :gal]

Yeah, basically. Didn't know a better way to test something that happened in the global scope. We could pass in the global object via this though. Not sure that would still trigger the bug?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Nah, no need to change anything.  I was just checking to see whether I was missing something.

Comment 12

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-453249.js,v  <--  regress-453249.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/8a23de31b12a