Closed Bug 1286244 Opened 4 years ago Closed 4 years ago

Assertion failure: !shadowZone->needsIncrementalBarrier(), at js/src/gc/Heap.h:1295

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: decoder, Assigned: terrence)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
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.
Flags: needinfo?(jdemooij)
Bug 1279295 + offThreadCompileScript has been a can of worms. I'll take a look.
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)
(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)
Attached patch fuzz_1286244-v0.diff (obsolete) — Splinter Review
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.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8770671 - Flags: review?(sphink)
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 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.
(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 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+
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.
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.
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)
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+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/5a9c26f8bb9d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.