1373414 - Crash [@ js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective] or Crash [@ js::StaticStrings::getLength2] or Assertion failure: !hasHelperThreadZones(), at js/src/vm/Runtime.cpp:322

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 035c25bef7b5 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --ion-eager --ion-offthread-compile=off):

evaluate(`
var module = "'use asm';\\n";
for (var i = 0; i < 100; i++) {
    module += "function f" + i + "(i) {\\n";
    module += "  i=i|0; var j=0; j=(i+1)|0; i=(j-4)|0; i=(i+j)|0; return i|0\\n";
    module += "}\\n";
}
var script = "(function() {\\n" + module + "})";
for (var i = 0; i < 10; i++) {
    try {
        offThreadCompileScript(script);
    } catch (e) {}
}
`);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff60fe700 (LWP 11414)]
js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective (this=this@entry=0x7ffff60fcff0, list=list@entry=0x7ffff69dc290, possibleDirective=possibleDirective@entry=0x7ffff69dc2f0, cont=cont@entry=0x7ffff60fb18f) at js/src/frontend/Parser.cpp:4115
#0  js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective (this=this@entry=0x7ffff60fcff0, list=list@entry=0x7ffff69dc290, possibleDirective=possibleDirective@entry=0x7ffff69dc2f0, cont=cont@entry=0x7ffff60fb18f) at js/src/frontend/Parser.cpp:4115
#1  0x00000000004bbeb3 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::statementList (this=0x7ffff60fcff0, yieldHandling=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4204
#2  0x00000000004bd25f in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::functionBody (this=this@entry=0x7ffff60fcff0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, kind=kind@entry=js::frontend::Expression, type=type@entry=js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::StatementListBody) at js/src/frontend/Parser.cpp:2727
#3  0x00000000004bd65a in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::functionFormalParametersAndBody (this=this@entry=0x7ffff60fcff0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, pn=pn@entry=0x7ffff69dc050, kind=kind@entry=js::frontend::Expression, parameterListEnd=..., isStandaloneFunction=false) at js/src/frontend/Parser.cpp:3801
#4  0x00000000004adc25 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::innerFunction (this=this@entry=0x7ffff60fcff0, pn=pn@entry=0x7ffff69dc050, outerpc=outerpc@entry=0x7ffff60fc610, funbox=funbox@entry=0x7ffff69dc1d0, toStringStart=toStringStart@entry=1, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::Expression, inheritedDirectives=..., newDirectives=0x7ffff60fb700) at js/src/frontend/Parser.cpp:3586
[...]
#27 0x00000000004930a2 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::globalBody (this=0x7ffff60fcff0, globalsc=0x7ffff60fca80) at js/src/frontend/Parser.cpp:2245
#28 0x00000000008e14c5 in BytecodeCompiler::compileScript (this=this@entry=0x7ffff60fcad0, environment=..., environment@entry=..., sc=sc@entry=0x7ffff60fca80) at js/src/frontend/BytecodeCompiler.cpp:348
#29 0x00000000008e1ce0 in BytecodeCompiler::compileGlobalScript (scopeKind=js::ScopeKind::Global, this=0x7ffff60fcad0) at js/src/frontend/BytecodeCompiler.cpp:394
#30 js::frontend::CompileGlobalScript (cx=<optimized out>, alloc=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, options=..., srcBuf=..., sourceObjectOut=sourceObjectOut@entry=0x7ffff60fd5c0) at js/src/frontend/BytecodeCompiler.cpp:589
#31 0x00000000009480c7 in js::ScriptParseTask::parse (this=0x7ffff69e5850, cx=<optimized out>) at js/src/vm/HelperThreads.cpp:408
#32 0x000000000094e832 in js::HelperThread::handleParseWorkload (this=this@entry=0x7ffff6966180, locked=...) at js/src/vm/HelperThreads.cpp:1890
#33 0x000000000094fb0b in js::HelperThread::threadLoop (this=0x7ffff6966180) at js/src/vm/HelperThreads.cpp:2152
#34 0x000000000095689a in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0ul> (this=0x7ffff691e0a0) at js/src/threading/Thread.h:234
#35 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0x7ffff691e0a0) at js/src/threading/Thread.h:227
#36 0x00007ffff7bc16fa in start_thread (arg=0x7ffff60fe700) at pthread_create.c:333
#37 0x00007ffff6c38b5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x0	0
rbx	0x0	0
rcx	0x17	23
rdx	0x7ffff69dc2f0	140737330922224
rsi	0x7ffff69dc290	140737330922128
rdi	0x7ffff60fcff0	140737321619440
rbp	0x0	0
rsp	0x7ffff60fb150	140737321611600
r8	0xe	14
r9	0x7	7
r10	0x7ffff5227320	140737306063648
r11	0x17	23
r12	0x0	0
r13	0x7ffff60fcff0	140737321619440
r14	0x7ffff69dc290	140737330922128
r15	0x7ffff60fd008	140737321619464
rip	0x496226 <js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective(js::frontend::ParseNode*, js::frontend::ParseNode*, bool*)+118>
=> 0x496226 <js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective(js::frontend::ParseNode*, js::frontend::ParseNode*, bool*)+118>:	cmp    %r10,0xb60(%rax)
   0x49622d <js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective(js::frontend::ParseNode*, js::frontend::ParseNode*, bool*)+125>:	je     0x496260 <js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::maybeParseDirective(js::frontend::ParseNode*, js::frontend::ParseNode*, bool*)+176>


I'm marking this s-s because the unreduced crash crashed at a non-zero address (0x1e00) which looks suspicious to me.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Shu, can you take a look at this please?

Comment 2

[Shu-yu Guo [:shu]]

This is caused by bug 1371216. The bug is that handleWasmIdleWorkload mutates a HelperThread's currentTask from a ParseTask to a wasm::CompileTask. This breaks the ability for the outer ParseTask to be correctly cancelled. Since CancelOffThreadParses waits until there are no pending ParseTasks and there no more threads that have a ParseTask as their currentTask, it's possible that the waiting loop to complete because a thread's currentTask has temporarily been mutated into a wasm::CompileTask.

This can be fixed by having CancelOffThreadParses also wait on wasm::CompileTasks, but it feels dirty. Lars, what do you think?

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Christian Holler (:decoder) from comment #0)

> I'm marking this s-s because the unreduced crash crashed at a non-zero
> address (0x1e00) which looks suspicious to me.

That's because there's a race with shutdown, and a helper thread is trying to access a field on a nulled-out JSRuntime pointer. This particular case isn't s-s, but this kind of shutdown race is very scary so let's keep it as a s-s bug.

Comment 4

[Luke Wagner [:luke]]

If there's not a cleaner route, another option is to disable workstealing for asm.js (keeping it for wasm which is never running in a parse task).

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #4)
> If there's not a cleaner route, another option is to disable workstealing
> for asm.js (keeping it for wasm which is never running in a parse task).

I like this option.

Comment 6

[Lars T Hansen [:lth]]

Yeah, I'll look into this today.  Luke's suggestion should be sufficient to avoid the deadlock-from-no-available-workers problem I was trying to avoid, but I'll need to check the logic carefully.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1373414-see-suspended-tasks.patch

Disabling work stealing on the asm.js thread would probably work: In that case, the parse/modulegenerator thread would need a guarantee that there is one other thread available to actually compile wasm code.  Since the CompileTier2Task will perform work stealing, it will eventually make progress, and a thread will eventually become available to do the compilation on behalf of the parse task.

I think the attached fix is a hair more elegant since it keeps the logic within the HelperThreads subsystem: it accounts for the situation where a thread may have a suspended task, and checks that case too (with one extra line of logic).  Since the circumstances for work stealing are limited, this will be a very local fix.  My pending patch for work stealing on tier-2 compilation will need a similar fix in its cancellation code.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8880421 [details] [diff] [review]
bug1373414-see-suspended-tasks.patch

Review of attachment 8880421 [details] [diff] [review]:
-----------------------------------------------------------------

I'm worried about how leaky this ad-hoc 1-tier workstealing logic is. In particular, for every task type Foo with a getter |fooTask()| that can be suspended, we need to audit the callsites |fooTask()| to check if it also needs to check |suspendedFooTask()|. This patch fixes CancelOffThreadParses, but at least JSContext::{addPendingCompileError, addPendingOverRecursed, addPendingOutOfMemory} need to also be fixed. Plus, this will be something that's really easy to forget to check when reviewing.

For now, let's add asserts that only ParseTasks can be suspended, because we've audited all the callsites of parseTask(). Adding capability for suspending new task types need to be thought through carefully.

::: js/src/vm/HelperThreads.cpp
@@ +1688,5 @@
>      if (HelperThreadState().canStartWasmCompile(locked, /*assumeThreadAvailable=*/ true)) {
> +        MOZ_ASSERT(suspendedTask.isNothing());
> +
> +        suspendedTask = Some(currentTask.value());
> +        suspendedThreadType = (js::oom::ThreadType)js::oom::GetThreadType();

Could use an AutoSuspendTask RAII thing.

Comment 9

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision c01aa84ded7e).
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/961e782cf7c7
user:        Andrew Osmond
date:        Tue Jun 13 15:19:36 2017 -0400
summary:     Bug 1365927 - Part 2. Add minimal PCompositorManager protocol. r=dvander

This iteration took 229.005 seconds to run.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The regression range suggested is likely inaccurate, as this testcase may be intermittent.

Comment 11

[Lars T Hansen [:lth]]

> For now, let's add asserts that only ParseTasks can be suspended, ...

This turns out not to be true; for wasm compilation, the parent task will be a PromiseTask.

> Adding capability for suspending new task types need to be thought through carefully.

Yeah...

Comment 12

[Lars T Hansen [:lth]]

Attachment: bug1373414-see-suspended-tasks-v2.patch

Updated as requested, apart from also handling a PromiseTask.  I grubbed through the code and did not see any place where we would have to recognize a suspended PromiseTask, but it would be useful for you to perform that exercise independently, probably.

Comment 13

[Lars T Hansen [:lth]]

Comment on attachment 8882439 [details] [diff] [review]
bug1373414-see-suspended-tasks-v2.patch

Cancelling review because I'm running into an assert.  Will revisit for real next week.

Comment 14

[Lars T Hansen [:lth]]

I talked to Shu about this at the all-hands.  I think we should back out my original patch, because the work stealing / task replacement is brittle, and it will remain brittle.

A less brittle situation is achieved by making the HelperThread system aware of the problem with deadlocks from starvation.  The situation we have is that a "manager" thread holds onto one of the available threads and depends on at least one other "worker" thread being available to do the grunt work.  If there is at most one manager thread then we need at least two threads in total; if there are two manager threads then we need at least three threads in total, etc.  The more general, and less brittle solution, is therefore not to deplete the pool of available worker threads, but to delay starting a manager if it would take the last available worker thread.  (Or perhaps better, there must be one worker thread available for every manager thread running.)

It seems that we should be able to perform the necessary scheduling by passing a flag with each task that is created (this is really a static attribute of the task type), designating it as "manager" (needs other worker threads and will block waiting for them) or "worker" (will not block on other threads).  This is probably related to the current CONSUMER / PRODUCER designation but I don't know how deep that goes.

I'll offer a patch that backs out the previous work.

Comment 15

[Lars T Hansen [:lth]]

Attachment: bug1373414-backout-wasm-work-stealing.patch

Simply the reverse of the patch in bug 1371216.

Comment 16

[Lars T Hansen [:lth]]

I don't think there's any reason this needs to be flagged as s-s.  Can we open up?

Comment 17

[Lars T Hansen [:lth]]

Also, looks like this did not make it onto the mozilla-beta branch.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Opening up as per comment 16.

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8884459 [details] [diff] [review]
bug1373414-backout-wasm-work-stealing.patch

Review of attachment 8884459 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/HelperThreads.h
@@ -350,5 @@
> -                         PromiseTask*,
> -                         ParseTask*,
> -                         SourceCompressionTask*,
> -                         GCHelperState*,
> -                         GCParallelTask*> HelperTaskUnion;

This is just a backout, but it'd be nice to keep in this refactoring, if you're so inclined to do the extra typing.

Comment 20

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95043a5e0f9732453d04812b8b6bb16e7e643ee8
Bug 1373414 - Backout wasm work stealing on JS helper threads. r=shu

Comment 21

[Lars T Hansen [:lth]]

Landed with suggested change.  We'll land the revamp discussed in comment 14 on a different bug.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95043a5e0f97

Comment 23

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.