Bug 203211

Summary: Fix issues when setting public length on ArrayWithContiguous type butterflies.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. keith_miller: review+

Description Mark Lam 2019-10-21 14:10:28 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-10-21 14:10:57 PDT
<rdar://problem/56476097>
Comment 2 Mark Lam 2019-10-21 15:42:18 PDT
Created attachment 381468 [details]
proposed patch.
Comment 3 Saam Barati 2019-10-21 16:26:39 PDT
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
Comment 4 Saam Barati 2019-10-21 16:27:10 PDT
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
Comment 5 Keith Miller 2019-10-21 16:28:33 PDT
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.
Comment 6 Saam Barati 2019-10-21 16:30:22 PDT
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
Comment 7 Mark Lam 2019-10-21 16:54:40 PDT
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.
Comment 8 Mark Lam 2019-10-21 17:05:33 PDT
Thanks for the reviews.  Landed in r251399: <http://trac.webkit.org/r251399>.