Bug 209266

Summary: [JSC] StructureStubInfo::bufferedStructures should not ref/deref UniquedStringImpl
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch saam: review+

Description Yusuke Suzuki 2020-03-18 19:22:15 PDT 
This data structure can be destroyed in CodeBlock::finalizeUnconditionally. So it should not include Strings.

1. Can we just set AtomStringTable when executing CodeBlock::finalizeUnconditionally?

This does not work correctly. Our Web Worker implementation is releasing heapAccess() when finishing code execution and waiting for runloop message.
This means that CodeBlock::finalizeUnconditionally can potentially work concurrently to Web Worker's main thread.
Comment 1 Yusuke Suzuki 2020-03-18 19:22:46 PDT 
<rdar://problem/60508312>
Comment 2 Yusuke Suzuki 2020-03-18 19:24:14 PDT 
(In reply to Yusuke Suzuki from comment #0)
> 1. Can we just set AtomStringTable when executing
> CodeBlock::finalizeUnconditionally?
> 
> This does not work correctly. Our Web Worker implementation is releasing
> heapAccess() when finishing code execution and waiting for runloop message.
> This means that CodeBlock::finalizeUnconditionally can potentially work
> concurrently to Web Worker's main thread.

2. Can we fix this issue by giving up resetJITData call in CodeBlock::finalizeUnconditionally?

No since we are calling visitWeakReferences(), which removes some of buffered-structures in CodeBlock::finalizeUnconditionally anyway.
Comment 3 Yusuke Suzuki 2020-03-18 19:59:53 PDT 
Created attachment 393939 [details]
Patch
Comment 4 Yusuke Suzuki 2020-03-18 20:00:53 PDT 
Comment on attachment 393939 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOperations.cpp:-2239
> -            if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))

This patch also fixes DeleteByVal's considerCaching. It was not getting CacheableIdentifier.
Comment 5 Saam Barati 2020-03-19 11:31:29 PDT 
Comment on attachment 393939 [details]
Patch

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

r=me

Nice find

> Source/JavaScriptCore/ChangeLog:25
> +        concurrent collector to run this, we introduce m_bufferedStructuresLock in StructureStubInfo to guard m_bufferedStructures.

aren't we always holding the code block's lock in both cases?

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:102
> +    ALWAYS_INLINE bool considerCaching(VM& vm, CodeBlock* codeBlock, Structure* structure, CacheableIdentifier impl = CacheableIdentifier())

I wonder if making this have a default argument was a mistake

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:291
> +        using KeyTraits = SimpleClassHashTraits<BufferedStructure>;

nit: Maybe static assert that emptyValueIsZero for documentation purpose?
Comment 6 Yusuke Suzuki 2020-03-19 13:48:35 PDT 
Comment on attachment 393939 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:25
>> +        concurrent collector to run this, we introduce m_bufferedStructuresLock in StructureStubInfo to guard m_bufferedStructures.
> 
> aren't we always holding the code block's lock in both cases?

We are not holding a lock while executing considerCaching. I think fine-grained locking here is nice.

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:102
>> +    ALWAYS_INLINE bool considerCaching(VM& vm, CodeBlock* codeBlock, Structure* structure, CacheableIdentifier impl = CacheableIdentifier())
> 
> I wonder if making this have a default argument was a mistake

I think,

1. Adding `considerCachingById` and `considerCachingByVal`
2. `considerCachingById` internally calls `considerCaching` with CacheableIdentifier()

would be nice. Fixed.

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:291
>> +        using KeyTraits = SimpleClassHashTraits<BufferedStructure>;
> 
> nit: Maybe static assert that emptyValueIsZero for documentation purpose?

Sounds nice. Added.
Comment 7 Yusuke Suzuki 2020-03-19 14:59:36 PDT 
Committed r258732: <https://trac.webkit.org/changeset/258732>