We need this word to store the forwarding pointer if the backing slots get put in the nursery. Since malloc isn't going to hand out less than one word anyway, this should not be any additional memory cost. We should also not need to track this size anywhere.

> Since malloc isn't going to hand out less than one word Don't be so sure. jemalloc is able to hand out 2-byte and 4-byte allocations, even though this (IIRC) violates the C standard. (The exception is Linux where we've disabled this capability -- for reasons I can't remember right now -- and the minimum is 8 bytes.)

Is the answer as simple as never allocating less than sizeof(RelocationOverlay) in Nursery::allocate()? I checked and this we were not allocating less than this when running jit-tests anyway. But I also couldn't see how ArrayBufferObject managed to allocate from the nursery.

qref'd patch this time

Comment on attachment 813094 [details] [diff] [review] reserve-space-for-forwarding Review of attachment 813094 [details] [diff] [review]: ----------------------------------------------------------------- This bug is actually about the slots and elements forwarding pointer, not about RelocationOverlay. The problem here is that Ion will pull out JSObject::{elements|slots} onto the stack or into a register and then hoist them above a minor GC. When we do a minor GC in this situation, if we move an object with inline or nursery allocated slots|elements, then the slots|elements pointer stored on the stack or in a register will be pointing into dead nursery space. The communication here is a bit hard to follow; it took me awhile to page back in, so let me write it down. When we call MarkRuntime from Nursery::collect, we end up in MarkIonFrame. This function first marks all of the object pointers it knows about. This ends up calling move{Slots|Elements}ToTenured, which calls set{Slots|Elements}ForwardingPointer from [1,2]. These functions write the new malloced pointer into the first word of the inline or nursery storage. When MarkIonFrame finishes marking all objects it knows about, it visits all reserved stack slots and spilled register slots [3] and calls Nursery::forwardBufferPointer on each one. This method checks if the pointer is into the nursery and if it is, reads the first word out of the array (which we wrote the new address into) and writes it back into the slot [4], updating the reference. 1 - http://dxr.allizom.org/mozilla-central/source/js/src/gc/Nursery.cpp#l530 (and some others below) 2 - http://dxr.allizom.org/mozilla-central/source/js/src/gc/Nursery.cpp#l497 3 - http://dxr.allizom.org/mozilla-central/source/js/src/jit/IonFrames.cpp#l828 (and below for stack slots) 4 - http://dxr.allizom.org/mozilla-central/source/js/src/gc/Nursery.cpp#l346 The worrisome case is [1] above: if the array buffer's physical allocation is shorter than 4 bytes on x86 or 8 bytes on x64, then this write will overwrite the buffer. ::: js/src/gc/Nursery.cpp @@ +97,5 @@ > { > JS_ASSERT(!runtime()->isHeapBusy()); > > + /* Ensure there's enough space to replace the contents with a RelocationOverlay. */ > + size = Max(size, sizeof(RelocationOverlay)); Please make this an assertion.

So both setElementsForwardingPointer and setSlotsForwarding pointer check there is space before writing the pointer. Is the idea that we should assert before we get to this point? As far as I can see ArrayBufferObjects are always created with the maximum number of fixed slots so I don't think it's actually a problem right now. Updated patch to assert in allocate() attached.

Comment on attachment 813506 [details] [diff] [review] assert-space-for-reloc-overlay Review of attachment 813506 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, thank you for auditing that! Since it is now known to be safe, please also assert that slot and element allocations are at least sizeof(void*).