Attachment 341922 Details for Bug 186284 – patch

[patch] patch

a-backup.diff (text/plain), 3.12 KB, created by Saam Barati on 2018-06-04 15:05:14 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-06-04 15:05:14 PDT

Size: 3.12 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 232480)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,29 @@
>+2018-06-04  Saam Barati  <sbarati@apple.com>
>+
>+        Set the activeLength of all ScratchBuffers to zero when exiting the VM
>+        https://bugs.webkit.org/show_bug.cgi?id=186284
>+        <rdar://problem/40780738>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Simon recently found instances where we leak global objects from the
>+        ScratchBuffer. Yusuke found that we forgot to set the active length
>+        back to zero when doing catch OSR entry in the DFG/FTL. His solution
>+        to this was adding a node that cleared the active length. This is
>+        a good node to have, but it's not a complete solution: the DFG/FTL
>+        could OSR exit before that node executes, which would cause us to leak
>+        the data in it.
>+        
>+        This patch makes it so that we set each scratch buffer's active length
>+        to zero on VM exit. This helps prevent leaks for JS code that eventually
>+        exits the VM (which is essentially all code on the web and all API users).
>+
>+        * runtime/VM.cpp:
>+        (JSC::VM::clearScratchBufferContents):
>+        * runtime/VM.h:
>+        * runtime/VMEntryScope.cpp:
>+        (JSC::VMEntryScope::~VMEntryScope):
>+
> 2018-06-04  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         Get rid of UnconditionalFinalizers and WeakReferenceHarvesters
>Index: Source/JavaScriptCore/runtime/VM.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/VM.cpp	(revision 232480)
>+++ Source/JavaScriptCore/runtime/VM.cpp	(working copy)
>@@ -1233,4 +1233,11 @@ ScratchBuffer* VM::scratchBufferForSize(
>     return result;
> }
> 
>+void VM::clearScratchBufferContents()
>+{
>+    auto lock = holdLock(m_scratchBufferLock);
>+    for (auto* scratchBuffer : m_scratchBuffers)
>+        scratchBuffer->setActiveLength(0);
>+}
>+
> } // namespace JSC
>Index: Source/JavaScriptCore/runtime/VM.h
>===================================================================
>--- Source/JavaScriptCore/runtime/VM.h	(revision 232480)
>+++ Source/JavaScriptCore/runtime/VM.h	(working copy)
>@@ -672,7 +672,9 @@ public:
>     // The threading protocol here is as follows:
>     // - You can call scratchBufferForSize from any thread.
>     // - You can only set the ScratchBuffer's activeLength from the main thread.
>+    // - You can only write to entries in the ScratchBuffer from the main thread.
>     ScratchBuffer* scratchBufferForSize(size_t size);
>+    void clearScratchBufferContents();
> 
>     EncodedJSValue* exceptionFuzzingBuffer(size_t size)
>     {
>Index: Source/JavaScriptCore/runtime/VMEntryScope.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/VMEntryScope.cpp	(revision 232480)
>+++ Source/JavaScriptCore/runtime/VMEntryScope.cpp	(working copy)
>@@ -82,6 +82,8 @@ VMEntryScope::~VMEntryScope()
> 
>     for (auto& listener : m_didPopListeners)
>         listener();
>+
>+    m_vm.clearScratchBufferContents();
> }
> 
> } // namespace JSC

Flags:

keith_miller: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186284: 341922 | 341940