1340537 - Assertion failure: group->ownedByCurrentThread(), at js/src/threading/ProtectedData.cpp:67

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 6cefe01ca774 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe min.js):

gczeal(15, 5);
offThreadCompileScript("");
gc();


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0864678d in js::CheckZoneGroup<(js::AllowedHelperThread)3>::check (this=0xf7958ba0) at js/src/threading/ProtectedData.cpp:67
#0  0x0864678d in js::CheckZoneGroup<(js::AllowedHelperThread)3>::check (this=0xf7958ba0) at js/src/threading/ProtectedData.cpp:67
#1  0x0897ab5d in js::ProtectedData<js::CheckZoneGroup<(js::AllowedHelperThread)3>, unsigned int>::ref (this=0xf7958b9c) at js/src/threading/ProtectedData.h:109
#2  js::ProtectedData<js::CheckZoneGroup<(js::AllowedHelperThread)3>, unsigned int>::operator unsigned int const& (this=0xf7958b9c) at js/src/threading/ProtectedData.h:81
#3  js::ObjectGroup::needsSweep (this=0xf14840e8, this@entry=0xf7958800) at js/src/vm/ObjectGroup-inl.h:19
#4  js::ObjectGroup::maybeSweep (this=this@entry=0xf14840e8, oom=0x0) at js/src/vm/ObjectGroup-inl.h:25
#5  0x08986c31 in js::ObjectGroup::flags (this=0xf14840e8) at js/src/vm/ObjectGroup-inl.h:32
#6  js::ObjectGroup::basePropertyCount (this=0xf14840e8) at js/src/vm/TypeInference-inl.h:1058
#7  js::ObjectGroup::getPropertyCount (this=0xf14840e8) at js/src/vm/TypeInference-inl.h:1134
#8  js::ObjectGroup::traceChildren (this=0xf14840e8, trc=0xffffc958) at js/src/gc/Marking.cpp:1402
#9  0x089c190d in TraceChildrenFunctor::operator()<js::ObjectGroup> (this=<synthetic pointer>, thing=<optimized out>, trc=<optimized out>) at js/src/gc/Tracer.cpp:117
#10 JS::DispatchTraceKindTyped<TraceChildrenFunctor, JSTracer*&, void*&> (f=..., traceKind=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/arm/compiler/gcc/sanitizer/none/type/debug/dist/include/js/TraceKind.h:188
#11 0x089b0f6b in js::TraceChildren (kind=<optimized out>, thing=0xf14840e8, trc=0xffffc958) at js/src/gc/Tracer.cpp:126
#12 JS::TraceChildren (trc=0xffffc958, thing=...) at js/src/gc/Tracer.cpp:111
#13 0x089b1146 in CheckHeapTracer::check (this=0xffffc954, lock=...) at js/src/gc/Verifier.cpp:548
#14 0x089b1488 in js::gc::CheckHeapAfterGC (rt=0xf7943000) at js/src/gc/Verifier.cpp:569
#15 0x085943b1 in js::gc::GCRuntime::collect (this=0xf7943380, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::API) at js/src/jsgc.cpp:6485
#16 0x08594564 in js::gc::GCRuntime::gc (this=0xf7943380, gckind=GC_NORMAL, reason=JS::gcreason::API) at js/src/jsgc.cpp:6515
#17 0x085945c1 in JS::GCForReason (cx=<optimized out>, gckind=<optimized out>, reason=<optimized out>) at js/src/jsgc.cpp:7380
#18 0x08457fda in GC (cx=0xf7947800, argc=0, vp=0xf104d058) at js/src/builtin/TestingFunctions.cpp:321
#19 0x0816daf0 in js::CallJSNative (cx=0xf7947800, native=0x8457e70 <GC(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:281
[...]
#33 0x080777c7 in main (argc=3, argv=0xffffd8d4, envp=0xffffd8e4) at js/src/shell/js.cpp:8166
eax	0x0	0
ebx	0x8cadff4	147513332
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0xf7947800	-141264896
edi	0xf7958800	-141195264
ebp	0xffffc7c8	4294952904
esp	0xffffc7c0	4294952896
eip	0x864678d <js::CheckZoneGroup<(js::AllowedHelperThread)3>::check() const+189>
=> 0x864678d <js::CheckZoneGroup<(js::AllowedHelperThread)3>::check() const+189>:	movl   $0x0,0x0
   0x8646797 <js::CheckZoneGroup<(js::AllowedHelperThread)3>::check() const+199>:	ud2    


This could be shell only but I'm not sure. Marking s-s due to GC involved.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

GlobalHelperThreadState::trace() traces parse tasks which are not in active use by a helper thread, but the zone groups for these tasks may have the helper thread bit set which means the tracing thread needs active ownership of the parse task zone to satisfy the protected data assertions.  Since these zone groups are ignored entirely by the GC it seems best to just never trace anything in them.

I'm not 100% sure this is the correct fix, but in any case there is no race or other security issue here.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8839122 [details] [diff] [review]
patch

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

::: js/src/vm/HelperThreads.cpp
@@ +350,5 @@
>  void
>  ParseTask::trace(JSTracer* trc)
>  {
> +    if (parseGlobal->runtimeFromAnyThread() != trc->runtime() ||
> +        parseGlobal->zoneFromAnyThread()->usedByHelperThread())

Could you add an assertion that if the zone is used by a helper thread then it is not being collected?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eca5587da3e
Don't trace parse task things if they cannot be GCed, r=jonco.

Comment 4

[Carsten Book [:Tomcat]]

had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=78801950&repo=mozilla-inbound

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be761d783526
Backed out changeset 6eca5587da3e on request from jonco

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8839122 [details] [diff] [review]
patch

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

::: js/src/vm/HelperThreads.cpp
@@ +350,5 @@
>  void
>  ParseTask::trace(JSTracer* trc)
>  {
> +    if (parseGlobal->runtimeFromAnyThread() != trc->runtime() ||
> +        parseGlobal->zoneFromAnyThread()->usedByHelperThread())

You need to do MaybeForwarded(parseGlobal)->zoneFromAnyThread here since we may be moving this object and parseGlobal doesn't get updated until we trace it below.

This doesn't matter for calling runtimeFromAnyThread because that doesn't access the contents of the object but relies in the runtime pointer stored in the chunk trailer, which is still valid.

Sorry I didn't spot this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd59724f67f
Don't trace parse task things if they cannot be GCed, r=jonco.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1dd59724f67f