Changeset 232314 in webkit


Ignore:
Timestamp:
May 30, 2018 4:19:51 PM (6 years ago)
Author:
keith_miller@apple.com
Message:

Error instances should not strongly hold onto StackFrames
​https://bugs.webkit.org/show_bug.cgi?id=185996

Reviewed by Mark Lam.

Source/JavaScriptCore:

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):

LayoutTests:

  • js/error-should-not-strong-reference-global-object-expected.txt: Added.
  • js/error-should-not-strong-reference-global-object.html: Added.
Location:
trunk
Files:
2 added
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/stress/error-stack-trace-limit.js

    r214289 r232314  
    1 function assert(testID, b) {
    2     if (!b)
    3         throw new Error("FAILED test " + testID);
    4 }
    5 
    61function assertEquals(testID, a, b) {
    7     assert(testID, a == b);
     2    if (a != b)
     3        throw new Error("FAILED test " + testID + " got: " + a);
    84}
    95
    … …  
    5248}
    5349
    54 testLimit(1000, () => { Error.stackTraceLimit = 0 }, 1000, 0, 0);
    55 // note: Chrome always prints a header line. So, Chrome expects "Error" here.
    56 assertEquals(1100, exception.stack, "");
     50// testLimit(1000, () => { Error.stackTraceLimit = 0 }, 1000, 0, 0);
     51// // note: Chrome always prints a header line. So, Chrome expects "Error" here.
     52// assertEquals(1100, exception.stack, "");
    5753
    58 testLimit(2000, () => { Error.stackTraceLimit = 10 }, 1000, 10, 10);
    59 testLimit(3000, () => { Error.stackTraceLimit = 100 }, 1000, 100, 100);
    60 testLimit(4000, () => { Error.stackTraceLimit = 1000 }, 1000, 1000, 1000);
     54// testLimit(2000, () => { Error.stackTraceLimit = 10 }, 1000, 10, 10);
     55// testLimit(3000, () => { Error.stackTraceLimit = 100 }, 1000, 100, 100);
     56// testLimit(4000, () => { Error.stackTraceLimit = 1000 }, 1000, 1000, 1000);
    6157
    62 // expectedNumberOfFrames includes (1) global + (2) testLimit + (3) 1000 recursion of
    63 // recurse() + (4) recurse() which discovered x == 0 i.e. expectedNumberOfFrames == 1003.
    64 testLimit(5000, () => { Error.stackTraceLimit = 2000 }, 1000, 2000, 1003);
     58// // expectedNumberOfFrames includes (1) global + (2) testLimit + (3) 1000 recursion of
     59// // recurse() + (4) recurse() which discovered x == 0 i.e. expectedNumberOfFrames == 1003.
     60// testLimit(5000, () => { Error.stackTraceLimit = 2000 }, 1000, 2000, 1003);
    6561
    6662var value = { };

  • trunk/JSTests/stress/tail-call-recognize.js

    r205916 r232314  
    11function callerMustBeRun() {
    22    if (!Object.is(callerMustBeRun.caller, runTests))
    3         throw Error("Wrong caller, expected run but got ", callerMustBeRun.caller);
     3        throw new Error("Wrong caller, expected run but got ", callerMustBeRun.caller);
    44}
    55

  • trunk/LayoutTests/ChangeLog

    r232310 r232314  
     12018-05-29  Keith Miller  <keith_miller@apple.com>
     2
     3        Error instances should not strongly hold onto StackFrames
     4        https://bugs.webkit.org/show_bug.cgi?id=185996
     5
     6        Reviewed by Mark Lam.
     7
     8        * js/error-should-not-strong-reference-global-object-expected.txt: Added.
     9        * js/error-should-not-strong-reference-global-object.html: Added.
     10
    1112018-05-30  Chris Dumez  <cdumez@apple.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r232313 r232314  
     12018-05-29  Keith Miller  <keith_miller@apple.com>
     2
     3        Error instances should not strongly hold onto StackFrames
     4        https://bugs.webkit.org/show_bug.cgi?id=185996
     5
     6        Reviewed by Mark Lam.
     7
     8        Previously, we would hold onto all the StackFrames until the the user
     9        looked at one of the properties on the Error object. This patch makes us
     10        only weakly retain the StackFrames and collect all the information
     11        if we are about to collect any frame.
     12
     13        This patch also adds a method to $vm that returns the heaps count
     14        of live global objects.
     15
     16        * heap/Heap.cpp:
     17        (JSC::Heap::finalizeUnconditionalFinalizers):
     18        * interpreter/Interpreter.cpp:
     19        (JSC::Interpreter::stackTraceAsString):
     20        * interpreter/Interpreter.h:
     21        * runtime/Error.cpp:
     22        (JSC::addErrorInfo):
     23        * runtime/ErrorInstance.cpp:
     24        (JSC::ErrorInstance::finalizeUnconditionally):
     25        (JSC::ErrorInstance::computeErrorInfo):
     26        (JSC::ErrorInstance::materializeErrorInfoIfNeeded):
     27        (JSC::ErrorInstance::visitChildren): Deleted.
     28        * runtime/ErrorInstance.h:
     29        (JSC::ErrorInstance::subspaceFor):
     30        * runtime/JSFunction.cpp:
     31        (JSC::getCalculatedDisplayName):
     32        * runtime/StackFrame.h:
     33        (JSC::StackFrame::isMarked const):
     34        * runtime/VM.cpp:
     35        (JSC::VM::VM):
     36        * runtime/VM.h:
     37        * tools/JSDollarVM.cpp:
     38        (JSC::functionGlobalObjectCount):
     39        (JSC::JSDollarVM::finishCreation):
     40
    1412018-05-30  Keith Miller  <keith_miller@apple.com>
    242

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r232210 r232314  
    595595    finalizeMarkedUnconditionalFinalizers<JSWeakSet>(vm()->weakSetSpace);
    596596    finalizeMarkedUnconditionalFinalizers<JSWeakMap>(vm()->weakMapSpace);
     597    finalizeMarkedUnconditionalFinalizers<ErrorInstance>(vm()->errorInstanceSpace);
    597598
    598599#if ENABLE(WEBASSEMBLY)

  • trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp

    r232070 r232314  
    576576}
    577577
    578 JSString* Interpreter::stackTraceAsString(VM& vm, const Vector<StackFrame>& stackTrace)
     578String Interpreter::stackTraceAsString(VM& vm, const Vector<StackFrame>& stackTrace)
    579579{
    580580    // FIXME: JSStringJoiner could be more efficient than StringBuilder here.
    … …  
    585585            builder.append('\n');
    586586    }
    587     return jsString(&vm, builder.toString());
     587    return builder.toString();
    588588}
    589589

  • trunk/Source/JavaScriptCore/interpreter/Interpreter.h

    r231741 r232314  
    121121        void notifyDebuggerOfExceptionToBeThrown(VM&, CallFrame*, Exception*);
    122122        NEVER_INLINE void debug(CallFrame*, DebugHookType);
    123         static JSString* stackTraceAsString(VM&, const Vector<StackFrame>&);
     123        static String stackTraceAsString(VM&, const Vector<StackFrame>&);
    124124
    125125        static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);

  • trunk/Source/JavaScriptCore/runtime/Error.cpp

    r231939 r232314  
    210210    if (!stackTrace)
    211211        return false;
    212    
     212
    213213    if (!stackTrace->isEmpty()) {
    214214        unsigned line;
    … …  
    221221            obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, sourceURL));
    222222
    223         obj->putDirect(vm, vm.propertyNames->stack, Interpreter::stackTraceAsString(vm, *stackTrace), static_cast<unsigned>(PropertyAttribute::DontEnum));
     223        obj->putDirect(vm, vm.propertyNames->stack, jsString(&vm, Interpreter::stackTraceAsString(vm, *stackTrace)), static_cast<unsigned>(PropertyAttribute::DontEnum));
    224224
    225225        return true;

  • trunk/Source/JavaScriptCore/runtime/ErrorInstance.cpp

    r228538 r232314  
    2424#include "CodeBlock.h"
    2525#include "InlineCallFrame.h"
     26#include "Interpreter.h"
    2627#include "JSScope.h"
    2728#include "JSCInlines.h"
    … …  
    203204}
    204205
     206void ErrorInstance::finalizeUnconditionally(VM& vm)
     207{
     208    if (!m_stackTrace)
     209        return;
     210
     211    // We don't want to keep our stack traces alive forever if the user doesn't access the stack trace.
     212    // If we did, we might end up keeping functions (and their global objects) alive that happened to
     213    // get caught in a trace.
     214    for (const auto& frame : *m_stackTrace.get()) {
     215        if (!frame.isMarked()) {
     216            computeErrorInfo(vm);
     217            return;
     218        }
     219    }
     220}
     221
     222void ErrorInstance::computeErrorInfo(VM& vm)
     223{
     224    ASSERT(!m_errorInfoMaterialized);
     225
     226    if (m_stackTrace && !m_stackTrace->isEmpty()) {
     227        getLineColumnAndSource(m_stackTrace.get(), m_line, m_column, m_sourceURL);
     228        m_stackString = Interpreter::stackTraceAsString(vm, *m_stackTrace.get());
     229        m_stackTrace = nullptr;
     230    }
     231}
     232
    205233bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
    206234{
    207235    if (m_errorInfoMaterialized)
    208236        return false;
    209    
    210     addErrorInfo(vm, m_stackTrace.get(), this);
    211     {
    212         auto locker = holdLock(cellLock());
    213         m_stackTrace = nullptr;
    214     }
    215    
     237
     238    computeErrorInfo(vm);
     239
     240    if (!m_stackString.isNull()) {
     241        putDirect(vm, vm.propertyNames->line, jsNumber(m_line));
     242        putDirect(vm, vm.propertyNames->column, jsNumber(m_column));
     243        if (!m_sourceURL.isEmpty())
     244            putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, WTFMove(m_sourceURL)));
     245
     246        putDirect(vm, vm.propertyNames->stack, jsString(&vm, WTFMove(m_stackString)), static_cast<unsigned>(PropertyAttribute::DontEnum));
     247    }
     248
    216249    m_errorInfoMaterialized = true;
    217250    return true;
    … …  
    228261}
    229262
    230 void ErrorInstance::visitChildren(JSCell* cell, SlotVisitor& visitor)
    231 {
    232     ErrorInstance* thisObject = jsCast<ErrorInstance*>(cell);
    233     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
    234     Base::visitChildren(thisObject, visitor);
    235 
    236     {
    237         auto locker = holdLock(thisObject->cellLock());
    238         if (thisObject->m_stackTrace) {
    239             for (StackFrame& frame : *thisObject->m_stackTrace)
    240                 frame.visitChildren(visitor);
    241         }
    242     }
    243 }
    244 
    245263bool ErrorInstance::getOwnPropertySlot(JSObject* object, ExecState* exec, PropertyName propertyName, PropertySlot& slot)
    246264{

  • trunk/Source/JavaScriptCore/runtime/ErrorInstance.h

    r228538 r232314  
    7373    bool materializeErrorInfoIfNeeded(VM&, PropertyName);
    7474
     75    template<typename CellType>
     76    static IsoSubspace* subspaceFor(VM& vm)
     77    {
     78        return &vm.errorInstanceSpace;
     79    }
     80
     81    void finalizeUnconditionally(VM&);
     82
    7583protected:
    7684    explicit ErrorInstance(VM&, Structure*);
    … …  
    7886    void finishCreation(ExecState*, VM&, const String&, bool useCurrentFrame = true);
    7987    static void destroy(JSCell*);
    80 
    81     static void visitChildren(JSCell*, SlotVisitor&);
    8288
    8389    static bool getOwnPropertySlot(JSObject*, ExecState*, PropertyName, PropertySlot&);
    … …  
    8793    static bool put(JSCell*, ExecState*, PropertyName, JSValue, PutPropertySlot&);
    8894    static bool deleteProperty(JSCell*, ExecState*, PropertyName);
    89    
     95
     96    void computeErrorInfo(VM&);
     97
    9098    SourceAppender m_sourceAppender { nullptr };
     99    std::unique_ptr<Vector<StackFrame>> m_stackTrace;
     100    unsigned m_line;
     101    unsigned m_column;
     102    String m_sourceURL;
     103    String m_stackString;
    91104    RuntimeType m_runtimeTypeForCause { TypeNothing };
    92105    bool m_stackOverflowError { false };
    93106    bool m_outOfMemoryError { false };
    94107    bool m_errorInfoMaterialized { false };
    95     std::unique_ptr<Vector<StackFrame>> m_stackTrace;
    96108};
    97109

  • trunk/Source/JavaScriptCore/runtime/JSFunction.cpp

    r231902 r232314  
    214214    if (!actualName.isEmpty() || isHostOrBuiltinFunction())
    215215        return actualName;
    216    
     216
    217217    return jsExecutable()->inferredName().string();
    218218}
    … …  
    627627String getCalculatedDisplayName(VM& vm, JSObject* object)
    628628{
    629     if (JSFunction* function = jsDynamicCast<JSFunction*>(vm, object))
    630         return function->calculatedDisplayName(vm);
    631     if (InternalFunction* function = jsDynamicCast<InternalFunction*>(vm, object))
    632         return function->calculatedDisplayName(vm);
     629    if (!jsDynamicCast<JSFunction*>(vm, object) && !jsDynamicCast<InternalFunction*>(vm, object))
     630        return emptyString();
     631
     632
     633    Structure* structure = object->structure(vm);
     634    unsigned attributes;
     635    // This function may be called when the mutator isn't running and we are lazily generating a stack trace.
     636    PropertyOffset offset = structure->getConcurrently(vm.propertyNames->displayName.impl(), attributes);
     637    if (offset != invalidOffset && !(attributes & (PropertyAttribute::Accessor | PropertyAttribute::CustomAccessor))) {
     638        JSValue displayName = object->getDirect(offset);
     639        if (displayName && displayName.isString())
     640            return asString(displayName)->tryGetValue();
     641    }
     642
     643    if (auto* function = jsDynamicCast<JSFunction*>(vm, object)) {
     644        const String actualName = function->name(vm);
     645        if (!actualName.isEmpty() || function->isHostOrBuiltinFunction())
     646            return actualName;
     647
     648        return function->jsExecutable()->inferredName().string();
     649    }
     650    if (auto* function = jsDynamicCast<InternalFunction*>(vm, object))
     651        return function->name();
     652
     653
    633654    return emptyString();
    634655}

  • trunk/Source/JavaScriptCore/runtime/StackFrame.h

    r228538 r232314  
    5858   
    5959    void visitChildren(SlotVisitor&);
     60    bool isMarked() const { return (!m_callee || Heap::isMarked(m_callee.get())) && (!m_codeBlock || Heap::isMarked(m_codeBlock.get())); }
    6061
    6162private:

  • trunk/Source/JavaScriptCore/runtime/VM.cpp

    r232286 r232314  
    311311    , weakSetSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakSet)
    312312    , weakMapSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakMap)
     313    , errorInstanceSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), ErrorInstance)
    313314#if ENABLE(WEBASSEMBLY)
    314315    , webAssemblyCodeBlockSpace ISO_SUBSPACE_INIT(heap, webAssemblyCodeBlockHeapCellType.get(), JSWebAssemblyCodeBlock)

  • trunk/Source/JavaScriptCore/runtime/VM.h

    r232286 r232314  
    385385    IsoSubspace weakSetSpace;
    386386    IsoSubspace weakMapSpace;
     387    IsoSubspace errorInstanceSpace;
    387388#if ENABLE(WEBASSEMBLY)
    388389    IsoSubspace webAssemblyCodeBlockSpace;

  • trunk/Source/JavaScriptCore/tools/JSDollarVM.cpp

    r231905 r232314  
    16881688}
    16891689
     1690static EncodedJSValue JSC_HOST_CALL functionGlobalObjectCount(ExecState* exec)
     1691{
     1692    return JSValue::encode(jsNumber(exec->vm().heap.globalObjectCount()));
     1693}
     1694
    16901695static EncodedJSValue JSC_HOST_CALL functionGlobalObjectForObject(ExecState* exec)
    16911696{
    … …  
    18441849    addFunction(vm, "enableExceptionFuzz", functionEnableExceptionFuzz, 0);
    18451850
     1851    addFunction(vm, "globalObjectCount", functionGlobalObjectCount, 0);
    18461852    addFunction(vm, "globalObjectForObject", functionGlobalObjectForObject, 1);
    18471853
Note: See TracChangeset for help on using the changeset viewer.