WebKit Bugzilla
Attachment 341547 Details for
Bug 185996
: Error instances should not strongly hold onto StackFrames
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-185996-20180529184812.patch (text/plain), 18.16 KB, created by
Keith Miller
on 2018-05-29 18:48:13 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Keith Miller
Created:
2018-05-29 18:48:13 PDT
Size:
18.16 KB
patch
obsolete
>Subversion Revision: 232157 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 3e27107cfe4ea3c1b6381a9e1125c6d179eb91b6..0538e1004929d20262ea1660eef75fa20980b3e2 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,43 @@ >+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/JSFunction.cpp: >+ (JSC::getCalculatedDisplayName): >+ * 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/JSFunction.cpp b/Source/JavaScriptCore/runtime/JSFunction.cpp >index 17e77e59e0d53453cb15f6fb1648bdcfe1840ff2..fe253d7de64aa14fe3a4c0451fd0fc1d4d11761a 100644 >--- a/Source/JavaScriptCore/runtime/JSFunction.cpp >+++ b/Source/JavaScriptCore/runtime/JSFunction.cpp >@@ -626,10 +626,20 @@ ConstructType JSFunction::getConstructData(JSCell* cell, ConstructData& construc > > String getCalculatedDisplayName(VM& vm, JSObject* object) > { >- if (JSFunction* function = jsDynamicCast<JSFunction*>(vm, object)) >- return function->calculatedDisplayName(vm); >- if (InternalFunction* function = jsDynamicCast<InternalFunction*>(vm, object)) >- return function->calculatedDisplayName(vm); >+ if (!jsDynamicCast<JSFunction*>(vm, object) && !jsDynamicCast<InternalFunction*>(vm, object)) >+ return emptyString(); >+ >+ >+ Structure* structure = object->structure(vm); >+ unsigned attributes; >+ // This function may be called when the mutator isn't running and we are lazily generating a stack trace. >+ PropertyOffset offset = structure->getConcurrently(vm.propertyNames->displayName.impl(), attributes); >+ if (offset == invalidOffset || (attributes & (PropertyAttribute::Accessor | PropertyAttribute::CustomAccessor))) >+ return emptyString(); >+ >+ JSValue displayName = object->getDirect(offset); >+ if (displayName && displayName.isString()) >+ return asString(displayName)->tryGetValue(); > return emptyString(); > } > >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>
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
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