1077949 - Assertion failure: !IsUninitializedLexical(val), at vm/Interpreter.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function() {
    switch (0) {
        case 1:
            let x
        case function() {
            print(x)
        }():
    }
}())

asserts js debug shell on m-c changeset 229801d17f7e with --no-ion --no-threads at Assertion failure: !IsUninitializedLexical(val), at vm/Interpreter.cpp.

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7027efe7fae3
user:        Shu-yu Guo
date:        Mon Sep 15 16:30:45 2014 -0700
summary:     Bug 1001090 - Part 1: Implement let temporal dead zone in the frontend and interpreter. (r=Waldo)

Shu-yu, is bug 1001090 a possible regressor?

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Fix TDZ checks when closing over non-dominating lexical declarations in switches.

Comment 2

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

Attachment: stack for m-c rev f0bb13ef0ee4

( I ran: " $ lldb -- ./js-dbg-opt-64-dm-nsprBuild-darwin-f0bb13ef0ee4 --ion-eager --no-threads 1077949.js " )

(lldb) bt 5
* thread #1: tid = 0x80840, 0x000000010020197a js-dbg-opt-64-dm-nsprBuild-darwin-f0bb13ef0ee4`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: 0x000000010020197a js-dbg-opt-64-dm-nsprBuild-darwin-f0bb13ef0ee4`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: 0x000000010020195e js-dbg-opt-64-dm-nsprBuild-darwin-f0bb13ef0ee4`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: 0x000000010020195e js-dbg-opt-64-dm-nsprBuild-darwin-f0bb13ef0ee4`js::jit::DoTypeMonitorFallback(cx=<unavailable>, frame=<unavailable>, stub=<unavailable>, value=<unavailable>, res=<unavailable>) + 894 at BaselineIC.cpp:1287
    frame #3: 0x0000000101adc5d7
(lldb)

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8500683 [details] [diff] [review]
Fix TDZ checks when closing over non-dominating lexical declarations in switches.

Review of attachment 8500683 [details] [diff] [review]:
-----------------------------------------------------------------

Woo, I just relearned all this mechanism.  My head hurts again.

::: js/src/frontend/Parser.cpp
@@ +1260,5 @@
> +{
> +    MOZ_ASSERT(dn->isLet());
> +    StmtInfoPC *stmt = LexicalLookup(pc, name, nullptr, nullptr);
> +    if (stmt && stmt->type == STMT_SWITCH)
> +        return dn->pn_cookie.slot() < stmt->firstDominatingLexicalInCase;

Apropos of nothing, I just found a stale "firstDominatingLetInCase" in a Parser.cpp comment -- please fix it and any others lurking.

@@ +1896,5 @@
> +        // lexical binding in a switch, as lexical declarations currently
> +        // disable syntax parsing. So a non-dominating but textually preceding
> +        // lexical declaration would have aborted syntax parsing, and a
> +        // textually following declaration would return true for
> +        // handler.isPlaceholderDefinition(dn) below.

An assertion of some sort might be nice here, to flag this when we get around to syntax-parsing lexical declarations.  If one's possible.  I'm out now, you figure it out.  :-)

::: js/src/jit-test/tests/basic/letTDZSwitchClosure.js
@@ +1,1 @@
> +function assertThrowsReferenceError(f) {

I don't believe this test, anywhere, exercises the case where |stmt && stmt->type == STMT_SWITCH| but the let-use *does* dominate the let-declaration.  I added a MOZ_ASSERT(dn->pn_cookie.slot() < stmt->firstDominatingLexicalInCase), and it didn't trigger at all in jstests.  It *might* have triggered once, incidentally, in jit-tests -- debug/Environment-variables.js -- but I'm out of time to check today, and I want to submit the r+ before leaving.  :-)

So please add this as a test, to ensure that path is hit (appropriately cleaned up for nice reading and whatever, and so it executes in some sane way rather than nonsensically as I wrote it):

(function() { switch (v) { case 0: let x; (function() { x; })(); } })()

@@ +23,5 @@
> +  switch (0) {
> +    case 1:
> +      let x;
> +    case 0:
> +      var inner = function () { 

Trailing WS

@@ +35,5 @@
> +
> +function h() {
> +  switch (0) {
> +    case 0:
> +      var inner = function () { 

Trailing WS

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8500683 [details] [diff] [review]
Fix TDZ checks when closing over non-dominating lexical declarations in switches.

Review of attachment 8500683 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +1344,4 @@
>               * (see LegacyCompExprTransplanter::transplant, the PN_CODE/PN_NAME
>               * case), and nowhere else, currently.
>               */
>              if (dn != outer_dn) {

Another comment, from my now-working nightly profile:

It might be nice to have a helper function to contain this whole |if (pc->lexdeps()->count()) { ... }| bit of code.  It's gotten pretty unwieldly, and it's hard to have a sense of what leaveFunction as a whole does.  rs=me for a new method, separate patch atop these changes.  I don't have immediate ideas for names.

@@ +1371,1 @@
>                          while (true) {

These two while-loops would be nicer inside named methods, markAllUsesAsLets() and associateUsesWithDefinition().  Or the latter method, with code in it to mask in PND_LET depending upon whether the arguments agree with doing so or not.

More cosmetics, basically.

Comment 5

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b865ab14ff0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23009633a7dc

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b865ab14ff0
https://hg.mozilla.org/mozilla-central/rev/23009633a7dc
https://hg.mozilla.org/mozilla-central/rev/d4aa76cff57b

Comment 8

[Shu-yu Guo [:shu]]

Attachment: bug1077947

Approval Request Comment
[Feature/regressing bug #]: 1001090
[User impact if declined]: Crashes in some addons, like Pentadactyl (see bug 1108345)
[Describe test coverage new/current, TBPL]: Been on trunk since 36.
[Risks and why]: 
[String/UUID change made/needed]: None

Backport for beta.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4c0971946531