597940 - TM: "Assertion failure: ((const Value *)p)->isNumber() == (t == JSVAL_TYPE_DOUBLE)," or "Assertion failure: hasInt32Repr(*(const Value *)p),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

for (z in [0])
function b() {
    Object.defineProperty(this, "z", ({
        value: this
    }))
    this.z = 1e81
}
for (a in [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) {
    try {
        b()
    } catch (e) {}
}

asserts js debug shell on TM changeset 3559d9fded5f with -j at Assertion failure: ((const Value *)p)->isNumber() == (t == JSVAL_TYPE_DOUBLE),

Comment 1

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

Here's a similar variant but with a different assert message.

function b() {
    Object.defineProperty(this, "z", ({
        value: this,
        writable: true
    }));
    z = 1
}
for (a in [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) {
    try {
        b()
    } catch (e) {}
}


Assertion failure: hasInt32Repr(*(const Value *)p),

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   36651:766a6b2e74e7
user:        Jeff Walden
date:        Fri Jun 05 12:56:45 2009 -0700
summary:     Bug 430133 - Implement ES3.1's Object.defineProperty and
Object.defineProperties.  r=jorendorff

Comment 3

[Jason Orendorff [:jorendorff]]

Fix coming.

Comment 4

[Jason Orendorff [:jorendorff]]

I thought this would be trivial to fix, but it's not. Abandoning.

Here's the minimized test:

z = 0;
for (var i = 0; i < 20; i++) {
    Object.defineProperty(this, "z", {value: ""});
    z = 1;
}

It asserts the second time we try to trace this loop. On loop entry, the TraceRecorder knows that global.z is an int32. Object.defineProperty quietly stores a string in the global slot for z. The tracer doesn't notice that this has happened. So when we execute z=1, the TraceRecorder tries to import global.z using the type from the importTypeMap, which is wrong.
I think if we went through and recorded this trace, we would just bail off it (safely) at run time. In this case.

A possible solution is for LeaveTraceIfGlobalObject to check for a TraceRecorder and abort recording if any is present; but dvander points out that this might be too pessimistic. The interpreter takes some slow paths, possibly in cases where the tracer knows exactly what's going on. We don't want to abort in those cases.

So, another possible solution is for LeaveTraceIfGlobalObject to set a flag in the TraceRecorder (if any) which tells it to re-check the global slot types it thinks it knows. The TraceRecorder would have to check this after every time the interpreter has a chance to do unexpected stuff.

Comment 5

[Jason Orendorff [:jorendorff]]

In bug 561954 dvander suggests aborting.

Comment 6

[Jason Orendorff [:jorendorff]]

> I think if we went through and recorded this trace, we would just bail off it
> (safely) at run time. In this case.

This is definitely true. You can arrange for Object.defineProperty to do nothing at record time, then do something nefarious only on trace. In that case we correctly leave trace every time.

var harmless = {value: 0};
var bad = {value: ""};
var a = [bad, bad, bad, bad, bad, bad, bad, bad, harmless, bad, bad, bad];

z = 0;
for (var i = 0; i < a.length; i++) {
    print(tracemonkey.onTrace);
    Object.defineProperty(this, "z", a[i]);
    z = 1;
}

Comment 7

[Andreas Gal :gal]

Aborting is really the way to go whenever the global object is manipulated behind the back of the tracer.

Comment 8

[Brendan Eich [:brendan]]

This bug seems fixable and needs an assignee. Rob, can you find a volunteer?

/be

Comment 9

[Johnny Stenback (:jst)]

waldo, any updates here?

Comment 10

[Jeff Walden [:Waldo]]

No, I haven't had time to look at this.  :-(

Comment 11

[Johnny Stenback (:jst)]

Jeff, any idea when you'll get to this?

Comment 12

[Jeff Walden [:Waldo]]

Sooner, now that bug 514568 is wrapping up.  Not sure about an exact time yet, though, but I hope before end of year, if other blockers cooperate.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Thanks to Luke's work in bug 561954 all this needs is a tweak.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Explanation: This check was trying to avoid aborting the trace when setting an unexpected, but untracked global. Checking if the global slot is in the recorder's tracker isn't quite enough, since it could also be in the import type map. We could check that as well, but this case never hits on SS, v8, or Kraken, and it seems very unlikely in general.

Comment 15

[Luke Wagner [:luke]]

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

Thanks for doing it right :)

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/48a2416b7f41

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

What a mess. Backed out as http://hg.mozilla.org/tracemonkey/rev/a65393138ec4.

Pretty much every fallible function in the engine can deep-abort the recorder. bug 561954 made it more likely, and this one made it extremely likely (since lazy resolving happens quite often). This isn't safe, since very few places in the recorder check to see if a fallible function caused a deep abort.

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Comment 20

[Luke Wagner [:luke]]

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

>+             * Otherwise, only abort if the global is not present in the
>+             * import typemap. Just deep aborting false here is not acceptable,
>+             * because the recorder does not guard on every operation that
>+             * could lazily resolve. Since resolving adds properties to
>+             * reserved slots, the tracer will never have imported them.

Yes, this is the comment I should have written the first time, sorry.  (Except, isn't it "only abort if the global is present in the import typemap." ?)

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

follow-up fix for assert botch, limit the global checks to only the global the trace is keyed on: http://hg.mozilla.org/tracemonkey/rev/d08f63692c60

Comment 23

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

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

Comment 24

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

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

Comment 25

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.