Closed Bug 1519705 Opened 9 months ago Closed 9 months ago

Crash [@ js::gc::Cell::isTenured] with over-recursion in GC

Categories

(Core :: JavaScript: GC, defect, P2, critical)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 74bb778f7879 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --ion-eager --no-sse3 --enable-streams --disable-oom-functions):

See attachment.

Backtrace:

received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf56ffb40 (LWP 29582)]
0x5693c407 in js::gc::Cell::isTenured (this=0xed5a84a0) at js/src/gc/Cell.h:79
#0 0x5693c407 in js::gc::Cell::isTenured (this=0xed5a84a0) at js/src/gc/Cell.h:79
#1 js::gc::Cell::asTenured (this=0xed5a84a0) at js/src/gc/Cell.h:225
#2 0x56e04a38 in AssertShouldMarkInZone (thing=<optimized out>) at js/src/gc/Marking.cpp:382
#3 0x56e723ff in js::GCMarker::mark<js::Scope> (thing=0xed5a84a0, this=0xf57862cc) at js/src/gc/Marking.cpp:981
#4 js::GCMarker::markAndScan<js::Scope> (this=0xf57862cc, thing=0xed5a84a0) at js/src/gc/Marking.cpp:834
#5 0x56e71e97 in js::GCMarker::traverse<js::Scope*> (thing=0xed5a84a0, this=0xf57862cc) at js/src/gc/Marking.cpp:854
#6 js::GCMarker::traverseEdge<js::Scope*, js::Scope> (target=0xed5a84a0, source=0xed5a84b0, this=0xf57862cc) at js/src/gc/Marking.cpp:931
#7 js::GCMarker::eagerlyMarkChildren (this=0xf57862cc, scope=0xed5a84b0) at js/src/gc/Marking.cpp:1323
#8 0x56e72449 in js::GCMarker::markAndScan<js::Scope> (this=<optimized out>, thing=<optimized out>) at js/src/gc/Marking.cpp:835
#9 0x56e71e97 in js::GCMarker::traverse<js::Scope*> (thing=0xed5a84b0, this=0xf57862cc) at js/src/gc/Marking.cpp:854
#10 js::GCMarker::traverseEdge<js::Scope*, js::Scope> (target=0xed5a84b0, source=0xed5a84c0, this=0xf57862cc) at js/src/gc/Marking.cpp:931
#11 js::GCMarker::eagerlyMarkChildren (this=0xf57862cc, scope=0xed5a84c0) at js/src/gc/Marking.cpp:1323
[...]
#124 0x56e72449 in js::GCMarker::markAndScan<js::Scope> (this=<optimized out>, thing=<optimized out>) at js/src/gc/Marking.cpp:835
#125 0x56e71e97 in js::GCMarker::traverse<js::Scope*> (thing=0xed5a8680, this=0xf57862cc) at js/src/gc/Marking.cpp:854
#126 js::GCMarker::traverseEdge<js::Scope*, js::Scope> (target=0xed5a8680, source=0xed5a8690, this=0xf57862cc) at js/src/gc/Marking.cpp:931
#127 js::GCMarker::eagerlyMarkChildren (this=0xf57862cc, scope=0xed5a8690) at js/src/gc/Marking.cpp:1323
eax 0xed5a84a0 -312834912
ebx 0xed5a84a0 -312834912
ecx 0x0 0
edx 0x1 1
esi 0x57c0cff4 1472253940
edi 0xed5a84b0 -312834896
ebp 0xf55c0018 4116447256
esp 0xf55c0000 4116447232
eip 0x5693c407 <js::gc::Cell::asTenured()+23>
=> 0x5693c407 <js::gc::Cell::asTenured()+23>: call 0x5675e200 <js::gc::IsInsideNursery(js::gc::Cell const*)>
0x5693c40c <js::gc::Cell::asTenured()+28>: add $0x10,%esp

Reducing the test further makes it much more intermittent. However, in its current state, it is fairly reliable on 32-bit and might point to the same problem that we have seen before on x86-64 with GC over-recursion, where tests failed to reproduce reliably.

Attached file Testcase

Based on the full stack trace decoder sent me before filing this, it looks like we can have deeply nested scopes and we end up running out of stack space when marking them. I wonder if there's an underlying bug of us not limiting things properly in some cases.

Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]

Jon, could you help me identify what the next steps are with this regression bug? Or who on our team should continue looking at it?

Flags: needinfo?(jcoppeard)

It would probably help to mark scopes using the mark stack rather than using recursion.

Component: JavaScript Engine → JavaScript: GC

In the source:

// Strings, LazyScripts, Shapes, and Scopes are extremely common, but have
// simple patterns of recursion. We traverse trees of these edges immediately,
// with aggressive, manual inlining, implemented by eagerlyTraceChildren.

We could use the mark stack, but I presume it would slow down regular Scope marking? I'm not sure all the ways you can create deeply nested Scopes, but I like jandem's idea. Is there an easy way to limit the maximum scope depth? Maybe jandem would know, but I'll needinfo jorendorff as a front end-y type person.

Flags: needinfo?(jorendorff)

We could use the mark stack, but I presume it would slow down regular Scope marking?

Yes, that's true.

Looking at this again: we should use a loop to mark enclosing scopes without recursion like we already do for shapes.

Flags: needinfo?(jcoppeard)
Priority: -- → P2

Patch to use a loop to mark enclosing scopes rather than recursion which can blow the stack.

Assignee: nobody → jcoppeard
Attachment #9039110 - Flags: review?(sphink)
Attachment #9039110 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a34a2ae476a2
Use a loop rather than recursion to mark enclosing scope r=sfink
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

It looks like the ni?me was obsoleted by events. Looks great.

Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.