889986 - GenerationalGC: ensure that all ArrayBufferObject backing allocations are at least one word

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

> 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.)

Comment 2

[Jon Coppeard (:jonco)]

Attachment: reserve-space-for-forwarding

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.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: reserve-space-for-forwarding

qref'd patch this time

Comment 4

[Terrence Cole [:terrence]]

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.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: assert-space-for-reloc-overlay

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 6

[Terrence Cole [:terrence]]

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*).

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f15e5ca2bf6

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3f15e5ca2bf6