Bug 203211 – Fix issues when setting public length on ArrayWithContiguous type butterflies.

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

[Mark Lam]

For ArrayWithContiguous type butterflies, SlotVisitor scans up to the public length of the butterfly.  When setting a new public length, if the new public length is greater than the current, we should always writeBarrier after the setting of the new public length.  Otherwise, there can be a race where the GC scans the butterfly after new values have been written to it but before the public length as been updated.  As a result, the new values never get scanned.

[Radar WebKit Bug Importer]

<rdar://problem/56476097>

[Mark Lam]

Created attachment 381468 [details]
proposed patch.

[Saam Barati]

Comment on attachment 381468 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSArray.cpp:887
> +    // Our memmoving of values around in the array could have concealed some of them from
> +    // the collector. Let's make sure that the collector scans this object again.
> +    vm.heap.writeBarrier(this);

how?

> Source/JavaScriptCore/runtime/JSArray.cpp:948
> +        if (indexingType == ArrayWithContiguous)
> +            vm.heap.writeBarrier(this);

why?

> Source/JavaScriptCore/runtime/JSArrayInlines.h:164
> +            // It's not effective to set the value with a barrier because in a
> +            // race, the collector may scan this object right after we barrier
> +            // the object (after setting of the value), but before updating the
> +            // public length. As a result, the GC may not have scanned the newly
> +            // pushed value.

this comment seems superfluous IMO

> Source/JavaScriptCore/runtime/JSObject.cpp:898
> +        // It's not effective to set the value with a barrier because in a
> +        // race, the collector may scan this object right after we barrier
> +        // the object (after setting of the value), but before updating the
> +        // public length. As a result, the GC may not have scanned the newly
> +        // set value.

ditto

> Source/JavaScriptCore/runtime/JSObject.h:416
> +            // It's not effective to set the value with a barrier because in a
> +            // race, the collector may scan this object right after we barrier
> +            // the object (after setting of the value), but before updating the
> +            // public length. As a result, the GC may not have scanned the newly
> +            // set value.

ditto

[Saam Barati]

Comment on attachment 381468 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSArray.cpp:948
>> +            vm.heap.writeBarrier(this);
> 
> why?

ignore this. it's an ok optimization to do

[Keith Miller]

Comment on attachment 381468 [details]
proposed patch.

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

r=me with comment on an existing but (I think)

> Source/JavaScriptCore/runtime/JSArray.cpp:946
>          // Our memmoving of values around in the array could have concealed some of them from
>          // the collector. Let's make sure that the collector scans this object again.

Doesn't memmove not guarantee anything about tearing while copying? That seems like a different bug though.

[Saam Barati]

Comment on attachment 381468 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSArray.cpp:887
>> +    vm.heap.writeBarrier(this);
> 
> how?

Note: we're holding the cell lock here b/c array storage. So I don't see how this does anything

[Mark Lam]

Comment on attachment 381468 [details]
proposed patch.

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

>>> Source/JavaScriptCore/runtime/JSArray.cpp:887
>>> +    vm.heap.writeBarrier(this);
>> 
>> how?
> 
> Note: we're holding the cell lock here b/c array storage. So I don't see how this does anything

Ok, I missed that detail.  Will remove.

>> Source/JavaScriptCore/runtime/JSArrayInlines.h:164
>> +            // pushed value.
> 
> this comment seems superfluous IMO

OK ok.  I added this comment because I didn't want the uninitiated reader to come along and naively reverse this change.  However, if so, I suppose he/she would think it's strange that I did it this way, and would do the right thing and read the ChangeLog first to checkout my rationale for this.  I'll remove the comment.

[Mark Lam]

Thanks for the reviews.  Landed in r251399: <http://trac.webkit.org/r251399>.