Attachment 341506 Details for Bug 185996 – Patch

[patch] Patch

bug-185996-20180529123825.patch (text/plain), 16.62 KB, created by Keith Miller on 2018-05-29 12:38:26 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-05-29 12:38:26 PDT

Size: 16.62 KB

patch

obsolete

>Subversion Revision: 232157
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 3e27107cfe4ea3c1b6381a9e1125c6d179eb91b6..7a04290bdd2fc92a38d58c8a55063baa1eec2311 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,41 @@
>+2018-05-29  Keith Miller  <keith_miller@apple.com>
>+
>+        Error instances should not strongly hold onto StackFrames
>+        https://bugs.webkit.org/show_bug.cgi?id=185996
>+
>+        Reviewed by Mark Lam.
>+
>+        Previously, we would hold onto all the StackFrames until the the user
>+        looked at one of the properties on the Error object. This patch makes us
>+        only weakly retain the StackFrames and collect all the information
>+        if we are about to collect any frame.
>+
>+        This patch also adds a method to $vm that returns the heaps count
>+        of live global objects.
>+
>+        * heap/Heap.cpp:
>+        (JSC::Heap::finalizeUnconditionalFinalizers):
>+        * interpreter/Interpreter.cpp:
>+        (JSC::Interpreter::stackTraceAsString):
>+        * interpreter/Interpreter.h:
>+        * runtime/Error.cpp:
>+        (JSC::addErrorInfo):
>+        * runtime/ErrorInstance.cpp:
>+        (JSC::ErrorInstance::finalizeUnconditionally):
>+        (JSC::ErrorInstance::computeErrorInfo):
>+        (JSC::ErrorInstance::materializeErrorInfoIfNeeded):
>+        (JSC::ErrorInstance::visitChildren): Deleted.
>+        * runtime/ErrorInstance.h:
>+        (JSC::ErrorInstance::subspaceFor):
>+        * runtime/StackFrame.h:
>+        (JSC::StackFrame::isMarked const):
>+        * runtime/VM.cpp:
>+        (JSC::VM::VM):
>+        * runtime/VM.h:
>+        * tools/JSDollarVM.cpp:
>+        (JSC::functionGlobalObjectCount):
>+        (JSC::JSDollarVM::finishCreation):
>+
> 2018-05-23  Keith Miller  <keith_miller@apple.com>
> 
>         Expose $vm if window.internals is exposed
>diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
>index 58670edabf217f9ecf9b9c308116ea9f510c5d65..a681acedaa549b029d45d20f07d8f665aedeaee8 100644
>--- a/Source/JavaScriptCore/heap/Heap.cpp
>+++ b/Source/JavaScriptCore/heap/Heap.cpp
>@@ -591,6 +591,7 @@ void Heap::finalizeUnconditionalFinalizers()
>     finalizeMarkedUnconditionalFinalizers<ExecutableToCodeBlockEdge>(vm()->executableToCodeBlockEdgesWithFinalizers);
>     finalizeMarkedUnconditionalFinalizers<JSWeakSet>(vm()->weakSetSpace);
>     finalizeMarkedUnconditionalFinalizers<JSWeakMap>(vm()->weakMapSpace);
>+    finalizeMarkedUnconditionalFinalizers<ErrorInstance>(vm()->errorInstanceSpace);
> 
> #if ENABLE(WEBASSEMBLY)
>     finalizeMarkedUnconditionalFinalizers<JSWebAssemblyCodeBlock>(vm()->webAssemblyCodeBlockSpace);
>diff --git a/Source/JavaScriptCore/interpreter/Interpreter.cpp b/Source/JavaScriptCore/interpreter/Interpreter.cpp
>index bb623cac694cf95204246a3342edf9ff2d45851e..f2ce53cf13a45746cb57c65097be55b1c1e9f107 100644
>--- a/Source/JavaScriptCore/interpreter/Interpreter.cpp
>+++ b/Source/JavaScriptCore/interpreter/Interpreter.cpp
>@@ -575,7 +575,7 @@ void Interpreter::getStackTrace(JSCell* owner, Vector<StackFrame>& results, size
>     ASSERT(results.size() == results.capacity());
> }
> 
>-JSString* Interpreter::stackTraceAsString(VM& vm, const Vector<StackFrame>& stackTrace)
>+String Interpreter::stackTraceAsString(VM& vm, const Vector<StackFrame>& stackTrace)
> {
>     // FIXME: JSStringJoiner could be more efficient than StringBuilder here.
>     StringBuilder builder;
>@@ -584,7 +584,7 @@ JSString* Interpreter::stackTraceAsString(VM& vm, const Vector<StackFrame>& stac
>         if (i != stackTrace.size() - 1)
>             builder.append('\n');
>     }
>-    return jsString(&vm, builder.toString());
>+    return builder.toString();
> }
> 
> ALWAYS_INLINE static HandlerInfo* findExceptionHandler(StackVisitor& visitor, CodeBlock* codeBlock, RequiredHandler requiredHandler)
>diff --git a/Source/JavaScriptCore/interpreter/Interpreter.h b/Source/JavaScriptCore/interpreter/Interpreter.h
>index 384d3dda7e58ced1d0a0ee5fc975baffe42f352a..49227ebe515663ffde03c9e0a3fcb64967a0f568 100644
>--- a/Source/JavaScriptCore/interpreter/Interpreter.h
>+++ b/Source/JavaScriptCore/interpreter/Interpreter.h
>@@ -120,7 +120,7 @@ namespace JSC {
>         NEVER_INLINE HandlerInfo* unwind(VM&, CallFrame*&, Exception*, UnwindStart);
>         void notifyDebuggerOfExceptionToBeThrown(VM&, CallFrame*, Exception*);
>         NEVER_INLINE void debug(CallFrame*, DebugHookType);
>-        static JSString* stackTraceAsString(VM&, const Vector<StackFrame>&);
>+        static String stackTraceAsString(VM&, const Vector<StackFrame>&);
> 
>         static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);
>         static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
>diff --git a/Source/JavaScriptCore/runtime/Error.cpp b/Source/JavaScriptCore/runtime/Error.cpp
>index 601f823656a1ccef9889a8905d96d2f217749f85..64ae736f117ab9721d6829627b930e81bdeaaf9c 100644
>--- a/Source/JavaScriptCore/runtime/Error.cpp
>+++ b/Source/JavaScriptCore/runtime/Error.cpp
>@@ -209,7 +209,7 @@ bool addErrorInfo(VM& vm, Vector<StackFrame>* stackTrace, JSObject* obj)
> {
>     if (!stackTrace)
>         return false;
>-    
>+
>     if (!stackTrace->isEmpty()) {
>         unsigned line;
>         unsigned column;
>@@ -220,7 +220,7 @@ bool addErrorInfo(VM& vm, Vector<StackFrame>* stackTrace, JSObject* obj)
>         if (!sourceURL.isEmpty())
>             obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, sourceURL));
> 
>-        obj->putDirect(vm, vm.propertyNames->stack, Interpreter::stackTraceAsString(vm, *stackTrace), static_cast<unsigned>(PropertyAttribute::DontEnum));
>+        obj->putDirect(vm, vm.propertyNames->stack, jsString(&vm, Interpreter::stackTraceAsString(vm, *stackTrace)), static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
>         return true;
>     }
>diff --git a/Source/JavaScriptCore/runtime/ErrorInstance.cpp b/Source/JavaScriptCore/runtime/ErrorInstance.cpp
>index c1724289145c9cde44295258560a034357d9272a..09176fd076a6da5f543ade7228e9fd7ca0eba8c7 100644
>--- a/Source/JavaScriptCore/runtime/ErrorInstance.cpp
>+++ b/Source/JavaScriptCore/runtime/ErrorInstance.cpp
>@@ -23,6 +23,7 @@
> 
> #include "CodeBlock.h"
> #include "InlineCallFrame.h"
>+#include "Interpreter.h"
> #include "JSScope.h"
> #include "JSCInlines.h"
> #include "ParseInt.h"
>@@ -202,17 +203,50 @@ String ErrorInstance::sanitizedToString(ExecState* exec)
>     return builder.toString();
> }
> 
>+void ErrorInstance::finalizeUnconditionally(VM& vm)
>+{
>+    if (!m_stackTrace)
>+        return;
>+
>+    // We don't want to keep our stack traces alive forever if the user doesn't access the stack trace.
>+    // If we did, we might end up keeping functions (and their global objects) alive that happened to
>+    // get caught in a trace.
>+    for (const auto& frame : *m_stackTrace.get()) {
>+        if (!frame.isMarked()) {
>+            computeErrorInfo(vm);
>+            return;
>+        }
>+    }
>+}
>+
>+void ErrorInstance::computeErrorInfo(VM& vm)
>+{
>+    ASSERT(!m_errorInfoMaterialized);
>+
>+    if (m_stackTrace && !m_stackTrace->isEmpty()) {
>+        getLineColumnAndSource(m_stackTrace.get(), m_line, m_column, m_sourceURL);
>+        m_stackString = Interpreter::stackTraceAsString(vm, *m_stackTrace.get());
>+        m_stackTrace = nullptr;
>+    }
>+}
>+
> bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
> {
>     if (m_errorInfoMaterialized)
>         return false;
>-    
>-    addErrorInfo(vm, m_stackTrace.get(), this);
>-    {
>-        auto locker = holdLock(cellLock());
>-        m_stackTrace = nullptr;
>-    }
>-    
>+
>+    computeErrorInfo(vm);
>+
>+    if (!m_stackString.isNull()) {
>+        putDirect(vm, vm.propertyNames->line, jsNumber(m_line));
>+        putDirect(vm, vm.propertyNames->column, jsNumber(m_column));
>+        if (!m_sourceURL.isEmpty())
>+            putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, WTFMove(m_sourceURL)));
>+
>+        putDirect(vm, vm.propertyNames->stack, jsString(&vm, WTFMove(m_stackString)), static_cast<unsigned>(PropertyAttribute::DontEnum));
>+    } else
>+        putDirect(vm, vm.propertyNames->stack, vm.smallStrings.emptyString(), static_cast<unsigned>(PropertyAttribute::DontEnum));
>+
>     m_errorInfoMaterialized = true;
>     return true;
> }
>@@ -227,21 +261,6 @@ bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm, PropertyName propertyNa
>     return false;
> }
> 
>-void ErrorInstance::visitChildren(JSCell* cell, SlotVisitor& visitor)
>-{
>-    ErrorInstance* thisObject = jsCast<ErrorInstance*>(cell);
>-    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
>-    Base::visitChildren(thisObject, visitor);
>-
>-    {
>-        auto locker = holdLock(thisObject->cellLock());
>-        if (thisObject->m_stackTrace) {
>-            for (StackFrame& frame : *thisObject->m_stackTrace)
>-                frame.visitChildren(visitor);
>-        }
>-    }
>-}
>-
> bool ErrorInstance::getOwnPropertySlot(JSObject* object, ExecState* exec, PropertyName propertyName, PropertySlot& slot)
> {
>     VM& vm = exec->vm();
>diff --git a/Source/JavaScriptCore/runtime/ErrorInstance.h b/Source/JavaScriptCore/runtime/ErrorInstance.h
>index 56b3ae28e2b66cbbcba7a9685cfa1dc262cc22c1..d95cbf81b40169581a5e2a144959bf72e49e2534 100644
>--- a/Source/JavaScriptCore/runtime/ErrorInstance.h
>+++ b/Source/JavaScriptCore/runtime/ErrorInstance.h
>@@ -72,27 +72,39 @@ public:
>     bool materializeErrorInfoIfNeeded(VM&);
>     bool materializeErrorInfoIfNeeded(VM&, PropertyName);
> 
>+    template<typename CellType>
>+    static IsoSubspace* subspaceFor(VM& vm)
>+    {
>+        return &vm.errorInstanceSpace;
>+    }
>+
>+    void finalizeUnconditionally(VM&);
>+
> protected:
>     explicit ErrorInstance(VM&, Structure*);
> 
>     void finishCreation(ExecState*, VM&, const String&, bool useCurrentFrame = true);
>     static void destroy(JSCell*);
> 
>-    static void visitChildren(JSCell*, SlotVisitor&);
>-
>     static bool getOwnPropertySlot(JSObject*, ExecState*, PropertyName, PropertySlot&);
>     static void getOwnNonIndexPropertyNames(JSObject*, ExecState*, PropertyNameArray&, EnumerationMode);
>     static void getStructurePropertyNames(JSObject*, ExecState*, PropertyNameArray&, EnumerationMode);
>     static bool defineOwnProperty(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool shouldThrow);
>     static bool put(JSCell*, ExecState*, PropertyName, JSValue, PutPropertySlot&);
>     static bool deleteProperty(JSCell*, ExecState*, PropertyName);
>-    
>+
>+    void computeErrorInfo(VM&);
>+
>     SourceAppender m_sourceAppender { nullptr };
>+    std::unique_ptr<Vector<StackFrame>> m_stackTrace;
>+    unsigned m_line;
>+    unsigned m_column;
>+    String m_sourceURL;
>+    String m_stackString;
>     RuntimeType m_runtimeTypeForCause { TypeNothing };
>     bool m_stackOverflowError { false };
>     bool m_outOfMemoryError { false };
>     bool m_errorInfoMaterialized { false };
>-    std::unique_ptr<Vector<StackFrame>> m_stackTrace;
> };
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/StackFrame.h b/Source/JavaScriptCore/runtime/StackFrame.h
>index 2877723e2c8380e0161b44e03ce76c87501f1813..6bb03ecc0123f7fb180651019e20e3053b47ba6d 100644
>--- a/Source/JavaScriptCore/runtime/StackFrame.h
>+++ b/Source/JavaScriptCore/runtime/StackFrame.h
>@@ -57,6 +57,7 @@ public:
>     }
>     
>     void visitChildren(SlotVisitor&);
>+    bool isMarked() const { return Heap::isMarked(m_callee.get()) && (!m_codeBlock || Heap::isMarked(m_codeBlock.get())); }
> 
> private:
>     WriteBarrier<JSCell> m_callee { };
>diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
>index 2af9d0a4eb8fc7068ce7318962e7ee7d7930ef1c..d5c9e15fb006f3f78dc92cf4c6a397cc44eb9e5a 100644
>--- a/Source/JavaScriptCore/runtime/VM.cpp
>+++ b/Source/JavaScriptCore/runtime/VM.cpp
>@@ -305,6 +305,7 @@ VM::VM(VMType vmType, HeapType heapType)
>     , structureSpace ISO_SUBSPACE_INIT(heap, destructibleCellHeapCellType.get(), Structure)
>     , weakSetSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakSet)
>     , weakMapSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakMap)
>+    , errorInstanceSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), ErrorInstance)
> #if ENABLE(WEBASSEMBLY)
>     , webAssemblyCodeBlockSpace ISO_SUBSPACE_INIT(heap, webAssemblyCodeBlockHeapCellType.get(), JSWebAssemblyCodeBlock)
>     , webAssemblyFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), WebAssemblyFunction)
>diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
>index 77e282ce238b5868be5c4374956e5e21a5793e6e..689465ae9f17f834ae671fa9a1fb019ef1aa7ca4 100644
>--- a/Source/JavaScriptCore/runtime/VM.h
>+++ b/Source/JavaScriptCore/runtime/VM.h
>@@ -384,6 +384,7 @@ public:
>     IsoSubspace structureSpace;
>     IsoSubspace weakSetSpace;
>     IsoSubspace weakMapSpace;
>+    IsoSubspace errorInstanceSpace;
> #if ENABLE(WEBASSEMBLY)
>     IsoSubspace webAssemblyCodeBlockSpace;
>     IsoSubspace webAssemblyFunctionSpace;
>diff --git a/Source/JavaScriptCore/tools/JSDollarVM.cpp b/Source/JavaScriptCore/tools/JSDollarVM.cpp
>index 3f466808bd76f7db3926cfd95c101b1044d7d191..7131e793577a41edd7df5c9fb6e3d60b3bd3d78f 100644
>--- a/Source/JavaScriptCore/tools/JSDollarVM.cpp
>+++ b/Source/JavaScriptCore/tools/JSDollarVM.cpp
>@@ -1687,6 +1687,11 @@ static EncodedJSValue JSC_HOST_CALL functionEnableExceptionFuzz(ExecState*)
>     return JSValue::encode(jsUndefined());
> }
> 
>+static EncodedJSValue JSC_HOST_CALL functionGlobalObjectCount(ExecState* exec)
>+{
>+    return JSValue::encode(jsNumber(exec->vm().heap.globalObjectCount()));
>+}
>+
> static EncodedJSValue JSC_HOST_CALL functionGlobalObjectForObject(ExecState* exec)
> {
>     JSValue value = exec->argument(0);
>@@ -1843,6 +1848,7 @@ void JSDollarVM::finishCreation(VM& vm)
> 
>     addFunction(vm, "enableExceptionFuzz", functionEnableExceptionFuzz, 0);
> 
>+    addFunction(vm, "globalObjectCount", functionGlobalObjectCount, 0);
>     addFunction(vm, "globalObjectForObject", functionGlobalObjectForObject, 1);
> 
>     addFunction(vm, "getGetterSetter", functionGetGetterSetter, 2);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 07e030c709b3bda2c9c95c5ae964317584514a7d..7a7b53afd14eba512796792feeb4c6d74eeddb5d 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-05-29  Keith Miller  <keith_miller@apple.com>
>+
>+        Error instances should not strongly hold onto StackFrames
>+        https://bugs.webkit.org/show_bug.cgi?id=185996
>+
>+        Reviewed by Mark Lam.
>+
>+        * js/error-should-not-strong-reference-global-object-expected.txt: Added.
>+        * js/error-should-not-strong-reference-global-object.html: Added.
>+
> 2018-05-24  Chris Dumez  <cdumez@apple.com>
> 
>         [iOS WK2] Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https.html is a flaky failure
>diff --git a/LayoutTests/js/error-should-not-strong-reference-global-object-expected.txt b/LayoutTests/js/error-should-not-strong-reference-global-object-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..43e5eeedbf1b435f96c3a20bf81401aee4331981
>--- /dev/null
>+++ b/LayoutTests/js/error-should-not-strong-reference-global-object-expected.txt
>@@ -0,0 +1,4 @@
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/js/error-should-not-strong-reference-global-object.html b/LayoutTests/js/error-should-not-strong-reference-global-object.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..0365cc61c5eedcbd50473391b204dee5f4e962c5
>--- /dev/null
>+++ b/LayoutTests/js/error-should-not-strong-reference-global-object.html
>@@ -0,0 +1,51 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<html>
>+<head>
>+<script src="../resources/js-test-pre.js"></script>
>+</head>
>+<body>
>+<script>
>+function foo(frames) {
>+    frames = document.getElementsByTagName("iframe");
>+    for (let i = 1; i < frames.length; i++) {
>+
>+        document.body.removeChild(frames[i]);
>+    }
>+    throw new Error(0);
>+}
>+
>+const iframeCount = 10;
>+function setup() {
>+    for (let i = 0; i < iframeCount; ++i) {
>+        let iframe = document.createElement("iframe");
>+        document.body.appendChild(iframe);
>+        iframe.contentWindow.foo = new iframe.contentWindow.Function("frames", "i", "frames[i].foo(frames, i - 1);");
>+    }
>+}
>+
>+let errors = [];
>+function run() {
>+    setup();
>+    let frames = window.frames;
>+
>+    frames = [window].concat(Array.from(frames));
>+    let last = frames.length - 1;
>+    try {
>+        frames[last].foo(frames, last);
>+    } catch (e) {
>+        errors.push(e);
>+    }
>+}
>+
>+for (let i = 0; i < 50; i++)
>+    run();
>+
>+$vm.gc();
>+// We shouldn't have more than 10% of the global objects we allocated.
>+if ($vm.globalObjectCount() >= 51)
>+    throw new Error("There are more global objects than there should be");
>+
>+</script>
>+<script src="../resources/js-test-post.js"></script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 185996: 341329 | 341334 | 341498 | 341501 | 341503 | 341505 | 341506 | 341511 | 341517 | 341518 | 341532 | 341547 | 341551 | 341554 | 341555 | 341559 | 341579 | 341581 | 341586 | 341587 | 341596 | 341598 | 341605