[JSC] Potential GC fix for JSPropertyEnumerator
Created attachment 374936 [details] Patch
Created attachment 374947 [details] Patch
<rdar://problem/23602966>
Created attachment 375005 [details] Patch
Created attachment 375006 [details] Patch
Comment on attachment 375006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375006&action=review Very nicely done. r=me > Source/JavaScriptCore/ChangeLog:8 > + We are seeing some JSPropertyNameEnumerator::visitChildren crashes for a long time. The crash frequency itself is not high, but it exists so long. I suggest /are seeing/have been seeing/ and /exists so long/has existed for a long time/. > Source/JavaScriptCore/ChangeLog:9 > + The crash happens when visiting m_propertyNames. It is also possible that this crash is caused by random corruption in somewhere, but JSPropertyNameEnumerator /in somewhere/somewhere/. > Source/JavaScriptCore/ChangeLog:13 > + We should use Auxiliary memory for this use case. And we should set this memory at a constructor so that I suggest /at a constructor/in the constructor/. > Source/JavaScriptCore/ChangeLog:20 > + In this patch, we align JSPropertyNameEnumerator implementation to the modern one to avoid using Vector<WriteBarrier<JSString>>. And we can make JSPropertyNameEnumerator > + non-destructible cell. Since JSCell's destructor is one of the cause of various issues, we should avoid it if we can. /make non-destructible cell/make a non-destructible cell/.
Comment on attachment 375006 [details] Patch Attachment 375006 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12819333 New failing tests: stress/for-in-stress.js.ftl-eager-no-cjit-b3o1 stress/for-in-stress.js.dfg-eager stress/for-in-stress.js.ftl-no-cjit-validate-sampling-profiler stress/for-in-stress.js.mini-mode stress/for-in-stress.js.ftl-no-cjit-no-put-stack-validate stress/for-in-stress.js.ftl-no-cjit-b3o0 stress/for-in-stress.js.no-cjit-validate-phases stress/for-in-stress.js.ftl-no-cjit-small-pool stress/for-in-stress.js.no-cjit-collect-continuously stress/for-in-stress.js.no-ftl stress/for-in-stress.js.ftl-eager stress/for-in-stress.js.no-llint stress/for-in-stress.js.bytecode-cache stress/for-in-stress.js.ftl-no-cjit-no-inline-validate stress/for-in-stress.js.default stress/for-in-stress.js.dfg-maximal-flush-validate-no-cjit stress/for-in-stress.js.dfg-eager-no-cjit-validate stress/for-in-stress.js.ftl-eager-no-cjit
(In reply to Build Bot from comment #7) > Comment on attachment 375006 [details] > Patch > > Attachment 375006 [details] did not pass jsc-ews (mac): > Output: https://webkit-queues.webkit.org/results/12819333 > > New failing tests: > stress/for-in-stress.js.ftl-eager-no-cjit-b3o1 > stress/for-in-stress.js.dfg-eager > stress/for-in-stress.js.ftl-no-cjit-validate-sampling-profiler > stress/for-in-stress.js.mini-mode > stress/for-in-stress.js.ftl-no-cjit-no-put-stack-validate > stress/for-in-stress.js.ftl-no-cjit-b3o0 > stress/for-in-stress.js.no-cjit-validate-phases > stress/for-in-stress.js.ftl-no-cjit-small-pool > stress/for-in-stress.js.no-cjit-collect-continuously > stress/for-in-stress.js.no-ftl > stress/for-in-stress.js.ftl-eager > stress/for-in-stress.js.no-llint > stress/for-in-stress.js.bytecode-cache > stress/for-in-stress.js.ftl-no-cjit-no-inline-validate > stress/for-in-stress.js.default > stress/for-in-stress.js.dfg-maximal-flush-validate-no-cjit > stress/for-in-stress.js.dfg-eager-no-cjit-validate > stress/for-in-stress.js.ftl-eager-no-cjit The iteration counter was too large, and timeout in the test harness. I'll decrease the counter to fit it in the timeout threshold.
Comment on attachment 375006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375006&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:8 >> + We are seeing some JSPropertyNameEnumerator::visitChildren crashes for a long time. The crash frequency itself is not high, but it exists so long. > > I suggest /are seeing/have been seeing/ and /exists so long/has existed for a long time/. Fixed. >> Source/JavaScriptCore/ChangeLog:9 >> + The crash happens when visiting m_propertyNames. It is also possible that this crash is caused by random corruption in somewhere, but JSPropertyNameEnumerator > > /in somewhere/somewhere/. Fixed. >> Source/JavaScriptCore/ChangeLog:13 >> + We should use Auxiliary memory for this use case. And we should set this memory at a constructor so that > > I suggest /at a constructor/in the constructor/. Fixed. >> Source/JavaScriptCore/ChangeLog:20 >> + non-destructible cell. Since JSCell's destructor is one of the cause of various issues, we should avoid it if we can. > > /make non-destructible cell/make a non-destructible cell/. Fixed.
Created attachment 375014 [details] Patch for landing
Committed r247888: <https://trac.webkit.org/changeset/247888>