Bug 214403 – [JSC] Use unvalidatedGet instead of get to access UnlinkedCodeBlock from CodeBlock destructor

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

[Yusuke Suzuki]

[JSC] Touch UnlinkedCodeBlock from CodeBlock destructor by using unvalidatedGet

[Yusuke Suzuki]

Created attachment 404433 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/65527229>

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 404474 [details]
Patch

[Yusuke Suzuki]

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