Assertion failure: CurrentThreadCanAccessRuntime(cx->runtime()), at js/src/threading/ProtectedData.cpp:65 with off-thread script

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla56
x86_64
Linux
assertion, jsbugmon, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [jsbugmon:update,bisect])

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
The following testcase crashes on mozilla-central revision 1b065ffd8a53 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

var fe = "vv";
for (i = 0; i < 24; i++) fe += fe;
offThreadCompileScript(fe, {});


Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff60fe700 (LWP 30279)]
0x0000000000a8bb88 in js::CheckActiveThread<(js::AllowedHelperThread)0>::check (this=this@entry=0x7ffff695fa30) at js/src/threading/ProtectedData.cpp:65
#0  0x0000000000a8bb88 in js::CheckActiveThread<(js::AllowedHelperThread)0>::check (this=this@entry=0x7ffff695fa30) at js/src/threading/ProtectedData.cpp:65
#1  0x0000000000a04091 in js::ProtectedData<js::CheckActiveThread<(js::AllowedHelperThread)0>, unsigned long>::ref (this=0x7ffff695fa28) at js/src/threading/ProtectedData.h:101
#2  0x00000000009b9b4a in js::ProtectedData<js::CheckActiveThread<(js::AllowedHelperThread)0>, unsigned long>::operator unsigned long const& (this=<optimized out>) at js/src/threading/ProtectedData.h:81
#3  js::gc::MemoryCounter<js::gc::GCRuntime>::maxBytes (this=<optimized out>) at js/src/gc/GCRuntime.h:670
#4  js::gc::GCRuntime::triggerGCForTooMuchMalloc (this=<optimized out>) at js/src/gc/GCRuntime.h:807
#5  js::gc::MemoryCounter<js::gc::GCRuntime>::update (bytes=33554433, owner=0x7ffff695e5f8, this=0x7ffff695fa20) at js/src/gc/GCRuntime.h:664
#6  js::gc::GCRuntime::updateMallocCounter (this=0x7ffff695e5f8, zone=0x7ffff697c000, nbytes=33554433) at js/src/jsgc.cpp:1667
#7  0x0000000000be80c1 in JSRuntime::updateMallocCounter (this=<optimized out>, zone=<optimized out>, nbytes=<optimized out>) at js/src/vm/Runtime.cpp:791
#8  0x000000000047baa7 in JSContext::updateMallocCounter (nbytes=33554433, this=0x7ffff60fd070) at js/src/jscntxt.h:162
#9  js::MallocProvider<JSContext>::pod_malloc<unsigned char> (this=0x7ffff60fd070, numElems=33554433) at js/src/vm/MallocProvider.h:99
#10 0x0000000000c3b87a in NewStringDeflated<(js::AllowGC)0> (cx=0x7ffff60fd070, s=0x7fffe9d00000 u'v' <repeats 200 times>..., n=33554432) at js/src/vm/String.cpp:1224
#11 0x0000000000524ea5 in AtomizeAndCopyChars<char16_t> (cx=0x7ffff60fd070, tbchars=0x7fffe9d00000 u'v' <repeats 200 times>..., length=33554432, pin=js::DoNotPinAtom, indexValue=...) at js/src/jsatom.cpp:403
#12 0x0000000000528d74 in js::AtomizeChars<char16_t> (cx=0x7ffff60fd070, chars=chars@entry=0x7fffe9d00000 u'v' <repeats 200 times>..., length=length@entry=33554432, pin=pin@entry=js::DoNotPinAtom) at js/src/jsatom.cpp:499
#13 0x0000000000d66995 in js::frontend::TokenStream::getTokenInternal (this=0x7ffff60fc958, ttp=0x7ffff60fbd1c, modifier=modifier@entry=js::frontend::Token::Operand) at js/src/frontend/TokenStream.cpp:1465
#14 0x00000000004b0eca in js::frontend::TokenStream::peekToken (this=this@entry=0x7ffff60fc958, ttp=ttp@entry=0x7ffff60fbd1c, modifier=js::frontend::Token::Operand, modifier=js::frontend::Token::Operand) at js/src/frontend/TokenStream.h:781
#15 0x00000000004f0423 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::statementList (this=this@entry=0x7ffff60fc940, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4188
#16 0x00000000004bd56a in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::globalBody (this=0x7ffff60fc940, globalsc=globalsc@entry=0x7ffff60fc340) at js/src/frontend/Parser.cpp:2245
#17 0x0000000000ad4e78 in BytecodeCompiler::compileScript (this=this@entry=0x7ffff60fc390, environment=..., environment@entry=..., sc=sc@entry=0x7ffff60fc340) at js/src/frontend/BytecodeCompiler.cpp:348
#18 0x0000000000ad57c1 in BytecodeCompiler::compileGlobalScript (scopeKind=<optimized out>, this=0x7ffff60fc390) at js/src/frontend/BytecodeCompiler.cpp:394
#19 js::frontend::CompileGlobalScript (cx=<optimized out>, alloc=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, options=..., srcBuf=..., sourceObjectOut=sourceObjectOut@entry=0x7ffff60fcf80) at js/src/frontend/BytecodeCompiler.cpp:589
#20 0x0000000000b888cd in js::ScriptParseTask::parse (this=0x7ffff4332800, cx=<optimized out>) at js/src/vm/HelperThreads.cpp:420
[...]
#27 0x00007ffff6c38b5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x0	0
rbx	0x7ffff695fa28	140737330412072
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7ffff60fb8d0	140737321613520
rsp	0x7ffff60fb8d0	140737321613520
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff60fe700	140737321625344
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff697c000	140737330528256
r13	0x7ffff695fa38	140737330412088
r14	0x7fffe9d00000	140737116110848
r15	0x7fffe9d00000	140737116110848
rip	0xa8bb88 <js::CheckActiveThread<(js::AllowedHelperThread)0>::check() const+72>
=> 0xa8bb88 <js::CheckActiveThread<(js::AllowedHelperThread)0>::check() const+72>:	movl   $0x0,0x0
   0xa8bb93 <js::CheckActiveThread<(js::AllowedHelperThread)0>::check() const+83>:	ud2


Marking s-s as the assertion is known to be (potentially) problematic. Not sure if this applies to the browser though.
jonco: not sure who should be looking at this bug. could you please push it in the right direction?
Flags: needinfo?(jcoppeard)
(Assignee)

Updated

9 months ago
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 2

9 months ago
Created attachment 8888699 [details] [diff] [review]
bug1382431-malloc-trigger

This is tripping up because the max malloc bytes setting is being accessed off main thread when we try to record stats about the trigger.  But we don't actually trigger GC if we are off main thread, so we can just make the access conditional on whether a GC happened.
Assignee: nobody → jcoppeard
Attachment #8888699 - Flags: review?(sphink)
(Assignee)

Comment 3

9 months ago
This isn't a security issue.
Group: javascript-core-security
Comment on attachment 8888699 [details] [diff] [review]
bug1382431-malloc-trigger

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

::: js/src/gc/GCRuntime.h
@@ +810,1 @@
>          stats().recordTrigger(mallocCounter.bytes(), mallocCounter.maxBytes());

Gah, sorry. But can you leave a comment here explaining why it's ok to access mallocCounter?
Attachment #8888699 - Flags: review?(sphink) → review+

Comment 5

9 months ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4d38213053
Only record malloc trigger stats if a GC was triggered r=sfink

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b4d38213053
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Looks like this goes back to the 54 timeframe? Should we consider this for backport or can it ride the 56 train to release?
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 8

9 months ago
The bug causes an assertion failure in debug builds and a possible race condition recording trigger statistics in release builds (with the risk being that the stats are not quite accurate).  I don't think this is serious enough to be worth backporting.
Flags: needinfo?(jcoppeard)
status-firefox54: --- → wontfix
status-firefox55: --- → wontfix
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.