Closed
Bug 1077949
Opened 10 years ago
Closed 10 years ago
Assertion failure: !IsUninitializedLexical(val), at vm/Interpreter.cpp
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: gkw, Assigned: shu)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])
Attachments
(3 files)
5.46 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.42 KB,
text/plain
|
Details | |
6.39 KB,
patch
|
shu
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(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 | ||
Comment 1•10 years ago
|
||
Attachment #8500683 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → shu
Status: NEW → ASSIGNED
Flags: needinfo?(shu)
Reporter | ||
Comment 2•10 years ago
|
||
( 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•10 years ago
|
||
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 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b865ab14ff0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/23009633a7dc
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b865ab14ff0 https://hg.mozilla.org/mozilla-central/rev/23009633a7dc https://hg.mozilla.org/mozilla-central/rev/d4aa76cff57b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 8•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8535298 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/4c0971946531
You need to log in
before you can comment on or make changes to this bug.
Description
•