Summary: | Fix issues when setting public length on ArrayWithContiguous type butterflies. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2019-10-21 14:10:28 PDT
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>. |