Just a driveby improvement, arguably not strictly necessary for a fix here. If there's a cleaner/clearer way to store |tmp1 + sizeof(NativeIterator)| into props_cursor in a single masm operation, let me know -- I couldn't find anything in the MacroAssembler interface in MA.h.

It's rather silly that we had this field before. Tho in fairness, the replacement has reinterpret_cast<> in it, so it's not like it's the user-friendliest improvement. :-|

This should play nice with the C++ type system to the utmost degree possible: every C++ object has its lifetime begun consistent with the spec, even the trailer properties/guards. Only constructors and such get called to initialize things. It is kind of beautiful, by which I mean it doesn't actually do the awful things everyone wants to do but do wrong. Well, except for the fallibility thinng, which is a little bit awkward. I don't see any way around that. But this way *does* have clear benefits like ensuring |props.length()| isn't carried around in two separate ways in two separate places. Note that NativeIterator's compiler-generated destructor is never called -- PropertyIteratorObject::finalize only frees, doesn't delete. If it *were* ever called, the JS_POISON would be wrong, because you couldn't call the destructor on NativeIterator::obj at a minimum. I have a separate patch I wrote that *would* have made such get called, and that destructor would call destructors on the trailing properties/guards. But then I remembered the poisoning thing for sentinels, and I decided it wasn't worth figuring out a hackaround to make the patch usable. I'll post it here 'cause it's mostly done and maybe someone will want to pick it up (doubtful), but I don't intend to push on it any more.

Oh, actually this patch *would* work -- sentinels are destroyed by JSCompartment which just calls js_free, not js_delete. Had I noticed five minutes ago maybe I'd have improved this that way, but I don't think *I* can be bothered now. SEP.

Comment on attachment 8976784 [details] [diff] [review] Remove NativeIterator::props_array (it's trivial to recalculate it when it's needed), and add a bunch of alignment assertions verifying the delicate memory layout of NativeIterator followed by the (only dynamically known number of) properties it iterates Review of attachment 8976784 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +9924,5 @@ > > // Reset property cursor. > + Address propCursor(temp1, offsetof(NativeIterator, props_cursor)); > + masm.storePtr(temp1, propCursor); > + masm.addPtr(Imm32(sizeof(NativeIterator)), propCursor); This is fine, but the following might be simpler/faster: masm.computeEffectiveAddress(Address(temp1, sizeof(NativeIterator)), temp2); masm.storePtr(temp2, propsCursor); temp2 is clobbered below so it should be fine to use it here as well. ::: js/src/vm/Iteration.h @@ +59,5 @@ > + static_assert(alignof(NativeIterator) >= alignof(GCPtrFlatString), > + "GCPtrFlatStrings for properties must be able to appear " > + "directly after NativeIterator, with no padding space " > + "required for correct alignment"); > + const NativeIterator* immediatelyAfter = this + 1; Maybe add a brief comment here warning about JIT code inlining this.

Comment on attachment 8976789 [details] [diff] [review] Initialize NativeIterator objects (and any associated property name strings and HeapReceiverGuards) all within a single constructor call, without using PodZero Review of attachment 8976789 [details] [diff] [review]: ----------------------------------------------------------------- These patches are pretty nice actually :)

Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae525ee499f Remove NativeIterator::props_array (it's trivial to recalculate it when it's needed), and add a bunch of alignment assertions verifying the delicate memory layout of NativeIterator followed by the (only dynamically known number of) properties it iterates followed by the (only dynamically known number of) ReceiverGuards it uses. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/7d83c7de9404 Remove NativeIterator::guard_array: its numeric value is identical to NativeIterator::props_end. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/7491ab23247f Initialize NativeIterator objects (and any associated property name strings and HeapReceiverGuards) all within a single constructor call, without using PodZero. r=jandem

https://hg.mozilla.org/mozilla-central/rev/6ae525ee499f https://hg.mozilla.org/mozilla-central/rev/7d83c7de9404 https://hg.mozilla.org/mozilla-central/rev/7491ab23247f

Per bug 1462261 comment 7 (written after querying me as to what rev was responsible), this might have been a perf improvement. That or one of a bunch of other revs in that push, so who really knows.