Bug 188151 – We should zero unused property storage when rebalancing array storage.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

When putting a new property, JSObject::prepareToPutDirectWithoutTransition() and JSObject::putDirectInternal() currently always asserts that the target slot contains an effectively empty value (and is safe for GC scanning).  This assertion may be false if the slot is from the existing butterfly and not a newly allocated one.  For example, an array splice operation may shift entries in the existing butterfly, and therefore, result in unused property slots containing valid values.  As a result, the slot for the new property may already contain a non-empty value (before the put) that is safe for GC.  But because it is non-empty, it will fail the assertion.

We should fix the assertion to only do this empty check if the butterfly is actually newly allocated.

<rdar://problem/42020385>

[Mark Lam]

Created attachment 346046 [details]
proposed patch.

[Saam Barati]

Maybe I’m missing something here but it feels like this patch is loosing a lot of the value of the assertion.

Also, you say that slot has a valid value. But is it guaranteed to be scanned during GC even if it’s an unused slot?

[Mark Lam]

(In reply to Saam Barati from comment #2)
> Maybe I’m missing something here but it feels like this patch is loosing a
> lot of the value of the assertion.

There's a more conservative way I can do this, that is: when asserts are enabled, nullify the shifted out slot when splicing.  I'll look into that.

> Also, you say that slot has a valid value. But is it guaranteed to be
> scanned during GC even if it’s an unused slot?

I should also double check this.  If your stipulation is true, then these assertions may be meaningless entirely.

[Mark Lam]

Comment on attachment 346046 [details]
proposed patch.

r- while I investigate additional details brought up by Saam.

[Keith Miller]

Comment on attachment 346046 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=346046&action=review

> Source/JavaScriptCore/runtime/JSObjectInlines.h:210
> +            ASSERT(!needToAllocateNewButterfly || getDirect(offset).isEffectivelyEmptyAndSafeForGCScanning());

This is wrong.

It's wrong to ever have bits that look like a cell in an unused slot. The slot may or may not get scanned do to raciness of our GC. So if it an unused slot had a stale cell in it we might deference scavenged or otherwise bad memory and crash.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:337
> +        ASSERT(!needToAllocateNewButterfly || getDirect(offset).isEffectivelyEmptyAndSafeForGCScanning());

ditto.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:394
> +    ASSERT(!needToAllocateNewButterfly || getDirect(offset).isEffectivelyEmptyAndSafeForGCScanning());

ditto.

[Saam Barati]

(In reply to Mark Lam from comment #3)
> (In reply to Saam Barati from comment #2)
> > Maybe I’m missing something here but it feels like this patch is loosing a
> > lot of the value of the assertion.
> 
> There's a more conservative way I can do this, that is: when asserts are
> enabled, nullify the shifted out slot when splicing.  I'll look into that.
> 
> > Also, you say that slot has a valid value. But is it guaranteed to be
> > scanned during GC even if it’s an unused slot?
> 
> I should also double check this.  If your stipulation is true, then these
> assertions may be meaningless entirely.

I didn't mean entirely. I just mean the splice case here. Do we skip the dead space in butterfly from splice when visiting or no? (I think we call that area the precapacity?)

[Keith Miller]

Created attachment 350877 [details]
Patch

[Michael Saboff]

Comment on attachment 350877 [details]
Patch

r=me

[Keith Miller]

Committed r236514: <https://trac.webkit.org/changeset/236514>