886287 - OdinMonkey: Assertion failure: begin <= end, at gc/RootMarking.cpp:264

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 76820c6dff7b (run with --fuzzing-safe --ion-eager):


try {
  (function (x) {
    gczeal(2)();
  })()
} catch(exc1) {}
function DiagModule(stdlib, foreign) {
    "use asm";
    var test = foreign.test;
    function diag() {
        var x = 0.0;
        while (1) {
          test(1, x);
        }
    }
    function diag_1() {}
    return { diag: diag, diag_1:diag_1 };
}
var foreign = {
  test:function(a,b) {
    unescape(a+":"+b)
  }
}
DiagModule(this, foreign).diag();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Marked this one s-s because the assertion is range-related. Here's a backtrace:


Program received signal SIGSEGV, Segmentation fault.
0x000000000051633c in MarkRangeConservatively (end=<optimized out>, begin=<optimized out>, trc=<optimized out>) at js/src/gc/RootMarking.cpp:264
264         JS_ASSERT(begin <= end);
(gdb) bt
#0  0x000000000051633c in MarkRangeConservatively (end=<optimized out>, begin=<optimized out>, trc=<optimized out>) at js/src/gc/RootMarking.cpp:264
#1  0x0000000000517528 in MarkRangeConservatively (end=<optimized out>, begin=<optimized out>, trc=<optimized out>) at js/src/gc/RootMarking.cpp:265
#2  MarkConservativeStackRoots (trc=0x186ff60, useSavedRoots=<optimized out>) at js/src/gc/RootMarking.cpp:333
#3  0x0000000000519bf7 in js::gc::MarkRuntime (trc=0x186ff60, useSavedRoots=false) at js/src/gc/RootMarking.cpp:660
#4  0x000000000063bfba in BeginMarkPhase (rt=<optimized out>) at js/src/jsgc.cpp:2808
#5  IncrementalCollectSlice (rt=<optimized out>, budget=<optimized out>, reason=JS::gcreason::DEBUG_GC, gckind=js::GC_NORMAL) at js/src/jsgc.cpp:4238
#6  0x000000000063e188 in GCCycle (rt=0x186fc90, incremental=false, budget=<optimized out>, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:4416
#7  0x000000000063e6d1 in Collect (rt=0x186fc90, incremental=false, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:4552
#8  0x0000000000557713 in NewGCThing<JSShortString, (js::AllowGC)1> (tcx=0x188b1f0, kind=<optimized out>, thingSize=<optimized out>, heap=<optimized out>) at ../jsgcinlines.h:524
#9  js_NewGCShortString<(js::AllowGC)1> (tcx=0x188b1f0) at ../jsgcinlines.h:578
#10 js::ConcatStrings<(js::AllowGC)1> (cx=0x188b1f0, left=..., right=...) at js/src/vm/String.cpp:345
#11 0x00007ffff6bc6387 in ?? ()

Comment 3

[Luke Wagner [:luke]]

Jan: this seems related to Activations; do you suppose this is the same sort of fix as the numFrameSlots bug?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #3)
> Jan: this seems related to Activations; do you suppose this is the same sort
> of fix as the numFrameSlots bug?

I can't reproduce this on OS X (with the info in comment 0). I will try on Linux tomorrow.

Comment 5

[Luke Wagner [:luke]]

I wasn't able to repro on linux x64 (--enable-debug --disable-optimize) on the cset in comment 0 either.

Comment 6

[Luke Wagner [:luke]]

Oh, there we go, --enable-debug --disable-optimize crashes x64 linux (even w/o --ion-eager or --fuzzing-safe).

Comment 7

[Jan de Mooij [:jandem]]

Hannes, if I make TryEnablingIon always return immediately, the crash goes away so I think this is a problem with the asm.js -> Ion fast path.

Comment 8

[Hannes Verschore [:h4writer]]

I had big trouble reproducing it on my own box. But decoder helped me out and provided one of his. So I'm able to reproduce and debug.

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Patch

I made active/non-active state look more closely to what happens at construction/destruction of Activation. This  fixed it for me. This patch also fixes bug 886255

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 768828 [details] [diff] [review]
Patch

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

::: js/src/vm/Stack.h
@@ +1178,5 @@
>      }
>      bool isActive() const {
>          return active_;
>      }
> +    virtual void setActive(JSContext *cx, bool active = true);

Can you move active_ etc from Activation to JitActivation? Other activations are never inactive so it makes sense for it to be on JitActivation I think and it avoids the virtual method.

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Patch

Looks like the previous patch don't fix the actual issue here. I'll move that patch to bug 886255 and let jandem review my updated patch.

The bug here is that we don't zero extend the descriptor on x64. Resulting in garbage on the stack. This isn't a problem, except when shifting to get the offset. We shift the garbage in and get the wrong value.

Comment 12

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9939c3648650

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9939c3648650

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 770218 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 860838

User impact if declined: No issue for x86 and arm, pretty serieus for x64. (A real security issue, with possibility to read random data, maybe even possibility to read special crafted data)

Testing completed (on m-c, etc.): m-i:2 days, m-c: 1 day

Risk to taking this patch (and alternatives if risky): Very very low. x86/arm will do the same as before this change. x64 will zerofill the upper 32bits (instead of using the garbage on the stack)

String or IDL/UUID changes made by this patch: /

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Hannes Verschore [:h4writer]]

http://hg.mozilla.org/releases/mozilla-aurora

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Marking Firefox 23 unaffected based on bug 860838 checkin date.