The following testcase crashes on mozilla-central revision 727d765a5ed8 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-eager): See attachment (does not reproduce for me, attached anyway) Backtrace: Program terminated with signal SIGSEGV, Segmentation fault. #0 0x0000000000be88d3 in js::Shape::fixupGetterSetterForBarrier (this=0x7ff35cc1c3f8, trc=<optimized out>) at js/src/jspropertytree.cpp:331 #1 0x00000000004fee90 in js::gc::StoreBuffer::GenericBuffer::trace (this=this@entry=0x7ff363737710, owner=owner@entry=0x7ff3637375a8, trc=trc@entry=0x7fff274fa400) at js/src/gc/StoreBuffer.cpp:34 #2 0x0000000000863589 in traceGenericEntries (trc=0x7fff274fa400, this=0x7ff3637375a8) at js/src/gc/StoreBuffer.h:438 #3 js::Nursery::collect (this=this@entry=0x7ff363737460, rt=0x7ff363737000, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT, pretenureGroups=pretenureGroups@entry=0x0) at js/src/gc/Nursery.cpp:460 #4 0x0000000000b3c365 in js::gc::GCRuntime::minorGCImpl (this=this@entry=0x7ff363737408, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT, pretenureGroups=pretenureGroups@entry=0x0) at js/src/jsgc.cpp:6566 #5 0x0000000000b80e69 in evictNursery (reason=JS::gcreason::DESTROY_CONTEXT, this=0x7ff363737408, this@entry=0x7ff363737738) at js/src/gc/GCRuntime.h:611 #6 js::gc::GCRuntime::gcCycle (this=this@entry=0x7ff363737408, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6151 #7 0x0000000000b8148a in js::gc::GCRuntime::collect (this=this@entry=0x7ff363737408, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6328 #8 0x0000000000b81793 in js::gc::GCRuntime::gc (this=0x7ff363737408, gckind=<optimized out>, reason=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6392 #9 0x0000000000b8236c in js::DestroyContext (cx=0x7ff363706c00, mode=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:186 #10 0x0000000000b8255e in JS_DestroyContext (cx=<optimized out>) at js/src/jsapi.cpp:799 #11 0x0000000000477492 in DestroyContext (withGC=true, cx=0x7ff363706c00) at js/src/shell/js.cpp:5748 #12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6588 rax 0x0 0 rbx 0x7ff35cc1c3f8 140683209982968 rcx 0x7ff3639ee88d 140683325139085 rdx 0x0 0 rsi 0x7ff363cc39d0 140683328109008 rdi 0x7ff363cc21c0 140683328102848 rbp 0x7fff274fa1f0 140733852918256 rsp 0x7fff274fa130 140733852918064 r8 0x7ff364d33780 140683345344384 r9 0x6372732f736a2f6c 7165916604736876396 r10 0x7ff363cbfbe0 140683328093152 r11 0x0 0 r12 0x7ff3618a8920 140683290249504 r13 0x7fff274fa1a0 140733852918176 r14 0x7ff3618a8920 140683290249504 r15 0x7ff35bf0a028 140683196276776 rip 0xbe88d3 <js::Shape::fixupGetterSetterForBarrier(JSTracer*)+371> => 0xbe88d3 <js::Shape::fixupGetterSetterForBarrier(JSTracer*)+371>: movl $0x14b,0x0 0xbe88de <js::Shape::fixupGetterSetterForBarrier(JSTracer*)+382>: callq 0x497970 <abort()> jonco already seems to have a solution for this one \o/

Doesn't reproduce, but it seems likely that we're simulating OOM here and causing the hash set put operation to fail, which we assert succeeds. In fact it should always succeed because we just removed an entry from the table. The fix is just to call putNewInfallible() which doesn't simulate OOM. I think you'll end up removing this code anyway with the stable object hash changes.

So the previous patch (and the original code) was actually wrong, because in our implementation removing an entry from a hashtable doesn't necessarily leave enough space to add a new entry without allocating. The problem is that we can replace the entry with a tombstone, but we don't rehash if there are too many of these. If we call putNewInfallible() in this case it will go into an infinite loop looking for a free place in the table. Instead of trying to ensure that remove followed by put always works which sounds like it would be difficult to gaurantee, I changed the fixup function to use rekey instead. This results in more code in the fixup function, but it hopefully all get removed soon anyway.

Comment on attachment 8673775 [details] [diff] [review] bug1214175-shape-fixup v2 Review of attachment 8673775 [details] [diff] [review]: ----------------------------------------------------------------- WFM. I was thinking that we could put a new entry over a tombstone, but I guess not? Now that I think about it, ISTR that there was some reason for this that was highly non-obvious. The UID code is all in tree, maybe fixing the hashing would be even easier here? Either way is fine.

(In reply to Terrence Cole [:terrence] from comment #4) Great. I'm going to land this fix and let you update to stable hashing whenever you get to it.