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

VERIFIED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: billm)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla13
x86_64
Linux
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox10- unaffected, firefox11- unaffected, firefox12+ unaffected, firefox13+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:critical] js-triage-needed [advisory-tracking+], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #598432 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

5 years ago
Created attachment 598435 [details] [diff] [review]
patch with test

Forgot the test.
Attachment #598432 - Attachment is obsolete: true
Attachment #598435 - Flags: review?(wmccloskey)
Attachment #598432 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

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

And, I'm an idiot.
Attachment #598435 - Flags: review?(wmccloskey) → review?(bhackett1024)
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.
Attachment #598435 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8cd92904684
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/b8cd92904684
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Guessing that 12 is affected here, if not, let me know and I'll fix the flags...
status-firefox10: --- → wontfix
status-firefox11: --- → wontfix
status-firefox12: --- → affected
status-firefox13: --- → fixed
tracking-firefox10: --- → -
tracking-firefox11: --- → -
tracking-firefox12: --- → +
tracking-firefox13: --- → +
(Assignee)

Comment 8

5 years ago
Another regression from bug 723313. So 12 is unaffected here as well.
status-firefox12: affected → unaffected
Blocks: 723313
Group: core-security
status-firefox-esr10: --- → unaffected
status-firefox10: wontfix → unaffected
status-firefox11: wontfix → unaffected
Keywords: regression
(Reporter)

Comment 9

5 years ago
Test committed with fix, marking verified based on that.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical] js-triage-needed → [sg:critical] js-triage-needed [advisory-tracking+]
(Reporter)

Comment 10

4 years ago
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug728190.js.
Flags: in-testsuite+
(Reporter)

Comment 11

4 years ago
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.
Flags: in-testsuite+ → in-testsuite-
You need to log in before you can comment on or make changes to this bug.