830042 - IonMonkey: Assertion failure: v->toGCThing(), at gc/Marking.cpp:488 or Opt-crash [@ js::gc::MarkKind]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 1761f4a9081c (run with --ion-eager):


function f() {
    var o1 = { a: 5 };
    for (var i = 0; i < 6; - i) {
	var o = i % 2 ? o2 : o1;
	("").a = i;
    }
}
f()

Comment 1

[Christian Holler (:decoder)]

Debug backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x083dd740 in MarkValueInternal (trc=0x894aefc, v=0xf76970c0) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:488
488             JS_ASSERT(v->toGCThing());
(gdb) bt
#0  0x083dd740 in MarkValueInternal (trc=0x894aefc, v=0xf76970c0) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:488
#1  0x083ddb19 in js::gc::MarkValueRootRange (trc=0x894aefc, len=3, vec=0xf76970b0, name=0x86d7df5 "vm_stack") at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:548
#2  0x083325a5 in js::gc::MarkValueRootRange (trc=0x894aefc, begin=0xf76970b0, end=0xf76970c8, name=0x86d7df5 "vm_stack") at ../gc/Marking.h:175
#3  0x08334959 in js::StackSpace::markFrame (this=0x894ad84, trc=0x894aefc, fp=0xf7697070, slotsEnd=0xf76970c8) at /srv/repos/mozilla-central/js/src/vm/Stack.cpp:646
#4  0x083349c6 in js::StackSpace::mark (this=0x894ad84, trc=0x894aefc) at /srv/repos/mozilla-central/js/src/vm/Stack.cpp:668
#5  0x083d8d54 in js::gc::MarkRuntime (trc=0x894aefc, useSavedRoots=false) at /srv/repos/mozilla-central/js/src/gc/RootMarking.cpp:771
#6  0x08129902 in BeginMarkPhase (rt=0x894ad30) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2634
#7  0x0812d895 in IncrementalCollectSlice (rt=0x894ad30, budget=0, reason=js::gcreason::TOO_MUCH_MALLOC, gckind=js::GC_NORMAL) at /srv/repos/mozilla-central/js/src/jsgc.cpp:3973
#8  0x0812dfe2 in GCCycle (rt=0x894ad30, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=js::gcreason::TOO_MUCH_MALLOC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4160
#9  0x0812e413 in Collect (rt=0x894ad30, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=js::gcreason::TOO_MUCH_MALLOC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4278
#10 0x0812e5fb in js::GCSlice (rt=0x894ad30, gckind=js::GC_NORMAL, reason=js::gcreason::TOO_MUCH_MALLOC, millis=0) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4316
#11 0x080dd181 in js_InvokeOperationCallback (cx=0x8971e20) at /srv/repos/mozilla-central/js/src/jscntxt.cpp:1058
#12 0x080dd1ea in js_HandleExecutionInterrupt (cx=0x8971e20) at /srv/repos/mozilla-central/js/src/jscntxt.cpp:1082
#13 0x0862d9c5 in js::mjit::stubs::Interrupt (f=..., pc=0x899e247 "mV") at /srv/repos/mozilla-central/js/src/methodjit/StubCalls.cpp:801
#14 0xf7fcc334 in ?? ()
#15 0x08912ff4 in ?? ()


S-s due to GC involvement.

Comment 2

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   115546:8275b86c0b62
user:        Brian Hackett
date:        Mon Dec 10 12:02:31 2012 -0700
summary:     Remove bytecode uses analysis, keep track of SSA values that were folded away when building MIR, bug 818869. r=jandem

This iteration took 133.228 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from bhackett for comment 2 :)

Comment 4

[Andrew McCreight [:mccr8]]

The value in question isMarkable() but is apparently not a gc thing? That seems a little odd.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

This is an interaction between Ion and JM.  When JM does OSR it does not check the types of the values on the interpreter stack, and assumes they match whatever was inferred.  When those values are dead, however, Ion can write an undefined value to the stack instead of the original, and JM can end up treating that undefined value's null payload as an object/string and write torn values to the stack.  Since the value is dead those torn values will only be observed during GC, and since the original value is undefined this can only manifest in a null deref.

I think it's best to fix this by just being tolerant of these values when marking the VM stack.  Trying to reason within Ion about what JM may or may not do is difficult and could hurt codegen quality, and fixing this during marking is not hard and can be easily removed once JM is gone.

Comment 6

[Brian Hackett [Laid off!]]

NULL deref, not s-s.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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

Yuck. Oh well.

::: js/src/gc/Marking.cpp
@@ +502,5 @@
> +MarkValueInternalMaybeNullPayload(JSTracer *trc, Value *v)
> +{
> +    if (v->isMarkable()) {
> +        void *thing = v->toGCThing();
> +        if (thing) {

If you don't call a mark function, I think you're supposed to call JS_UNSET_TRACING_LOCATION.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f87c11f27b0

Comment 9

[Brian Hackett [Laid off!]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 818869
User impact if declined: potential null deref
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): none

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f87c11f27b0

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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

low risk null deref, approved for aurora.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I've got this in my queue for Aurora landing once it reopens. Clearing checkin-needed so this doesn't show up in my queries.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1145d8d79fbf

Comment 15

[Mihaela Velimiroviciu (:mihaelav)]

Verified the fix with IonMonkey from latest Firefox 20.0 beta 2: no assertion

Comment 16

[Paul Silaghi, QA [:pauly]]

No assertion on FF 21 2013-05-07-mozilla-beta-debug build. Verified fixed