Last Comment Bug 728190 - Assertion failure: kind == GetGCThingTraceKind(thing), at js/src/jsgcmark.cpp:219 or Crash [@ JSString::isLinear]
: Assertion failure: kind == GetGCThingTraceKind(thing), at js/src/jsgcmark.cpp...
Status: VERIFIED FIXED
[sg:critical] js-triage-needed [advis...
: assertion, crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla13
Assigned To: Bill McCloskey (:billm)
:
:
Mentors:
Depends on:
Blocks: langfuzz 723313
  Show dependency treegraph
 
Reported: 2012-02-17 05:19 PST by Christian Holler (:decoder)
Modified: 2013-01-16 07:55 PST (History)
9 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
unaffected
-
unaffected
+
unaffected
+
fixed
unaffected


Attachments
patch (757 bytes, patch)
2012-02-17 16:25 PST, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
patch with test (1.57 KB, patch)
2012-02-17 16:30 PST, Bill McCloskey (:billm)
bhackett1024: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-02-17 05:19:53 PST
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 (:billm) 2012-02-17 16:25:25 PST
Created attachment 598432 [details] [diff] [review]
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 (:billm) 2012-02-17 16:30:26 PST
Created attachment 598435 [details] [diff] [review]
patch with test

Forgot the test.
Comment 3 Bill McCloskey (:billm) 2012-02-17 16:30:57 PST
Comment on attachment 598435 [details] [diff] [review]
patch with test

And, I'm an idiot.
Comment 4 Brian Hackett (:bhackett) 2012-02-22 10:31:54 PST
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 6 Ed Morley [:emorley] 2012-02-23 13:24:18 PST
https://hg.mozilla.org/mozilla-central/rev/b8cd92904684
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 13:45:08 PST
Guessing that 12 is affected here, if not, let me know and I'll fix the flags...
Comment 8 Bill McCloskey (:billm) 2012-02-23 13:48:48 PST
Another regression from bug 723313. So 12 is unaffected here as well.
Comment 9 Christian Holler (:decoder) 2012-03-23 16:54:10 PDT
Test committed with fix, marking verified based on that.
Comment 10 Christian Holler (:decoder) 2013-01-14 08:27:35 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug728190.js.
Comment 11 Christian Holler (:decoder) 2013-01-16 07:55:47 PST
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.

Note You need to log in before you can comment on or make changes to this bug.