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.
<rdar://problem/56476097>
Created attachment 381468 [details] proposed patch.
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 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 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 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 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.
Thanks for the reviews. Landed in r251399: <http://trac.webkit.org/r251399>.