Closed Bug 1077949 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: gkw, Assigned: shu)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(3 files)

(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?
Flags: needinfo?(shu)
Assignee: nobody → shu
Status: NEW → ASSIGNED
Flags: needinfo?(shu)
( 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 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
Attachment #8500683 - Flags: review?(jwalden+bmo) → review+
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.
Attached patch bug1077947Splinter Review
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.
Attachment #8535298 - Flags: review+
Attachment #8535298 - Flags: approval-mozilla-beta?
Attachment #8535298 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.