Assertion failure: !empty(), at ../../dist/include/mozilla/Vector.h:452 with OOM

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla44
x86_64
Linux
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox44 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision 3c26bef95d54 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off):

setGCCallback({
    action: "majorGC",
});
oomAfterAllocations(50);


Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00000000008121c2 in mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back (this=<optimized out>)
    at ../../dist/include/mozilla/Vector.h:452
To enable execution of this file add
	add-auto-load-safe-path /home/ubuntu/mozilla-central/js/src/debug64/dist/bin/js-gdb.py
line to your configuration file "/home/ubuntu/.gdbinit".
To completely disable this security protection add
	set auto-load safe-path /
line to your configuration file "/home/ubuntu/.gdbinit".
For more information about this security protection see the
"Auto-loading safe path" section in the GDB manual.  E.g., run from the shell:
	info "(gdb)Auto-loading safe path"
#0  0x00000000008121c2 in mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back (this=<optimized out>) at ../../dist/include/mozilla/Vector.h:452
#1  0x00000000007f618c in js::gcstats::Statistics::endSlice (this=0x7f067203d6d8) at js/src/gc/Statistics.cpp:956
#2  0x0000000000b477af in ~AutoGCSlice (this=0x7fffb74a4020, __in_chrg=<optimized out>) at js/src/gc/Statistics.h:341
#3  js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6148
#4  0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#5  JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#6  0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#7  0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#8  0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#9  JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#10 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#11 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#12 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#13 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#14 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#15 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#16 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#17 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#18 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#19 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#20 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#21 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#22 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#23 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#24 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#25 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#26 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#27 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#28 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#29 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#30 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#31 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#32 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#33 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#34 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#35 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159
#36 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224
#37 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056
#38 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560
#39 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6159
#40 0x0000000000b479b0 in js::gc::GCRuntime::gc (this=this@entry=0x7f0672037350, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6224
#41 0x0000000000a8b5b3 in js::DestroyContext (cx=0x7f067201b330, mode=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:185
#42 0x0000000000a8b92e in JS_DestroyContext (cx=<optimized out>) at js/src/jsapi.cpp:738
#43 0x00000000004725f2 in DestroyContext (withGC=true, cx=0x7f067201b330) at js/src/shell/js.cpp:5698
#44 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6494
rax	0x0	0
rbx	0x7f067203d6d8	139665659385560
rcx	0x7f067236888d	139665662707853
rdx	0x0	0
rsi	0x7f067263d9d0	139665665677776
rdi	0x7f067263c1c0	139665665671616
rbp	0x7fffb74a3f40	140736268484416
rsp	0x7fffb74a3f40	140736268484416
r8	0x7f06736ad780	139665682913152
r9	0x6c697a6f6d2f6564	7811909647642617188
r10	0x7f0672639be0	139665665661920
r11	0x0	0
r12	0x7f067203d710	139665659385616
r13	0x0	0
r14	0x0	0
r15	0x0	0
rip	0x8121c2 <mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back()+66>
=> 0x8121c2 <mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back()+66>:	movl   $0x1c4,0x0
   0x8121cd <mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back()+77>:	callq  0x4941f0 <abort()>
(Reporter)

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 1

2 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4f64a711181d
user:        Steve Fink
date:        Tue Jan 13 14:01:42 2015 -0800
summary:     Bug 1117768 - Fix assertion in AutoStopVerifyingBarriers and add tests, r=terrence

This iteration took 553.749 seconds to run.
Flags: needinfo?(sphink)
Assignee: nobody → evilpies
Assignee: evilpies → nobody
Created attachment 8670314 [details] [diff] [review]
aborted.patch

So there is actually already a gcDepth counter in the Statistics object, for taking into account nested GCs. Let's just do the same thing for "aborted", making it a counter rather than a boolean.

Also, I've realized that gcDepth wouldn't be incremented if aborted is set to true, but it still could be decremented later, leading to a false count. I made increment and decrement dependent upon aborted, so as to ensure it's more correct.
Flags: needinfo?(sphink)
Attachment #8670314 - Flags: review?(jcoppeard)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8670314 [details] [diff] [review]
aborted.patch

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

Sorry I don't think this is the right fix for the problem here.  We need to stop the begin callback from running a GC (this will never happen in practice).

> Also, I've realized that gcDepth wouldn't be incremented if aborted is set to true, but it still could be decremented later, leading to a false count.

This sounds like a real problem however!
Attachment #8670314 - Flags: review?(jcoppeard)
Created attachment 8670386 [details] [diff] [review]
gccallback.patch

So something like this, instead?
Attachment #8670314 - Attachment is obsolete: true
Attachment #8670386 - Flags: review?(jcoppeard)
(Assignee)

Comment 5

2 years ago
Comment on attachment 8670386 [details] [diff] [review]
gccallback.patch

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

Please add a test for this.

::: js/src/jsgc.cpp
@@ +6326,5 @@
>          return;
>  
> +    // The GC callback shouldn't call collect()
> +    if (callingGCCallback)
> +        return;

Can you move this to checkIfGCAllowedInCurrentState()?

Also, comment needs a fullstop at the end.

@@ +6351,5 @@
>          poked = false;
>          bool wasReset = gcCycle(nonincrementalByAPI, budget, reason);
>  
>          if (!isIncrementalGCInProgress()) {
> +            AutoSetCallingGCCallback scgc(*this);

I think this will fail as the cycle collector does trigger GC from here.  We can just remove this line though.
Attachment #8670386 - Flags: review?(jcoppeard) → review+
Created attachment 8670408 [details] [diff] [review]
gccallback.patch

Thanks for the review!
However, the test case embedded in the patch fails if we don't put the RAII class in the second branch... The issue shows up if the gc callback is called in the BEGIN or in the END position. If the CC wants to call collect() at the END position, then the first patch looks more valid, what do you think? If not, feel free to take over this bug :)
Attachment #8670386 - Attachment is obsolete: true
After discussing it, Jon said IRL he'd take over the bug.
Flags: needinfo?(jcoppeard)
(Assignee)

Updated

2 years ago
Assignee: nobody → jcoppeard
(Assignee)

Comment 8

2 years ago
Created attachment 8673026 [details] [diff] [review]
bug1175755-stats-crash

I'm sorry, I didn't understand this bug at first and what I told you about how to fix it was wrong.

This patch is more like your original patch, but instead of making |aborted| a counter, we only clear it at the end of the outermost GC.

I moved the increment/decrement of gcDepth so that it is always > 0 when we are inside a GC, and added an assert that it doesn't go negative which could happen previously if we git OOM and returned early from beginSlice().
Attachment #8670408 - Attachment is obsolete: true
Flags: needinfo?(jcoppeard)
Attachment #8673026 - Flags: review?(benj)
Comment on attachment 8673026 [details] [diff] [review]
bug1175755-stats-crash

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

Thanks, that's simpler indeed.

::: js/src/jit-test/tests/gc/bug-1175755.js
@@ +1,4 @@
> +setGCCallback({
> +    action: "majorGC",
> +});
> +oomAfterAllocations(50);

Do we need to test for oomAfterAllocations, and wrap the code in a try/catch to ignore the OOM message?
Attachment #8673026 - Flags: review?(benj) → review+
(Assignee)

Comment 10

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Do we need to test for oomAfterAllocations and wrap the code in a try/catch to ignore the OOM message?

Yes, indeed we do.

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/946d59ccc8da
https://hg.mozilla.org/mozilla-central/rev/946d59ccc8da
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.