[webkit-reviews] review granted: [Bug 214403] [JSC] Touch UnlinkedCodeBlock from CodeBlock destructor by using unvalidatedGet : [Attachment 404433] 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 214403: [JSC] Touch UnlinkedCodeBlock from CodeBlock destructor by using
unvalidatedGet
https://bugs.webkit.org/show_bug.cgi?id=214403

Attachment 404433: Patch

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




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 404433
  --> https://bugs.webkit.org/attachment.cgi?id=404433
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404433&action=review

r=me if EWS bots are green.  The mac-wk1 bot was red but I told it to re-run
the test.  If the failure is real, please fix before landing.  Thanks.

> Source/JavaScriptCore/ChangeLog:3
> +	   [JSC] Touch UnlinkedCodeBlock from CodeBlock destructor by using
unvalidatedGet

I suggest renaming this title to "Use unvalidatedGet instead of get to access
UnlinkedCodeBlock from CodeBlock destructor".  The current title reads to me
like an imperative to add some code to access the UnlinkedCodeBlock, whereas
what you meant is that when we access it, we need to use unvalidatedGet.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:820
> +    // We use unvalidatedGet because validation rejects get() access when we
are destroying cells.

I suggest rephrasing as "because get() has a validation assertion that rejects
access".  Mentioning the "assertion" here also sets the context for the next
point.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:821
> +    // This is right assertion since destruction order of cells is not
defined, so member cells would get destroyed already.

I suggest rephrasing as "This assertion is correct since destruction order of
cells is not guaranteed, and member cells could already be destroyed."

> JSTests/stress/codeblock-destructor-access-unlinkedcodeblock.js:2
> +//@ runDefault("--returnEarlyFromInfiniteLoopsForFuzzing=1")
> +//@ slow!

Please skip for memoryLimited.	I'm using memoryLimited here as a proxy for
slow machines here.  The armv7 and mips bots are showing that this tests times
out there.  I'm guessing this might also be the case for other embedded /
memory limited devices.