[webkit-reviews] review granted: [Bug 200151] [JSC] Potential GC fix for JSPropertyNameEnumerator : [Attachment 375006] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 200151: [JSC] Potential GC fix for JSPropertyNameEnumerator
https://bugs.webkit.org/show_bug.cgi?id=200151

Attachment 375006: Patch

https://bugs.webkit.org/attachment.cgi?id=375006&action=review




--- Comment #6 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 375006
  --> https://bugs.webkit.org/attachment.cgi?id=375006
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/.