for (var j = 0; j < 9; ++j) { try { (function() { (function() { eval("x") let x })() })() } catch (e) {} } asserts js debug shell on m-c changeset 20408ad61ce5 with --ion-eager --no-threads at Assertion failure: data.s.payload.why == why, at dist/include/js/Value.h. Debug configure options: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests Due to skipped revisions, the first bad revision could be any of: changeset: https://hg.mozilla.org/mozilla-central/rev/03242a11d044 user: Shu-yu Guo date: Mon Sep 15 16:30:45 2014 -0700 summary: Bug 1001090 - Part 2a: Compile new let opcodes in Baseline. (r=jandem) changeset: https://hg.mozilla.org/mozilla-central/rev/8114e77c253e user: Shu-yu Guo date: Mon Sep 15 16:30:46 2014 -0700 summary: Bug 1001090 - Part 2b: Fix unwinding all scopes to not use pc. (r=jimb) changeset: https://hg.mozilla.org/mozilla-central/rev/31714af41f2c user: Shu-yu Guo date: Mon Sep 15 16:30:46 2014 -0700 summary: Bug 1001090 - Part 3: Compile new let opcodes in Ion. (r=jandem) Shu-yu, is bug 1001090 a possible regressor?

(lldb) bt 5 * thread #1: tid = 0x473e92, 0x000000010020ac52 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::DoTypeMonitorFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] JS::Value::isMagic(this=<unavailable>, why=<unavailable>) const + 28 at Value.h:1165, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x000000010020ac52 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::DoTypeMonitorFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] JS::Value::isMagic(this=<unavailable>, why=<unavailable>) const + 28 at Value.h:1165 frame #1: 0x000000010020ac36 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::DoTypeMonitorFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] js::ValueOperations<JS::Handle<JS::Value> >::isMagic(why=<unavailable>) const at Value.h:1681 frame #2: 0x000000010020ac36 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::DoTypeMonitorFallback(cx=<unavailable>, frame=<unavailable>, stub=<unavailable>, value=<unavailable>, res=<unavailable>) + 998 at BaselineIC.cpp:1287 frame #3: 0x0000000101afa69f frame #4: 0x0000000101aef940 (lldb)

Jan, this fixes 2 bugs: the bug you pointed out on IRC about fixed slots, and the one that causes this bug. I was only setting lexicals to throw on touch on the normal CallObject::create path, and not the createTemplateObject path, so Ion would copy over undefineds instead of JS_MAGIC_UNINITIALIZED in the fast path.

Comment on attachment 8512194 [details] [diff] [review] Initialize lexicals to throw on touch on CallObject templates. Review of attachment 8512194 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/vm/ScopeObject-inl.h @@ +56,5 @@ > inline void > CallObject::setAliasedLexicalsToThrowOnTouch(JSScript *script) > { > uint32_t aliasedLexicalBegin = script->bindings.aliasedBodyLevelLexicalBegin(); > + uint32_t aliasedLexicalEnd = numFixedSlots() + numDynamicSlots(); Nit: uint32_t aliasedLexicalEnd = slotSpan(); ::: js/src/vm/ScopeObject.cpp @@ +198,5 @@ > if (!obj) > return nullptr; > > + // Set uninitialized lexicals even on template objects, as Ion will use > + // copy over the template object's slot values in the fast path. Nit: s/use//

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d847bcf86f for rootanalysis orange: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3405688&repo=mozilla-inbound

Turns out we have many, many ways to create CallObjects.

Comment on attachment 8513882 [details] [diff] [review] Initialize lexicals to throw on CallObject::create cutouts for Ion. Review of attachment 8513882 [details] [diff] [review]: ----------------------------------------------------------------- Tacking on an extra index argument, the correctness of the passed value which isn't all that apparent at many call sites, is pretty smelltastic. I don't have any better ideas, tho. ::: js/src/vm/ScopeObject-inl.h @@ +53,5 @@ > types::AddTypePropertyId(cx, this, id, v); > } > > inline void > +CallObject::initRemainingSlotsToUninitializedLexicals(uint32_t begin) Not entirely sure why this helper method exists, seeing as it has only one caller and the overall method is rather far from being overly complex. Just assign the begin-index to a reasonably-named variable and things should be clear enough, seems to me.

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 675913ddbb55).

This needs a backport, no?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > This needs a backport, no? I suppose so.

Approval Request Comment [Feature/regressing bug #]: 1001090 [User impact if declined]: Incorrect JS behavior when using 'let' bindings with closures. [Describe test coverage new/current, TBPL]: Tested on m-c. [Risks and why]: Low risk. Not a security issue. Spec compliance for ES6 feature that nobody is using yet. [String/UUID change made/needed]: None.

Hm, this reminds me, bug 1092833 will need backport as well.