Closed Bug 1340537 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed

People

(Reporter: decoder, Assigned: bhackett1024)

Details

(4 keywords, Whiteboard: [jsbugmon:update,bisect])

Attachments

(1 file)

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.
Flags: needinfo?(bhackett1024)
Attached patch patchSplinter Review
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.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8839122 - Flags: review?(jcoppeard)
Group: javascript-core-security
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?
Attachment #8839122 - Flags: review?(jcoppeard) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eca5587da3e
Don't trace parse task things if they cannot be GCed, r=jonco.
had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=78801950&repo=mozilla-inbound
Flags: needinfo?(bhackett1024)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be761d783526
Backed out changeset 6eca5587da3e on request from 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.
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd59724f67f
Don't trace parse task things if they cannot be GCed, r=jonco.
https://hg.mozilla.org/mozilla-central/rev/1dd59724f67f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.