The following testcase crashes on mozilla-central revision 1d26b327ee6b (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off): var o = { p: 1, r: 3, s: 4, }; for (var i in o) { gc(); delete o.s; } Backtrace: received signal SIGSEGV, Segmentation fault. MOZ_CrashPrintf (aLine=aLine@entry=2613, aFormat=aFormat@entry=0xf404d0 "Got 0x%lx expected magic word 0x%lx flags 0x%lx objectSet 0x%lx") at mfbt/Assertions.cpp:63 #0 MOZ_CrashPrintf (aLine=aLine@entry=2613, aFormat=aFormat@entry=0xf404d0 "Got 0x%lx expected magic word 0x%lx flags 0x%lx objectSet 0x%lx") at mfbt/Assertions.cpp:63 #1 0x0000000000a68de3 in js::ReportMagicWordFailure (actual=<optimized out>, expected=expected@entry=11647069175327152090, flags=<optimized out>, objectSet=<optimized out>) at js/src/vm/TypeInference.cpp:2611 #2 0x0000000000c37a77 in js::ConstraintTypeSet::checkMagic (this=0x1ea1c88 <js::PropertyIteratorObject::class_+8>) at js/src/vm/TypeInference.h:739 #3 js::ObjectGroup::getProperty (sweep=..., i=<optimized out>, this=0x7ffff4ca2128) at js/src/vm/TypeInference-inl.h:1211 #4 js::GCMarker::lazilyMarkChildren (this=this@entry=0x7ffff5f1a098, group=0x7ffff4ca2128) at js/src/gc/Marking.cpp:1522 #5 0x0000000000c4c7ad in js::GCMarker::processMarkStackTop (this=this@entry=0x7ffff5f1a098, budget=...) at js/src/gc/Marking.cpp:1752 #6 0x0000000000c3cf37 in js::GCMarker::drainMarkStack (this=this@entry=0x7ffff5f1a098, budget=...) at js/src/gc/Marking.cpp:1651 #7 0x0000000000bf05d0 in js::gc::GCRuntime::drainMarkStack (this=0x7ffff5f194b0, sliceBudget=..., phase=<optimized out>) at js/src/gc/GC.cpp:5903 #8 0x0000000000c0e45a in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff5f194b0, budget=..., reason=reason@entry=JS::gcreason::API, session=...) at js/src/gc/GC.cpp:7116 #9 0x0000000000c0f427 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f194b0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:7478 #10 0x0000000000c0f9ad in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f194b0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:7621 #11 0x0000000000c0fc78 in js::gc::GCRuntime::gc (this=0x7ffff5f194b0, gckind=<optimized out>, reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:7691 #12 0x0000000000c0fcb0 in JS::GCForReason (cx=cx@entry=0x7ffff5f14000, gckind=<optimized out>, reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:8531 #13 0x000000000079338a in GC (cx=0x7ffff5f14000, argc=<optimized out>, vp=0x7ffff49eb0a0) at js/src/builtin/TestingFunctions.cpp:351 #14 0x00000000005657b1 in js::CallJSNative (args=..., native=0x7932a0 <GC(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff5f14000) at js/src/vm/JSContext-inl.h:280 [...] #27 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9330 rax 0x1ec8660 32278112 rbx 0x7ffff5f1a098 140737319641240 rcx 0x7fffffad 2147483565 rdx 0x1ec8712 32278290 rsi 0x0 0 rdi 0x1ec86c0 32278208 rbp 0x1ea1c80 <js::PropertyIteratorObject::class_> rsp 0x7fffffffc800 140737488340992 r8 0x0 0 r9 0x52 82 r10 0x0 0 r11 0xfffffffffffffffa -6 r12 0x0 0 r13 0x7ffff4ca2128 140737300275496 r14 0x7ff8 32760 r15 0xa1a2b3b4c5c6d7da -6799674898382399526 rip 0x430bcb <MOZ_CrashPrintf(int, char const*, ...)+243> => 0x430bcb <MOZ_CrashPrintf(int, char const*, ...)+243>: movl $0x0,0x0 0x430bd6 <MOZ_CrashPrintf(int, char const*, ...)+254>: ud2 This crashes in various ways depending on 32 or 64-bit build, sometimes asserts with magic word failure, in debug builds shows a type mismatch GC assert. Marking s-s and sec-high.

This can also show up as: Assertion failure: [barrier verifier] Unmarked edge: Object 0xf59610e0 'receiver_guard_shape' edge to Shape 0xf597a7f0, at gc/Verifier.cpp:384

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/7d83c7de9404 user: Jeff Walden date: Wed May 16 23:24:28 2018 -0700 summary: Bug 1462540 - Remove NativeIterator::guard_array: its numeric value is identical to NativeIterator::props_end. r=jandem This iteration took 244.659 seconds to run.

Also seeing: Assertion failure: !overlay->isForwarded(), at gc/Marking-inl.h:41 Needinfo from Jeff based on comment 2. Also marking fuzzblocker due to the high amount of different signatures caused by this.

FYI, OSS-Fuzz has also discovered this, which attaches a 90 day disclosure deadline to it.

Good to know. This should be fixable far before then. Also: hrm. This was very carefully written to avoid such issues, so it will be interesting to see what's up here.

Bah, I am a dumb. Property deletion causes NativeIterator::props_end to be decremented, which means guardArray() starts pointing into zeroed-out GCPtrFlatStrings and mis-offsetting which causes (in this case, with a property deletions) an ObjectGroup* to be interpreted as a Shape* and so that gets you the trace-kind mismatch from the assertion. Readding the field is the stupidest fix, but obviously it's better to not have the extra field. What if we preserved the lost-field by flipping the trailer fields after the NativeIterator? That is, instead of having [NativeIterator fields][prop0][prop1]...[propM-1][guard0][guard1]...[guardN-1] we had [NativeIterator fields][guard0][guard1]...[guardM-1][prop0][prop1]...[propN-1] ? NativeIterator::guard_length is only mutated during NativeIterator creation, now, so we could basically just swap props/guard in all the fields and functions and the current broken design would be fixed. This would slow down begin() because it'd have to do a pointer-load in |this->guards_end|, but it's only called in somewhat rare-ish places -- and I can't find any in JITted code, where |this->props_cursor| is already a necessary pointer-load -- so marginally slower perf seems unimportant. And if we changed how props are accessed to have a index/length to access them, deleting a property would just be decrementing the length, not doing |this->props_end - 1|, so it seems like a perf wash. Is there anything in this that seems un-sensible? I don't see clear, new complexity to making the change, but I'd rather get a seems-right before tackling.

I went ahead and did comment 6. The renaming is a combination of style cleanup and forcing function to be sure I touched everything that could be affected by this. (I could style-fix the rest of this struct in a followup patch if desired, but I wanted to not grow this too much.) As far as size-consumption goes, we change from ... JSString* props_cursor; JSString* props_end; uint32_t guard_length, guard_key, flags; NativeIterator* next_; ... which including next_ (because it forces extra alignment on 64-bit) is 6 uint32_t on 32-bit, 10 uint32_t on 64-bit, to HeapReceiverGuard* guardsEnd_; JSString* propertyCursor_; JSString* propertiesEnd_; uint32_t guard_key, flags; NativeIterator* next_; ... which including next_ is 6 uint32_t on 32-bit, 10 uint32_t on 64-bit. So, no size-regression in this patch.

The 1-deletion variant is the only one that crashes. 1 gets you the offset that mis-interprets a Shape* as ObjectGroup*. But if you have 2 or 3 or presumably more, you're going to just hit a mismatch and nothing will happen. Or so it appears at casual glance -- the extra tests are because why not, not because anything breaks if you run them.

Comment on attachment 8979773 [details] [diff] [review] Patch that reorders guards to precede property strings and has pointer fields for guards-end, property-cursor, properties-end Review of attachment 8979773 [details] [diff] [review]: ----------------------------------------------------------------- Gah, I wish I had thought of the property suppression code messing with the properties list. ::: js/src/vm/Iteration.cpp @@ +63,5 @@ > // GC removes any elements from the list, it won't remove this one. > if (iterObj_) > TraceManuallyBarrieredEdge(trc, &iterObj_, "iterObj"); > + > + std::for_each(guardsBegin(), guardsEnd(), Fancy ::: js/src/vm/Iteration.h @@ +50,5 @@ > + // The next property, pointing into an array of strings directly after any > + // HeapReceiverGuards that appear directly after |*this|, as part of an > + // overall allocation that stores |*this|, receiver guards, and iterated > + // strings. > + GCPtrFlatString* propertyCursor_; // initialized by constructor Nit: for these public fields I'd prefer either making them private and adding accessors, or dropping the trailing _. That would also be consistent with {obj, guard_key, flags}.

(In reply to Jeff Walden [:Waldo] from comment #7) > (I could style-fix the rest of this struct in a followup > patch if desired, but I wanted to not grow this too much.) I missed this part. That's fine, also considering [fuzzblocker] status.

Kicked off a try-run, will land once that looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1295b639a833b12b2084a3d3cc318261da77ccf In light of comment 10, I didn't bother changing anything style-wise for comment 9's nit. Will get to after this has landed in a separate bug. Also per my usual practice, I'll hold off landing the test for a week or so as nicety to nightly users, even if technically I could just land it with the patch because nightly-only.

Landed the patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7c84e815c0c2eef81ac9cf8f5fa94b5a416029

JSBugMon: This bug has been automatically verified fixed.

Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4299313fa51 Add a test. r=jandem