1089761 - Assertion failure: data.s.payload.why == why, at dist/include/js/Value.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

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?

Comment 1

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

Attachment: stack

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

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Initialize lexicals to throw on touch on CallObject templates.

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 3

[Jan de Mooij [:jandem]]

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//

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Initialize lexicals to throw on CallObject::create cutouts for Ion.

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

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fd815999686

Comment 8

[Christian Holler (:decoder)]

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

Comment 9

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/0fd815999686

Comment 10

[Jeff Walden [:Waldo]]

This needs a backport, no?

Comment 11

[Shu-yu Guo [:shu]]

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

I suppose so.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: bug1089761.patch

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.

Comment 13

[Shu-yu Guo [:shu]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1af24d572c4