Bug 200151 – [JSC] Potential GC fix for JSPropertyNameEnumerator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Potential GC fix for JSPropertyEnumerator

[Yusuke Suzuki]

Created attachment 374936 [details]
Patch

[Yusuke Suzuki]

Created attachment 374947 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/23602966>

[Yusuke Suzuki]

Created attachment 375005 [details]
Patch

[Yusuke Suzuki]

Created attachment 375006 [details]
Patch

[Mark Lam]

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/.

[EWS Watchlist]

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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 375014 [details]
Patch for landing

[Yusuke Suzuki]

Committed r247888: <https://trac.webkit.org/changeset/247888>