Closed Bug 1466392 Opened Last year Closed Last year

Assertion failure: !mIsSome, at dist/include/mozilla/Maybe.h:598 (race with evalInWorker and setJitCompilerOption)

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

The following crash happened on mozilla-central revision 8c9263730393 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize):

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00000000007e06f3 in mozilla::Maybe<unsigned int>::emplace<int>(int&&) (this=<optimized out>) at dist/include/mozilla/Maybe.h:598
#1  js::jit::DefaultJitOptions::setEagerCompilation (this=<optimized out>) at js/src/jit/JitOptions.cpp:280
#2  0x0000000000a2d777 in JS_SetGlobalJitCompilerOption (cx=<optimized out>, opt=opt@entry=JSJITCOMPILER_ION_WARMUP_TRIGGER, value=0) at js/src/jsapi.cpp:7249
#3  0x00000000008b94bf in SetJitCompilerOption (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:2780
#4  0x00000000005b7ef1 in js::CallJSNative (cx=0x7fbf21082000, native=0x8b9290 <SetJitCompilerOption(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:274
#5  0x00000000005acf1f in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7fbf21082000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:471
#6  0x00000000005ad27d in InternalCall (cx=0x7fbf21082000, args=...) at js/src/vm/Interpreter.cpp:520
#7  0x00000000005a07bd in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:526
#8  Interpret (cx=0x7fbf21082000, state=...) at js/src/vm/Interpreter.cpp:3122
#9  0x00000000005ac9fd in js::RunScript (cx=0x7fbf21082000, state=...) at js/src/vm/Interpreter.cpp:421
#10 0x00000000005acfc7 in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7fbf21082000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:493
#11 0x00000000005ad27d in InternalCall (cx=0x7fbf21082000, args=...) at js/src/vm/Interpreter.cpp:520
#12 0x00000000005ad400 in js::Call (cx=<optimized out>, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:539
#13 0x000000000098b1be in js::jit::InvokeFunction (cx=<optimized out>, cx@entry=0x7fbf21082000, obj=..., obj@entry=..., constructing=constructing@entry=false, ignoresReturnValue=ignoresReturnValue@entry=false, argc=0, argv=0x7fbf202fcc90, rval=...) at js/src/jit/VMFunctions.cpp:106
#14 0x000000000098b55a in js::jit::InvokeFromInterpreterStub (cx=0x7fbf21082000, frame=0x7fbf202fcc68) at js/src/jit/VMFunctions.cpp:135
#15 0x0000358c37e8b2cf in ?? ()
#16 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fbf22206000	140458888028160
rcx	0x7fbf23e752dd	140458917843677
rdx	0x0	0
rsi	0x7fbf24144770	140458920789872
rdi	0x7fbf24143540	140458920785216
rbp	0x7fbf202fbe30	140458855480880
rsp	0x7fbf202fbdb0	140458855480752
r8	0x7fbf24144770	140458920789872
r9	0x7fbf202ff700	140458855495424
r10	0x0	0
r11	0x0	0
r12	0x7fbf202fbf08	140458855481096
r13	0x1	1
r14	0x0	0
r15	0x1	1
rip	0x7e06f3 <js::jit::DefaultJitOptions::setEagerCompilation()+211>
=> 0x7e06f3 <js::jit::DefaultJitOptions::setEagerCompilation()+211>:	movl   $0x0,0x0
   0x7e06fe <js::jit::DefaultJitOptions::setEagerCompilation()+222>:	ud2


No testcase available for this one, all tests are highly racy. But :jandem mentioned that we don't need a test for this, it is clear from the stack what is going on.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Flags: needinfo?(jdemooij)
Attached patch Patch (obsolete) — Splinter Review
setJitCompilerOption is inherently racy so this patch makes it throw an exception when there's > 1 live runtime.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8983373 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8983373 [details] [diff] [review]
Patch

Actually this can still race I think. Maybe we should move setJitCompilerOption to shell/js.cpp and check for worker threads there...
Attachment #8983373 - Flags: review?(nicolas.b.pierron)
I guess we could replace the Maybe<> classes from the JitOptions, and whitelist all JitOptions members for TSan.
Otherwise we would need a RWLock to prevent SetJitCompilerOption from writing while we have some off-thread compilations running.
Off thread compilations are not the problem, we cancel these in setJitCompilerOption. It's just worker threads created by evalInWorker.

I think we should just move setJitCompilerOption to shell/js.cpp and then make it throw if there are worker threads.
(In reply to Jan de Mooij [:jandem] from comment #6)
> I think we should just move setJitCompilerOption to shell/js.cpp and then
> make it throw if there are worker threads.

If I understand correctly, the problem is coming from JS_SetGlobalJitCompilerOption, which is used for about:config, right?

https://searchfox.org/mozilla-central/search?q=symbol:_Z29JS_SetGlobalJitCompilerOptionP9JSContext19JSJitCompilerOptionj&redirect=false
(In reply to Nicolas B. Pierron [:nbp] {backlog: 36} from comment #7)
> If I understand correctly, the problem is coming from
> JS_SetGlobalJitCompilerOption, which is used for about:config, right?

I think it's okay for JS_SetGlobalJitCompilerOption to be racy - the browser calls that at startup when there is no other runtime and when you go into about:config and change these prefs, a very unlikely crash is not the end of the world.

Fixing that would involve locks around some of the fields in the JitOptions and I don't think that's worth the complexity/overhead/time.
Ok, so the solution from comment 6 sounds good to me.
Is this ready to land on 62?
Flags: needinfo?(jdemooij)
Tracking for 62 to make sure to follow up.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Is this ready to land on 62?

No, we need another approach.

This is an old issue and only affects the JS shell, though. We don't need to track this for 62 IMO.
This is a top crasher in fuzzing, can we get this landed?
Whiteboard: [jsbugmon:] → [jsbugmon:][fuzzblocker]
Attachment #8983373 - Attachment is obsolete: true
Flags: needinfo?(jdemooij)
Attachment #9006204 - Flags: review?(nicolas.b.pierron)
Comment on attachment 9006204 [details] [diff] [review]
Disallow setting JIT compiler options when there are worker threads, to avoid races

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

maybe follow-up: move getJitCompilerOptions[1] to the js shell too.

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN20BaselineStackBuilder14setMonitorStubEPN2js3jit6ICStubE&redirect=false
Attachment #9006204 - Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a325eb7c6a
Disallow setting JIT compiler options when there are worker threads, to avoid races. r=nbp
https://hg.mozilla.org/mozilla-central/rev/78a325eb7c6a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.