Bug 175604 - Make VM::scratchBufferForSize thread safe
Summary: Make VM::scratchBufferForSize thread safe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks: 174590
  Show dependency treegraph
 
Reported: 2017-08-15 16:03 PDT by Saam Barati
Modified: 2017-08-15 17:04 PDT (History)
13 users (show)

See Also:


Attachments
patch (5.95 KB, patch)
2017-08-15 16:29 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-08-15 16:03:36 PDT
We currently call it from many threads, but it's not thread safe ...
Comment 1 Saam Barati 2017-08-15 16:29:21 PDT
Created attachment 318196 [details]
patch
Comment 2 Build Bot 2017-08-15 16:32:08 PDT
Attachment 318196 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VM.h:574:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Geoffrey Garen 2017-08-15 16:48:11 PDT
Comment on attachment 318196 [details]
patch

r=me
Comment 4 Mark Lam 2017-08-15 16:50:10 PDT
Comment on attachment 318196 [details]
patch

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

r=me with issues resolved.

> Source/JavaScriptCore/runtime/VM.cpp:1025
> +        ScratchBuffer* newBuffer = ScratchBuffer::create(m_sizeOfLastScratchBuffer);

Make sure that the newly allocated buffer's activeLength is set to 0.  I'm not sure it's an issue, but I'm thinking of GC scanning it incorrectly if the active length is not set.
Comment 5 Saam Barati 2017-08-15 17:01:29 PDT
Comment on attachment 318196 [details]
patch

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

>> Source/JavaScriptCore/runtime/VM.cpp:1025
>> +        ScratchBuffer* newBuffer = ScratchBuffer::create(m_sizeOfLastScratchBuffer);
> 
> Make sure that the newly allocated buffer's activeLength is set to 0.  I'm not sure it's an issue, but I'm thinking of GC scanning it incorrectly if the active length is not set.

ScratchBuffer() already sets it to zero
Comment 6 Saam Barati 2017-08-15 17:04:16 PDT
landed in:
https://trac.webkit.org/changeset/220777/webkit
Comment 7 Radar WebKit Bug Importer 2017-08-15 17:04:48 PDT
<rdar://problem/33907882>