728190 - Assertion failure: kind == GetGCThingTraceKind(thing), at js/src/jsgcmark.cpp:219 or Crash [@ JSString::isLinear]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts/crashes on mozilla-central revision 78fde7e54d92 (options -m -n -a):


function TestCase(n, d, e, a) {}
var lfcode = new Array();
lfcode.push("");
lfcode.push("\
  var summary = 'foo';\
  test();\
  function test() {\
    test(\"TEST-UNEXPECTED-FAIL | TestPerf | \" + summary);\
  }\
");
lfcode.push("gczeal(2);");
lfcode.push("\
  new TestCase(TestFunction_3( \"P\", \"A\", \"S\", \"S\" ) +\"\");\
  new TestCase(TestFunction_4( \"F\", \"A\", (\"length\"), \"L\" ) +\"\");\
  function TestFunction_3( a, b, c, d, e ) {\
    TestFunction_3(arguments);\
  }\
");
while (true) {
        var file = lfcode.shift(); if (file == undefined) { break; }
        try { evaluate(file); } catch (lfVare) {}
}


Stepping through the assert crashes:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000442c72 in JSString::isLinear (this=0x3ffffb2269fc) at /srv/repos/mozilla-central/js/src/vm/String.h:316
316             return (d.lengthAndFlags & LINEAR_MASK) == LINEAR_FLAGS;
(gdb) bt 8
#0  0x0000000000442c72 in JSString::isLinear (this=0x3ffffb2269fc) at /srv/repos/mozilla-central/js/src/vm/String.h:316
#1  0x00000000004c7392 in js::gc::ScanLinearString (gcmarker=0x7fffffffa740, str=0x3ffffb2269fc) at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:528
#2  0x00000000004c774a in js::gc::ScanString (gcmarker=0x7fffffffa740, str=0x7ffff6130520) at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:593
#3  0x00000000004c7810 in js::gc::PushMarkStack (gcmarker=0x7fffffffa740, str=0x7ffff6130520) at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:609
#4  0x00000000004cc45a in js::gc::MarkInternal<JSString> (trc=0x7fffffffa740, thing=0x7ffff6130520) at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:110
#5  0x00000000004c65bf in js::gc::MarkKind (trc=0x7fffffffa740, thing=0x7ffff6130520, kind=JSTRACE_STRING) at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:225
#6  0x00000000004c6930 in js::gc::MarkValueInternal (trc=0x7fffffffa740, v=...) at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:306
#7  0x00000000004c6ac2 in js::gc::MarkValueRootRange (trc=0x7fffffffa740, len=3, vec=0x7ffff6489fa8, name=0x7d37e6 "vm_stack")
    at /srv/repos/mozilla-central/js/src/jsgcmark.cpp:338
(More stack frames follow...)
(gdb) x /4i $pc
=> 0x442c72 <JSString::isLinear() const+12>:    mov    (%rax),%rax
   0x442c75 <JSString::isLinear() const+15>:    and    $0x1,%eax
   0x442c78 <JSString::isLinear() const+18>:    test   %rax,%rax
   0x442c7b <JSString::isLinear() const+21>:    sete   %al
(gdb) info register rax
rax            0x3ffffb2269fc   70368662546940


S-s and sg:critical due to GC related crash. Although this is also a GC issue, I don't think it's related to the other issue I recently reported (bug 728086) because this one is much easier to reproduce and doesn't seem to involve natives.

Comment 1

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

Attachment: patch

The JSOP_ARGUMENTS stub was moving up the stack pointer without initializing the new area with anything, and then calling a function that can GC. I just did the increment afterwards, which seems to be what the interpreter does anyway.

I also looked through the rest of StubCalls.cpp and I didn't see any other examples cases this. I can't really think of a way to assert against this sort of situation without changing a lot of code. There are just too many places where we directly manipulate regs.sp.

Comment 2

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

Attachment: patch with test

Forgot the test.

Comment 3

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

Comment on attachment 598435 [details] [diff] [review]
patch with test

And, I'm an idiot.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 598435 [details] [diff] [review]
patch with test

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

::: js/src/methodjit/StubCalls.cpp
@@ +1368,2 @@
>          THROW();
> +    *f.regs.sp++ = argsobj;

The ++ isn't actually necessary, regs.sp is not live at the end of a stub call.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8cd92904684

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8cd92904684

Comment 7

[Johnny Stenback (:jst)]

Guessing that 12 is affected here, if not, let me know and I'll fix the flags...

Comment 8

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

Another regression from bug 723313. So 12 is unaffected here as well.

Comment 9

[Christian Holler (:decoder)]

Test committed with fix, marking verified based on that.

Comment 10

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug728190.js.

Comment 11

[Christian Holler (:decoder)]

Setting in-testsuite- because this test is in the testsuite but marked as slow test (which means it won't run automatically). I verified manually that the tests pass, so keeping VERIFIED.