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),

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),

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

Fix coming.

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.

In bug 561954 dvander suggests aborting.

> 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; }

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

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

waldo, any updates here?

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

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

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.

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

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 on attachment 501545 [details] [diff] [review] fix Thanks for doing it right :)

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 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." ?)

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

JSBugMon: This bug has been automatically verified fixed.