976021 - GenerationalGC: Crash [@ GetGCThingRuntime] with poisoned pointer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central built with --enable-exact-rooting --enable-gcgenerational, revision d9c58306bfbc (run with --fuzzing-safe --ion-eager):


for (;; RegExp = Array(RegExp, [""])) {}

Comment 1

[Christian Holler (:decoder)]

Crash trace:

Program received signal SIGSEGV, Segmentation fault.
GetGCThingRuntime (thing=0x2b2b2b2b) at ../../dist/include/js/HeapAPI.h:133
133         return *reinterpret_cast<JS::shadow::Runtime **>(addr);
(gdb) bt 32
#0  GetGCThingRuntime (thing=0x2b2b2b2b) at ../../dist/include/js/HeapAPI.h:133
#1  isTenured (this=0x2b2b2b2b) at  js/src/gc/Heap.h:1055
#2  js::gc::Cell::arenaHeader (this=0x2b2b2b2b) at  js/src/gc/Heap.h:964
#3  0x080b0817 in tenuredZone (this=0x2b2b2b2b) at  js/src/gc/Heap.h:1024
#4  zone (this=0x2b2b2b2b) at  js/src/gc/Barrier.h:185
#5  js::gc::BarrieredCell<js::ObjectImpl>::zone (this=0xf69fffa0) at  js/src/vm/ObjectImpl.h:1540
#6  0x08191afe in js::GCMarker::processMarkStackTop (this=0x9479278, budget=...) at  js/src/gc/Marking.cpp:1468
#7  0x08157684 in js::GCMarker::drainMarkStack (this=0x9479278, budget=...) at  js/src/gc/Marking.cpp:1554
#8  0x084a8b6e in DrainMarkStack (phase=js::gcstats::PHASE_MARK, sliceBudget=..., rt=0x9478ff8) at  js/src/jsgc.cpp:4067
#9  IncrementalCollectSlice (rt=0x9478ff8, budget=156190872, reason=JS::gcreason::ALLOC_TRIGGER, gckind=js::GC_NORMAL) at  js/src/jsgc.cpp:4627
#10 0x084aad00 in GCCycle (rt=0x9478ff8, incremental=<optimized out>, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at  js/src/jsgc.cpp:4791
#11 0x084ab3df in Collect (rt=0x9478ff8, incremental=<optimized out>, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at  js/src/jsgc.cpp:4929
#12 0x084abaf7 in Collect (reason=JS::gcreason::ALLOC_TRIGGER, gckind=js::GC_NORMAL, budget=0, incremental=true, rt=0x9478ff8) at  js/src/jsgc.cpp:4859
#13 GCSlice (reason=JS::gcreason::ALLOC_TRIGGER, gckind=js::GC_NORMAL, rt=0x9478ff8, millis=<optimized out>) at  js/src/jsgc.cpp:4974
#14 js::gc::GCIfNeeded (cx=0x94855d8) at  js/src/jsgc.cpp:5074
#15 0x083f6f65 in js_InvokeOperationCallback (cx=0x94855d8) at  js/src/jscntxt.cpp:1024
#16 js_HandleExecutionInterrupt (cx=0x94855d8) at  js/src/jscntxt.cpp:1051
#17 0xf66efa63 in ?? ()

Comment 3

[Terrence Cole [:terrence]]

Sorry, wrong bug.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/357895a6aaac
user:        Jon Coppeard
date:        Wed Feb 05 14:09:41 2014 +0000
summary:     Bug 965745 - Always patch loop backedges in Ion if interrupt flag is set r=jandem

This iteration took 369.611 seconds to run.

Comment 5

[Terrence Cole [:terrence]]

The bisected bug only uncovered the pre-existing bug. This is very similar to 975959: barriers are omitted from MCallOptimize's Array call inlining because the new Array will "always" be in the nursery. Sadly this ends up not happening because the interpreter path hits the cache and object allocation from the cache is NoGC, which falls back to the tenured heap -- e.g. bug 919544. I think the right solution here is to finish up the patch on that bug.

Comment 6

[Terrence Cole [:terrence]]

Of course even once we fix the first bug, the MNewArray is initialized with an initial generation based on the type, actively flouting the comment.

Comment 7

[Terrence Cole [:terrence]]

Attachment: fix_new_array_call_with_many_args-v0.diff

Add the requisite barriers. I tested with appropriate printfs to ensure these don't actually get exercised in any octane benchmarks, so performance shouldn't be an issue.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8383356 [details] [diff] [review]
fix_new_array_call_with_many_args-v0.diff

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

Good catch.

::: js/src/jit/MCallOptimize.cpp
@@ +298,5 @@
>  
> +            // There is normally no need for a post barrier on these writes
> +            // because the new array will be in the nursery. However, this
> +            // assumption is volated if we specifically requested pre-tenuring.
> +            if (templateObject->type()->initialHeap(constraints()) == gc::TenuredHeap)

Nit: it's slightly more efficient to do ins->initialHeap() == gc::TenuredHeap, so that we don't potentially add a constraint for each element.

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b9cdf425f7

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/49b9cdf425f7