Crash [@ js::NativeObject::replaceWithNewEquivalentShape] or Assertion failure: !inDictionary(), at vm/Shape.h

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: Waldo)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla40
x86_64
Mac OS X
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 unaffected, firefox39 fixed, firefox40 fixed)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
// Randomly chosen test: js/src/jit-test/tests/basic/bug646968-7.js
for (let x = 0; x < 9; ++x) {
    (function() {
        eval('var x');
    })();
}

asserts js debug shell on m-c changeset 385840329d91 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: !inDictionary(), at vm/Shape.h and crashes js opt shell at js::NativeObject::replaceWithNewEquivalentShape.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 385840329d91

Opt configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --disable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build" -r 385840329d91

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b05e10ed40c4
user:        Jeff Walden
date:        Fri Mar 27 12:29:50 2015 -0400
summary:     Bug 854037 - Make lexical declarations in the initializing component of a for(;;) loop create a fresh binding for each iteration of the loop.  r=shu

Waldo, is bug 854037 a likely regressor?
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 1

3 years ago
Created attachment 8586392 [details]
stack

(lldb) bt 5
* thread #1: tid = 0x50a02, 0x000000010087394b js-dbg-64-dm-nsprBuild-darwin-385840329d91`JSObject::create(js::ExclusiveContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) [inlined] js::Shape::slotSpan(this=<unavailable>, this=<unavailable>) const + 67 at Shape.h:905, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010087394b js-dbg-64-dm-nsprBuild-darwin-385840329d91`JSObject::create(js::ExclusiveContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) [inlined] js::Shape::slotSpan(this=<unavailable>, this=<unavailable>) const + 67 at Shape.h:905
    frame #1: 0x0000000100873908 js-dbg-64-dm-nsprBuild-darwin-385840329d91`JSObject::create(js::ExclusiveContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) [inlined] js::Shape::slotSpan(this=<unavailable>) const at Shape.h:911
    frame #2: 0x0000000100873908 js-dbg-64-dm-nsprBuild-darwin-385840329d91`JSObject::create(cx=<unavailable>, kind=<unavailable>, heap=<unavailable>, shape=<unavailable>, group=<unavailable>) + 2616 at jsobjinlines.h:277
    frame #3: 0x00000001002b9b6f js-dbg-64-dm-nsprBuild-darwin-385840329d91`js::ClonedBlockObject::clone(cx=0x00000001028a5180, block=<unavailable>) + 223 at ScopeObject.cpp:673
    frame #4: 0x00000001006bfa0f js-dbg-64-dm-nsprBuild-darwin-385840329d91`js::jit::FreshenBlockScope(JSContext*, js::jit::BaselineFrame*) [inlined] js::jit::BaselineFrame::freshenBlock(this=0x00007fff5fbfe878) + 76 at BaselineFrame-inl.h:80
(lldb)
(Reporter)

Comment 2

3 years ago
Created attachment 8586393 [details]
stack of opt crash

(lldb) bt 5
* thread #1: tid = 0x50ddf, 0x0000000100195df1 js-64-dm-nsprBuild-darwin-385840329d91`js::NativeObject::replaceWithNewEquivalentShape(js::ExclusiveContext*, js::Shape*, js::Shape*, bool) [inlined] js::BarrieredBase<js::Shape*>::pre() at Barrier.h:452, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100195df1 js-64-dm-nsprBuild-darwin-385840329d91`js::NativeObject::replaceWithNewEquivalentShape(js::ExclusiveContext*, js::Shape*, js::Shape*, bool) [inlined] js::BarrieredBase<js::Shape*>::pre() at Barrier.h:452
    frame #1: 0x0000000100195df1 js-64-dm-nsprBuild-darwin-385840329d91`js::NativeObject::replaceWithNewEquivalentShape(js::ExclusiveContext*, js::Shape*, js::Shape*, bool) [inlined] js::HeapPtr<js::Shape*>::set(this=0x0000000000000000) at Barrier.h:537
    frame #2: 0x0000000100195df1 js-64-dm-nsprBuild-darwin-385840329d91`js::NativeObject::replaceWithNewEquivalentShape(js::ExclusiveContext*, js::Shape*, js::Shape*, bool) [inlined] js::HeapPtr<js::Shape*>::operator=(this=0x0000000000000000) at Barrier.h:523
    frame #3: 0x0000000100195df1 js-64-dm-nsprBuild-darwin-385840329d91`js::NativeObject::replaceWithNewEquivalentShape(js::ExclusiveContext*, js::Shape*, js::Shape*, bool) [inlined] js::Shape::removeFromDictionary(js::NativeObject*) + 21 at Shape.cpp:90
    frame #4: 0x0000000100195ddc js-64-dm-nsprBuild-darwin-385840329d91`js::NativeObject::replaceWithNewEquivalentShape(this=<unavailable>, cx=<unavailable>, oldShape=<unavailable>, newShape=<unavailable>, accessorShape=<unavailable>) + 924 at Shape.cpp:1115
(lldb)
(Assignee)

Comment 3

3 years ago
The freshenblockscope opcode thinks it can clone the ClonedBlockObject that's at the end of the scope chain for each loop iteration, looking solely at that block object.  But when the block object has indeterminate bindings, because of an eval, or a nested function statement, or any of the things that make the scope object indeterminate -- that's not enough.  The block object might contain variable definitions added dynamically.  So we probably need to add the block object's index to freshenblockscope and have it create the clone using that, then copying in values from the existing block on the scope chain.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 4

3 years ago
Created attachment 8586741 [details] [diff] [review]
Patch

It's kinda stupid just how much extra work the previous patch went to, to create a clone and copy into it -- wrongly.  Fortunately the process of modifying the current code to be correct pointed out how it'd just be easier to reuse the real, canonical method to create this stuff.  \o/
Attachment #8586741 - Flags: review?(shu)
(Assignee)

Updated

3 years ago
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED

Updated

3 years ago
Attachment #8586741 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/9a931cd178a7
https://hg.mozilla.org/mozilla-central/rev/e3b60ee3163b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 7

3 years ago
Created attachment 8587779 [details] [diff] [review]
Aurora rollup patch
(Assignee)

Comment 8

3 years ago
Comment on attachment 8587779 [details] [diff] [review]
Aurora rollup patch

Approval Request Comment
[Feature/regressing bug #]: bug 854037
[User impact if declined]: using eval inside a for-loop with a let-declaration in the head will have unexpected behaviors -- I haven't determined what those behaviors are, we should just take this this early in the cycle
[Describe test coverage new/current, TreeHerder]: various testcases landed in the patch
[Risks and why]: very little -- indeed this *simplifies* our current code by reusing an existing, well-used method, making the code even more reliable
[String/UUID change made/needed]: N/A
Attachment #8587779 - Flags: review+
Attachment #8587779 - Flags: approval-mozilla-aurora?
Marking this as unaffected for 38 based on bug 854037.
status-firefox38: --- → unaffected
status-firefox39: --- → affected
Comment on attachment 8587779 [details] [diff] [review]
Aurora rollup patch

Approving for aurora for crash fix.
Attachment #8587779 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/8cef59e11f86
status-firefox39: affected → fixed
You need to log in before you can comment on or make changes to this bug.