Bug 214403

Created attachment 404433 [details]
Patch

<rdar://problem/65527229>

Comment on attachment 404433 [details]
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.

Comment on attachment 404433 [details]
Patch

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

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

Changed.

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

Changed.

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

Changed.

>> JSTests/stress/codeblock-destructor-access-unlinkedcodeblock.js:2
>> +//@ 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.

OK, added.

Created attachment 404474 [details]
Patch

Committed r264473: <https://trac.webkit.org/changeset/264473>