Closed
Bug 1286244
Opened 8 years ago
Closed 8 years ago
Assertion failure: !shadowZone->needsIncrementalBarrier(), at js/src/gc/Heap.h:1295
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: decoder, Assigned: terrence)
Details
(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update])
Attachments
(1 file, 1 obsolete file)
6.52 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision aac8ff1024c5 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe): verifyprebarriers(); var lfGlobal = newGlobal(); lfGlobal.offThreadCompileScript(` version(185); `); Backtrace: received signal SIGSEGV, Segmentation fault. #0 0x0000000000506c62 in js::gc::TenuredCell::readBarrier (thing=0x7ffff3f7e7a8) at js/src/gc/Heap.h:1294 #1 JSString::readBarrier (thing=0x7ffff3f7e7a8) at js/src/vm/String.h:519 #2 js::AtomStateEntry::asPtr (this=0x7ffff68fd608) at js/src/jsatominlines.h:25 #3 js::AtomHasher::match (entry=..., lookup=...) at js/src/jsatominlines.h:171 #4 0x00000000005089dc in js::detail::HashTable<js::AtomStateEntry const, js::HashSet<js::AtomStateEntry, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::match (l=..., e=...) at dist/include/js/HashTable.h:1316 #5 js::detail::HashTable<js::AtomStateEntry const, js::HashSet<js::AtomStateEntry, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::lookup (collisionBit=1, keyHash=3288026198, l=..., this=0x7ffff3cfece0) at dist/include/js/HashTable.h:1343 #6 js::detail::HashTable<js::AtomStateEntry const, js::HashSet<js::AtomStateEntry, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::lookupForAdd (l=..., this=0x7ffff3cfece0) at dist/include/js/HashTable.h:1699 #7 js::HashSet<js::AtomStateEntry, js::AtomHasher, js::SystemAllocPolicy>::lookupForAdd (l=..., this=<optimized out>) at dist/include/js/HashTable.h:389 #8 AtomizeAndCopyChars<char16_t> (pin=js::DoNotPinAtom, length=7, tbchars=0x7ffff69aa682 u"version(185);\n", ' ' <repeats 28 times>, cx=0x7ffff3cf7560) at js/src/jsatom.cpp:331 #9 js::AtomizeChars<char16_t> (cx=0x7ffff3cf7560, chars=chars@entry=0x7ffff69aa682 u"version(185);\n", ' ' <repeats 28 times>, length=length@entry=7, pin=pin@entry=js::DoNotPinAtom) at js/src/jsatom.cpp:427 #10 0x0000000000d12acd in js::frontend::TokenStream::getTokenInternal (this=0x7ffff68fe3f0, ttp=0x7ffff68fd7ec, modifier=modifier@entry=js::frontend::Token::Operand) at js/src/frontend/TokenStream.cpp:1238 #11 0x00000000004950da in js::frontend::TokenStream::peekToken (this=this@entry=0x7ffff68fe3f0, ttp=ttp@entry=0x7ffff68fd7ec, modifier=js::frontend::Token::Operand, modifier=js::frontend::Token::Operand) at js/src/frontend/TokenStream.h:569 #12 0x00000000004d444f in js::frontend::Parser<js::frontend::FullParseHandler>::statements (this=this@entry=0x7ffff68fe3c0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3555 #13 0x00000000004a30a0 in js::frontend::Parser<js::frontend::FullParseHandler>::globalBody (this=0x7ffff68fe3c0) at js/src/frontend/Parser.cpp:1113 #14 0x0000000000ceb6e7 in BytecodeCompiler::compileScript (this=this@entry=0x7ffff68fdd50, scopeChain=..., scopeChain@entry=..., evalCaller=..., evalCaller@entry=...) at js/src/frontend/BytecodeCompiler.cpp:531 #15 0x0000000000cebde7 in js::frontend::CompileScript (cx=<optimized out>, alloc=alloc@entry=0x7ffff69e98c8, scopeChain=scopeChain@entry=..., enclosingStaticScope=..., enclosingStaticScope@entry=..., evalCaller=evalCaller@entry=..., options=..., srcBuf=..., source_=0x0, extraSct=0x0, sourceObjectOut=0x7ffff69e9970) at js/src/frontend/BytecodeCompiler.cpp:742 #16 0x0000000000aceffe in js::ScriptParseTask::parse (this=0x7ffff69e97e0) at js/src/vm/HelperThreads.cpp:277 #17 0x0000000000ad944e in js::HelperThread::handleParseWorkload (this=this@entry=0x7ffff6948000, locked=...) at js/src/vm/HelperThreads.cpp:1527 #18 0x0000000000adbbd1 in js::HelperThread::threadLoop (this=0x7ffff6948000) at js/src/vm/HelperThreads.cpp:1717 #19 0x0000000000b0a8e1 in nspr::Thread::ThreadRoutine (arg=0x7ffff693c060) at js/src/vm/PosixNSPR.cpp:45 #20 0x00007ffff7bc16fa in start_thread (arg=0x7ffff68ff700) at pthread_create.c:333 #21 0x00007ffff6c38b5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109 rax 0x0 0 rbx 0x7ffff6962000 140737330421760 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7ffff68fd550 140737330009424 rsp 0x7ffff68fd500 140737330009344 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff68ff700 140737330018048 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff3f7e7a8 140737286498216 r13 0x7ffff68fd600 140737330009600 r14 0x7ffff3f00000 140737285980160 r15 0x7ffff3ffffe8 140737287028712 rip 0x506c62 <js::AtomHasher::match(js::AtomStateEntry const&, js::AtomHasher::Lookup const&)+834> => 0x506c62 <js::AtomHasher::match(js::AtomStateEntry const&, js::AtomHasher::Lookup const&)+834>: movl $0x0,0x0 0x506c6d <js::AtomHasher::match(js::AtomStateEntry const&, js::AtomHasher::Lookup const&)+845>: ud2 This bug is trigger-happy, so marking as fuzzblocker. Please fix this as soon as possible.
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
Comment 1•8 years ago
|
||
JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/0ca871e39a20 user: Jan de Mooij date: Wed Jun 22 09:47:52 2016 +0200 summary: Bug 1279295 - Create the runtime's JSContext when we create the runtime. r=luke This iteration took 226.200 seconds to run.
Updated•8 years ago
|
Flags: needinfo?(jdemooij)
Comment 2•8 years ago
|
||
Bug 1279295 + offThreadCompileScript has been a can of worms. I'll take a look.
Comment 3•8 years ago
|
||
This is caused by the pre-barrier verifier + off-thread parsing issue we fixed a few days ago. The parse thread is failing this assert in TenuredCell::readBarrier: JS::shadow::Zone* shadowZone = thing->shadowZoneFromAnyThread(); MOZ_ASSERT_IF(!CurrentThreadCanAccessRuntime(thing->runtimeFromAnyThread()), !shadowZone->needsIncrementalBarrier()); Terrence, what do you think we should do? At first I thought TenuredCell::readBarrier should just return if we're verifying barriers, but that will probably cause verifier failures?
Flags: needinfo?(jdemooij) → needinfo?(terrence)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > This is caused by the pre-barrier verifier + off-thread parsing issue we > fixed a few days ago. The parse thread is failing this assert in > TenuredCell::readBarrier: > > JS::shadow::Zone* shadowZone = thing->shadowZoneFromAnyThread(); > > MOZ_ASSERT_IF(!CurrentThreadCanAccessRuntime(thing->runtimeFromAnyThread()), > !shadowZone->needsIncrementalBarrier()); > > Terrence, what do you think we should do? At first I thought > TenuredCell::readBarrier should just return if we're verifying barriers, but > that will probably cause verifier failures? Actually, looking at the full backtrace, AtomHasher::match should be using unbarriered access: we don't want lookups to increase the lifetime of everything that the table happens to incidentally compare against. I have this exact hunk in my patches for bug 844769. Unfortunately, that patch series is red on TH; however, I think that a more targeted fix will work fine here. Let me cut a patch.
Flags: needinfo?(terrence)
Assignee | ||
Comment 5•8 years ago
|
||
This works by adding a second race to readBarriered. Not great, but I can easily fold in a backout in bug 844769. I guess I'll prioritize that bug next.
Comment 6•8 years ago
|
||
Comment on attachment 8770671 [details] [diff] [review] fuzz_1286244-v0.diff Review of attachment 8770671 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Heap.h @@ +1299,2 @@ > return; > + } Can you do this as two separate "if(...) return;" chunks, so you have a natural place to add a comment as to wtf is going on here? Because I for one don't know what you're talking about. This change reads something like "if we're verifying prebarriers, then just skip read barriers entirely because they'll crash." I'm sure there's a good reason that makes sense and why it's ok to skip the barrier, but I don't know what it is. Help?
Comment 7•8 years ago
|
||
Comment on attachment 8770671 [details] [diff] [review] fuzz_1286244-v0.diff Review of attachment 8770671 [details] [diff] [review]: ----------------------------------------------------------------- Oh, wait. Is this because IsVerifyPreBarriersEnabled really "is prebarrier verification currently running"? Then this might make some sense.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #7) > Comment on attachment 8770671 [details] [diff] [review] > fuzz_1286244-v0.diff > > Review of attachment 8770671 [details] [diff] [review]: > ----------------------------------------------------------------- > > Oh, wait. Is this because IsVerifyPreBarriersEnabled really "is prebarrier > verification currently running"? Then this might make some sense. Yes, it is.
Comment 9•8 years ago
|
||
Comment on attachment 8770671 [details] [diff] [review] fuzz_1286244-v0.diff Review of attachment 8770671 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Heap.h @@ +1301,1 @@ > Ok. I still want the comment. And ditch the #ifdef JS_GC_ZEAL; GCRuntime::isVerify... already handles that. Any chance of s/isVerifyPreBarriersEnabled/isVerifyingPreBarriers/? Or isHeapVerifyingPreBarriers? (Not in this patch; I'm fine with just landing it as a minimal stopgap.)
Attachment #8770671 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8770671 [details] [diff] [review] fuzz_1286244-v0.diff Review of attachment 8770671 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Heap.h @@ +1301,1 @@ > I can do two separate ifs, but I think the #ifdef JS_GC_ZEAL has to stay: this is incredibly hot code and I'm not sure the compiler is going to be able to eliminate it in all cases.
Assignee | ||
Comment 11•8 years ago
|
||
Note that I also tried to update the pre-barrier verifier to not enable needsIncrementalBarriers on background threads to better match our actual usage. This is not sufficient because of the atoms zone.
Assignee | ||
Comment 12•8 years ago
|
||
Actually, if the atoms zone is the only thing that is problematic after fixing the pre-barrier verifier, then we should be able to import just the atoms bits of bug 844769 to prevent the bad cases. And this appears to work.
Attachment #8770671 -
Attachment is obsolete: true
Attachment #8770710 -
Flags: review?(sphink)
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af42d0fed15
Comment 14•8 years ago
|
||
Comment on attachment 8770710 [details] [diff] [review] fuzz_1286244-v1.diff Review of attachment 8770710 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsatominlines.h @@ +22,4 @@ > { > + JSAtom* atom = asPtrUnbarriered(); > + if (cx->isJSContext()) > + JSString::readBarrier(atom); I looked for a bit for comments explaining this. First, why do atoms need a read barrier at all? The natural place to look would be http://dxr.mozilla.org/mozilla-central/source/js/src/gc/Barrier.h#557 but that is surprisingly specific to empty shapes. The best description I found was bug 720853 comment 0. I'm not sure where to put that as a comment -- it could go on AtomStateEntry's asPtr() definition (since it comes along with asPtrUnbarriered()). Though it seems like it ought to be connected to JSAtom? Which confusingly is not in jsatom.h, but at <http://dxr.mozilla.org/mozilla-central/source/js/src/vm/String.h#968>. Which isn't so bad, though I wonder if we ought to have a forwarding (doc) pointer from jsatom.h to vm/String.h. Second, why do non-mainthread JSContexts not need this barrier? http://dxr.mozilla.org/mozilla-central/source/js/src/jscntxt.h#78 discusses but does not answer it. I could speculate, but I'm not confident I know the exact answer. Is there a one-liner you could add here? (This sort of thing shows up enough places that I feel like I *ought* to know the answer.)
Attachment #8770710 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #14) > Comment on attachment 8770710 [details] [diff] [review] > fuzz_1286244-v1.diff > > Review of attachment 8770710 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsatominlines.h > @@ +22,4 @@ > > { > > + JSAtom* atom = asPtrUnbarriered(); > > + if (cx->isJSContext()) > > + JSString::readBarrier(atom); > > I looked for a bit for comments explaining this. > > First, why do atoms need a read barrier at all? They are weak pointers and that is how we ensure alive-at-the-beginning for weak pointers? I don't think there's a more complicated answer than that. We have atom pinning and AutoKeepAtoms, but those are not always enabled and when they aren't, the atoms table is just a normal WeakCache. > The natural place to look > would be > http://dxr.mozilla.org/mozilla-central/source/js/src/gc/Barrier.h#557 but > that is surprisingly specific to empty shapes. Wow, yeah, that comment is more than a little out of date. To be fair, I think that was written by Bill right when he discovered the issue; at the time we had no idea that we'd need more than one or two ReadBarriers in the entire system. I'll update that. > The best description I found > was bug 720853 comment 0. I'm not sure where to put that as a comment -- it > could go on AtomStateEntry's asPtr() definition (since it comes along with > asPtrUnbarriered()). Though it seems like it ought to be connected to > JSAtom? Yeah, this isn't an atom specific issue. I think the comment should actually be in the large block comment at the top of Barrier.h where we discuss the IGC invariants. > Which confusingly is not in jsatom.h, but at > <http://dxr.mozilla.org/mozilla-central/source/js/src/vm/String.h#968>. > Which isn't so bad, though I wonder if we ought to have a forwarding (doc) > pointer from jsatom.h to vm/String.h. Not for this bug. > Second, why do non-mainthread JSContexts not need this barrier? Well, trivially, marking from OMT would be hella racy, so they better not need a barrier! The reason we don't need a barrier is that each zone has its own list of arenas it allocates out of and we don't collect zones that are being used OMT. If we want to do a full collection while there are OMT compiles going on, we have to cancel them first. > http://dxr.mozilla.org/mozilla-central/source/js/src/jscntxt.h#78 discusses > but does not answer it. I could speculate, but I'm not confident I know the > exact answer. Is there a one-liner you could add here? (This sort of thing > shows up enough places that I feel like I *ought* to know the answer.) This also seems like it needs an architectural doc somewhere, although I'm not sure either what the right place is.
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9c26f8bb9d599e80c92f6a7f30ad91bd54a854 Bug 1286244 - Allow OMT parse to work when the pre-barrier verifier is running; r=sfink
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a9c26f8bb9d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•