Title: [221836] trunk
Revision
221836
Author
fpi...@apple.com
Date
2017-09-10 12:00:03 -0700 (Sun, 10 Sep 2017)

Log Message

Error should compute .stack and friends lazily
https://bugs.webkit.org/show_bug.cgi?id=176645

Reviewed by Saam Barati.
JSTests:


* ChakraCore.yaml: Skip test that was testing non-standard behavior of these fields.
* microbenchmarks/new-error.js: Added.
* microbenchmarks/throw.js: Added.

Source/_javascript_Core:

        
Building the string portion of the stack trace after we walk the stack accounts for most of
the cost of computing the .stack property. So, this patch makes ErrorInstance hold onto the
Vector<StackFrame> so that it can build the string only once it's really needed.
        
This is an enormous speed-up for programs that allocate and throw exceptions.
        
It's a 5.6x speed-up for "new Error()" with a stack that is 4 functions deep.
        
It's a 2.2x speed-up for throwing and catching an Error.
        
It's a 1.17x speed-up for the WSL test suite (which throws a lot).
        
It's a significant speed-up on many of our existing try-catch microbenchmarks. For example,
delta-blue-try-catch is 1.16x faster.

* interpreter/Interpreter.cpp:
(JSC::GetStackTraceFunctor::GetStackTraceFunctor):
(JSC::GetStackTraceFunctor::operator() const):
(JSC::Interpreter::getStackTrace):
* interpreter/Interpreter.h:
* runtime/Error.cpp:
(JSC::getStackTrace):
(JSC::getBytecodeOffset):
(JSC::addErrorInfo):
(JSC::addErrorInfoAndGetBytecodeOffset): Deleted.
* runtime/Error.h:
* runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::ErrorInstance):
(JSC::ErrorInstance::finishCreation):
(JSC::ErrorInstance::materializeErrorInfoIfNeeded):
(JSC::ErrorInstance::visitChildren):
(JSC::ErrorInstance::getOwnPropertySlot):
(JSC::ErrorInstance::getOwnNonIndexPropertyNames):
(JSC::ErrorInstance::defineOwnProperty):
(JSC::ErrorInstance::put):
(JSC::ErrorInstance::deleteProperty):
* runtime/ErrorInstance.h:
* runtime/Exception.cpp:
(JSC::Exception::visitChildren):
(JSC::Exception::finishCreation):
* runtime/Exception.h:
* runtime/StackFrame.cpp:
(JSC::StackFrame::visitChildren):
* runtime/StackFrame.h:
(JSC::StackFrame::StackFrame):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChakraCore.yaml (221835 => 221836)


--- trunk/JSTests/ChakraCore.yaml	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/JSTests/ChakraCore.yaml	2017-09-10 19:00:03 UTC (rev 221836)
@@ -1245,7 +1245,8 @@
 - path: ChakraCore/test/StackTrace/ErrorPrototype.js
   cmd: runChakra :baseline, "NoException", "ErrorPrototype.baseline-jsc", ["TrimStackTracePath.js"]
 - path: ChakraCore/test/StackTrace/ErrorDotStackAlreadyExists.js
-  cmd: runChakra :baseline, "NoException", "ErrorDotStackAlreadyExists.baseline-jsc", []
+  # Tests non-standard behavior.
+  cmd: runChakra :skip, "NoException", "ErrorDotStackAlreadyExists.baseline-jsc", []
 - path: ChakraCore/test/StackTrace/FunctionName.js
   cmd: runChakra :baseline, "NoException", "FunctionName.js.baseline-jsc", ["TrimStackTracePath.js"]
 - path: ChakraCore/test/StackTrace/x64StackWalk.js

Modified: trunk/JSTests/ChangeLog (221835 => 221836)


--- trunk/JSTests/ChangeLog	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/JSTests/ChangeLog	2017-09-10 19:00:03 UTC (rev 221836)
@@ -1,3 +1,14 @@
+2017-09-09  Filip Pizlo  <fpi...@apple.com>
+
+        Error should compute .stack and friends lazily
+        https://bugs.webkit.org/show_bug.cgi?id=176645
+
+        Reviewed by Saam Barati.
+
+        * ChakraCore.yaml: Skip test that was testing non-standard behavior of these fields.
+        * microbenchmarks/new-error.js: Added.
+        * microbenchmarks/throw.js: Added.
+
 2017-09-09  Mark Lam  <mark....@apple.com>
 
         [Re-landing] Use JIT probes for DFG OSR exit.

Added: trunk/JSTests/microbenchmarks/new-error.js (0 => 221836)


--- trunk/JSTests/microbenchmarks/new-error.js	                        (rev 0)
+++ trunk/JSTests/microbenchmarks/new-error.js	2017-09-10 19:00:03 UTC (rev 221836)
@@ -0,0 +1,17 @@
+function foo()
+{
+    for (var i = 0; i < 100000; ++i)
+        new Error();
+}
+
+function bar()
+{
+    foo();
+}
+
+function baz()
+{
+    bar();
+}
+
+baz();

Added: trunk/JSTests/microbenchmarks/throw.js (0 => 221836)


--- trunk/JSTests/microbenchmarks/throw.js	                        (rev 0)
+++ trunk/JSTests/microbenchmarks/throw.js	2017-09-10 19:00:03 UTC (rev 221836)
@@ -0,0 +1,28 @@
+function foo()
+{
+    throw new Error();
+}
+
+function bar()
+{
+    foo();
+}
+
+function baz()
+{
+    bar();
+}
+
+function thingy()
+{
+    try {
+        baz();
+    } catch (e) {
+        if (e.constructor != Error)
+            throw new Error("Bad error: " + e);
+    }
+}
+
+for (var i = 0; i < 10000; ++i)
+    thingy();
+

Modified: trunk/Source/_javascript_Core/ChangeLog (221835 => 221836)


--- trunk/Source/_javascript_Core/ChangeLog	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/ChangeLog	2017-09-10 19:00:03 UTC (rev 221836)
@@ -1,3 +1,56 @@
+2017-09-08  Filip Pizlo  <fpi...@apple.com>
+
+        Error should compute .stack and friends lazily
+        https://bugs.webkit.org/show_bug.cgi?id=176645
+
+        Reviewed by Saam Barati.
+        
+        Building the string portion of the stack trace after we walk the stack accounts for most of
+        the cost of computing the .stack property. So, this patch makes ErrorInstance hold onto the
+        Vector<StackFrame> so that it can build the string only once it's really needed.
+        
+        This is an enormous speed-up for programs that allocate and throw exceptions.
+        
+        It's a 5.6x speed-up for "new Error()" with a stack that is 4 functions deep.
+        
+        It's a 2.2x speed-up for throwing and catching an Error.
+        
+        It's a 1.17x speed-up for the WSL test suite (which throws a lot).
+        
+        It's a significant speed-up on many of our existing try-catch microbenchmarks. For example,
+        delta-blue-try-catch is 1.16x faster.
+
+        * interpreter/Interpreter.cpp:
+        (JSC::GetStackTraceFunctor::GetStackTraceFunctor):
+        (JSC::GetStackTraceFunctor::operator() const):
+        (JSC::Interpreter::getStackTrace):
+        * interpreter/Interpreter.h:
+        * runtime/Error.cpp:
+        (JSC::getStackTrace):
+        (JSC::getBytecodeOffset):
+        (JSC::addErrorInfo):
+        (JSC::addErrorInfoAndGetBytecodeOffset): Deleted.
+        * runtime/Error.h:
+        * runtime/ErrorInstance.cpp:
+        (JSC::ErrorInstance::ErrorInstance):
+        (JSC::ErrorInstance::finishCreation):
+        (JSC::ErrorInstance::materializeErrorInfoIfNeeded):
+        (JSC::ErrorInstance::visitChildren):
+        (JSC::ErrorInstance::getOwnPropertySlot):
+        (JSC::ErrorInstance::getOwnNonIndexPropertyNames):
+        (JSC::ErrorInstance::defineOwnProperty):
+        (JSC::ErrorInstance::put):
+        (JSC::ErrorInstance::deleteProperty):
+        * runtime/ErrorInstance.h:
+        * runtime/Exception.cpp:
+        (JSC::Exception::visitChildren):
+        (JSC::Exception::finishCreation):
+        * runtime/Exception.h:
+        * runtime/StackFrame.cpp:
+        (JSC::StackFrame::visitChildren):
+        * runtime/StackFrame.h:
+        (JSC::StackFrame::StackFrame):
+
 2017-09-09  Mark Lam  <mark....@apple.com>
 
         [Re-landing] Use JIT probes for DFG OSR exit.

Modified: trunk/Source/_javascript_Core/inspector/ScriptCallStack.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/inspector/ScriptCallStack.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/inspector/ScriptCallStack.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -33,6 +33,7 @@
 #include "ScriptCallStack.h"
 
 #include "InspectorValues.h"
+#include <wtf/DataLog.h>
 
 namespace Inspector {
 

Modified: trunk/Source/_javascript_Core/inspector/ScriptCallStackFactory.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/inspector/ScriptCallStackFactory.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/inspector/ScriptCallStackFactory.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -118,7 +118,7 @@
     return ScriptCallStack::create(frames);
 }
 
-static void extractSourceInformationFromException(JSC::ExecState* exec, JSObject* exceptionObject, int* lineNumber, int* columnNumber, String* sourceURL)
+static bool extractSourceInformationFromException(JSC::ExecState* exec, JSObject* exceptionObject, int* lineNumber, int* columnNumber, String* sourceURL)
 {
     VM& vm = exec->vm();
     auto scope = DECLARE_CATCH_SCOPE(vm);
@@ -125,12 +125,29 @@
 
     // FIXME: <http://webkit.org/b/115087> Web Inspector: Should not need to evaluate _javascript_ handling exceptions
     JSValue lineValue = exceptionObject->getDirect(vm, Identifier::fromString(exec, "line"));
-    *lineNumber = lineValue && lineValue.isNumber() ? int(lineValue.toNumber(exec)) : 0;
     JSValue columnValue = exceptionObject->getDirect(vm, Identifier::fromString(exec, "column"));
-    *columnNumber = columnValue && columnValue.isNumber() ? int(columnValue.toNumber(exec)) : 0;
     JSValue sourceURLValue = exceptionObject->getDirect(vm, Identifier::fromString(exec, "sourceURL"));
-    *sourceURL = sourceURLValue && sourceURLValue.isString() ? sourceURLValue.toWTFString(exec) : ASCIILiteral("undefined");
+    
+    bool result = false;
+    if (lineValue && lineValue.isNumber()
+        && sourceURLValue && sourceURLValue.isString()) {
+        *lineNumber = int(lineValue.toNumber(exec));
+        *columnNumber = columnValue && columnValue.isNumber() ? int(columnValue.toNumber(exec)) : 0;
+        *sourceURL = sourceURLValue.toWTFString(exec);
+        result = true;
+    } else if (ErrorInstance* error = jsDynamicCast<ErrorInstance*>(vm, exceptionObject)) {
+        unsigned unsignedLine;
+        unsigned unsignedColumn;
+        result = getLineColumnAndSource(error->stackTrace(), unsignedLine, unsignedColumn, *sourceURL);
+        *lineNumber = static_cast<int>(unsignedLine);
+        *columnNumber = static_cast<int>(unsignedColumn);
+    }
+    
+    if (sourceURL->isEmpty())
+        *sourceURL = ASCIILiteral("undefined");
+    
     scope.clearException();
+    return result;
 }
 
 Ref<ScriptCallStack> createScriptCallStackFromException(JSC::ExecState* exec, JSC::Exception* exception, size_t maxStackSize)
@@ -154,13 +171,18 @@
         int columnNumber;
         String exceptionSourceURL;
         if (!frames.size()) {
-            extractSourceInformationFromException(exec, exceptionObject, &lineNumber, &columnNumber, &exceptionSourceURL);
-            frames.append(ScriptCallFrame(String(), exceptionSourceURL, noSourceID, lineNumber, columnNumber));
+            if (extractSourceInformationFromException(exec, exceptionObject, &lineNumber, &columnNumber, &exceptionSourceURL))
+                frames.append(ScriptCallFrame(String(), exceptionSourceURL, noSourceID, lineNumber, columnNumber));
         } else {
+            // FIXME: The typical stack trace will have a native frame at the top, and consumers of
+            // this code already know this (see JSDOMExceptionHandling.cpp's reportException, for
+            // example - it uses firstNonNativeCallFrame). This looks like it splats something else
+            // over it. That something else is probably already at stackTrace[1].
+            // https://bugs.webkit.org/show_bug.cgi?id=176663
             if (!stackTrace[0].hasLineAndColumnInfo() || stackTrace[0].sourceURL().isEmpty()) {
                 const ScriptCallFrame& firstCallFrame = frames.first();
-                extractSourceInformationFromException(exec, exceptionObject, &lineNumber, &columnNumber, &exceptionSourceURL);
-                frames[0] = ScriptCallFrame(firstCallFrame.functionName(), exceptionSourceURL, stackTrace[0].sourceID(), lineNumber, columnNumber);
+                if (extractSourceInformationFromException(exec, exceptionObject, &lineNumber, &columnNumber, &exceptionSourceURL))
+                    frames[0] = ScriptCallFrame(firstCallFrame.functionName(), exceptionSourceURL, stackTrace[0].sourceID(), lineNumber, columnNumber);
             }
         }
     }

Modified: trunk/Source/_javascript_Core/interpreter/Interpreter.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/interpreter/Interpreter.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/interpreter/Interpreter.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -479,8 +479,9 @@
 
 class GetStackTraceFunctor {
 public:
-    GetStackTraceFunctor(VM& vm, Vector<StackFrame>& results, size_t framesToSkip, size_t capacity)
+    GetStackTraceFunctor(VM& vm, JSCell* owner, Vector<StackFrame>& results, size_t framesToSkip, size_t capacity)
         : m_vm(vm)
+        , m_owner(owner)
         , m_results(results)
         , m_framesToSkip(framesToSkip)
         , m_remainingCapacityForFrameCapture(capacity)
@@ -500,10 +501,10 @@
                 m_results.append(StackFrame::wasm(visitor->wasmFunctionIndexOrName()));
             } else if (!!visitor->codeBlock() && !visitor->codeBlock()->unlinkedCodeBlock()->isBuiltinFunction()) {
                 m_results.append(
-                    StackFrame(m_vm, visitor->callee().asCell(), visitor->codeBlock(), visitor->bytecodeOffset()));
+                    StackFrame(m_vm, m_owner, visitor->callee().asCell(), visitor->codeBlock(), visitor->bytecodeOffset()));
             } else {
                 m_results.append(
-                    StackFrame(m_vm, visitor->callee().asCell()));
+                    StackFrame(m_vm, m_owner, visitor->callee().asCell()));
             }
     
             m_remainingCapacityForFrameCapture--;
@@ -514,13 +515,15 @@
 
 private:
     VM& m_vm;
+    JSCell* m_owner;
     Vector<StackFrame>& m_results;
     mutable size_t m_framesToSkip;
     mutable size_t m_remainingCapacityForFrameCapture;
 };
 
-void Interpreter::getStackTrace(Vector<StackFrame>& results, size_t framesToSkip, size_t maxStackSize)
+void Interpreter::getStackTrace(JSCell* owner, Vector<StackFrame>& results, size_t framesToSkip, size_t maxStackSize)
 {
+    DisallowGC disallowGC;
     VM& vm = m_vm;
     CallFrame* callFrame = vm.topCallFrame;
     if (!callFrame)
@@ -537,7 +540,7 @@
     framesCount -= framesToSkip;
     framesCount = std::min(maxStackSize, framesCount);
 
-    GetStackTraceFunctor functor(vm, results, framesToSkip, framesCount);
+    GetStackTraceFunctor functor(vm, owner, results, framesToSkip, framesCount);
     StackVisitor::visit(callFrame, &vm, functor);
     ASSERT(results.size() == results.capacity());
 }

Modified: trunk/Source/_javascript_Core/interpreter/Interpreter.h (221835 => 221836)


--- trunk/Source/_javascript_Core/interpreter/Interpreter.h	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/interpreter/Interpreter.h	2017-09-10 19:00:03 UTC (rev 221836)
@@ -129,7 +129,7 @@
 
         JS_EXPORT_PRIVATE void dumpCallFrame(CallFrame*);
 
-        void getStackTrace(Vector<StackFrame>& results, size_t framesToSkip = 0, size_t maxStackSize = std::numeric_limits<size_t>::max());
+        void getStackTrace(JSCell* owner, Vector<StackFrame>& results, size_t framesToSkip = 0, size_t maxStackSize = std::numeric_limits<size_t>::max());
 
     private:
         enum ExecutionFlag { Normal, InitializeAndReturn };

Modified: trunk/Source/_javascript_Core/runtime/Error.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/Error.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/Error.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -38,12 +38,10 @@
 #include "NativeErrorConstructor.h"
 #include "SourceCode.h"
 #include "StackFrame.h"
+#include "SuperSampler.h"
 
 namespace JSC {
 
-static const char* linePropertyName = "line";
-static const char* sourceURLPropertyName = "sourceURL";
-
 JSObject* createError(ExecState* exec, const String& message, ErrorInstance::SourceAppender appender)
 {
     ASSERT(!message.isEmpty());
@@ -160,49 +158,70 @@
     mutable unsigned m_index;
 };
 
-bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bool useCurrentFrame, CallFrame*& callFrame, unsigned* bytecodeOffset)
+std::unique_ptr<Vector<StackFrame>> getStackTrace(ExecState* exec, VM& vm, JSObject* obj, bool useCurrentFrame)
 {
     JSGlobalObject* globalObject = obj->globalObject();
     ErrorConstructor* errorConstructor = globalObject->errorConstructor();
     if (!errorConstructor->stackTraceLimit())
-        return false;
+        return nullptr;
 
-    Vector<StackFrame> stackTrace = Vector<StackFrame>();
     size_t framesToSkip = useCurrentFrame ? 0 : 1;
-    vm.interpreter->getStackTrace(stackTrace, framesToSkip, errorConstructor->stackTraceLimit().value());
-    if (!stackTrace.isEmpty()) {
+    std::unique_ptr<Vector<StackFrame>> stackTrace = std::make_unique<Vector<StackFrame>>();
+    vm.interpreter->getStackTrace(obj, *stackTrace, framesToSkip, errorConstructor->stackTraceLimit().value());
+    if (!stackTrace->isEmpty())
+        ASSERT_UNUSED(exec, exec == vm.topCallFrame || exec == exec->lexicalGlobalObject()->globalExec() || exec == exec->vmEntryGlobalObject()->globalExec());
+    return stackTrace;
+}
 
-        ASSERT(exec == vm.topCallFrame || exec == exec->lexicalGlobalObject()->globalExec() || exec == exec->vmEntryGlobalObject()->globalExec());
+void getBytecodeOffset(ExecState* exec, VM& vm, Vector<StackFrame>* stackTrace, CallFrame*& callFrame, unsigned& bytecodeOffset)
+{
+    FindFirstCallerFrameWithCodeblockFunctor functor(exec);
+    StackVisitor::visit(vm.topCallFrame, &vm, functor);
+    callFrame = functor.foundCallFrame();
+    unsigned stackIndex = functor.index();
+    bytecodeOffset = 0;
+    if (stackTrace && stackIndex < stackTrace->size() && stackTrace->at(stackIndex).hasBytecodeOffset())
+        bytecodeOffset = stackTrace->at(stackIndex).bytecodeOffset();
+}
 
-        StackFrame* firstFrameWithLineAndColumnInfo = nullptr;
-        for (unsigned i = 0 ; i < stackTrace.size(); ++i) {
-            firstFrameWithLineAndColumnInfo = &stackTrace.at(i);
-            if (firstFrameWithLineAndColumnInfo->hasLineAndColumnInfo())
-                break;
+bool getLineColumnAndSource(Vector<StackFrame>* stackTrace, unsigned& line, unsigned& column, String& sourceURL)
+{
+    line = 0;
+    column = 0;
+    sourceURL = String();
+    
+    if (!stackTrace)
+        return false;
+    
+    for (unsigned i = 0 ; i < stackTrace->size(); ++i) {
+        StackFrame& frame = stackTrace->at(i);
+        if (frame.hasLineAndColumnInfo()) {
+            frame.computeLineAndColumn(line, column);
+            sourceURL = frame.sourceURL();
+            return true;
         }
+    }
+    
+    return false;
+}
 
-        if (bytecodeOffset) {
-            FindFirstCallerFrameWithCodeblockFunctor functor(exec);
-            StackVisitor::visit(vm.topCallFrame, &vm, functor);
-            callFrame = functor.foundCallFrame();
-            unsigned stackIndex = functor.index();
-            *bytecodeOffset = 0;
-            if (stackIndex < stackTrace.size() && stackTrace.at(stackIndex).hasBytecodeOffset())
-                *bytecodeOffset = stackTrace.at(stackIndex).bytecodeOffset();
-        }
-        
+bool addErrorInfo(VM& vm, Vector<StackFrame>* stackTrace, JSObject* obj)
+{
+    if (!stackTrace)
+        return false;
+    
+    if (!stackTrace->isEmpty()) {
         unsigned line;
         unsigned column;
-        firstFrameWithLineAndColumnInfo->computeLineAndColumn(line, column);
+        String sourceURL;
+        getLineColumnAndSource(stackTrace, line, column, sourceURL);
         obj->putDirect(vm, vm.propertyNames->line, jsNumber(line));
         obj->putDirect(vm, vm.propertyNames->column, jsNumber(column));
+        if (!sourceURL.isEmpty())
+            obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, sourceURL));
 
-        String frameSourceURL = firstFrameWithLineAndColumnInfo->sourceURL();
-        if (!frameSourceURL.isEmpty())
-            obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, frameSourceURL));
+        obj->putDirect(vm, vm.propertyNames->stack, Interpreter::stackTraceAsString(vm, *stackTrace), DontEnum);
 
-        obj->putDirect(vm, vm.propertyNames->stack, Interpreter::stackTraceAsString(vm, stackTrace), DontEnum);
-
         return true;
     }
 
@@ -212,19 +231,33 @@
 
 void addErrorInfo(ExecState* exec, JSObject* obj, bool useCurrentFrame)
 {
-    CallFrame* callFrame = nullptr;
-    addErrorInfoAndGetBytecodeOffset(exec, exec->vm(), obj, useCurrentFrame, callFrame);
+    VM& vm = exec->vm();
+    std::unique_ptr<Vector<StackFrame>> stackTrace = getStackTrace(exec, vm, obj, useCurrentFrame);
+    addErrorInfo(vm, stackTrace.get(), obj);
 }
 
 JSObject* addErrorInfo(CallFrame* callFrame, JSObject* error, int line, const SourceCode& source)
 {
-    VM* vm = &callFrame->vm();
+    VM& vm = callFrame->vm();
     const String& sourceURL = source.provider()->url();
-
+    
+    // The putDirect() calls below should really be put() so that they trigger materialization of
+    // the line/sourceURL properties. Otherwise, what we set here will just be overwritten later.
+    // But calling put() would be bad because we'd rather not do effectful things here. Luckily, we
+    // know that this will get called on some kind of error - so we can just directly ask the
+    // ErrorInstance to materialize whatever it needs to. There's a chance that we get passed some
+    // other kind of object, which also has materializable properties. But this code is heuristic-ey
+    // enough that if we're wrong in such corner cases, it's not the end of the world.
+    if (ErrorInstance* errorInstance = jsDynamicCast<ErrorInstance*>(vm, error))
+        errorInstance->materializeErrorInfoIfNeeded(vm);
+    
+    // FIXME: This does not modify the column property, which confusingly continues to reflect
+    // the column at which the exception was thrown.
+    // https://bugs.webkit.org/show_bug.cgi?id=176673
     if (line != -1)
-        error->putDirect(*vm, Identifier::fromString(vm, linePropertyName), jsNumber(line));
+        error->putDirect(vm, vm.propertyNames->line, jsNumber(line));
     if (!sourceURL.isNull())
-        error->putDirect(*vm, Identifier::fromString(vm, sourceURLPropertyName), jsString(vm, sourceURL));
+        error->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, sourceURL));
     return error;
 }
 

Modified: trunk/Source/_javascript_Core/runtime/Error.h (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/Error.h	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/Error.h	2017-09-10 19:00:03 UTC (rev 221836)
@@ -74,10 +74,11 @@
 
 JS_EXPORT_PRIVATE JSObject* createError(ExecState*, ErrorType, const String&);
 
-
-bool addErrorInfoAndGetBytecodeOffset(ExecState*, VM&, JSObject*, bool, CallFrame*&, unsigned* = nullptr);
-
-JS_EXPORT_PRIVATE void addErrorInfo(ExecState*, JSObject*, bool); 
+std::unique_ptr<Vector<StackFrame>> getStackTrace(ExecState*, VM&, JSObject*, bool useCurrentFrame);
+void getBytecodeOffset(ExecState*, VM&, Vector<StackFrame>*, CallFrame*&, unsigned& bytecodeOffset);
+bool getLineColumnAndSource(Vector<StackFrame>* stackTrace, unsigned& line, unsigned& column, String& sourceURL);
+bool addErrorInfo(VM&, Vector<StackFrame>*, JSObject*);
+JS_EXPORT_PRIVATE void addErrorInfo(ExecState*, JSObject*, bool);
 JSObject* addErrorInfo(ExecState*, JSObject* error, int line, const SourceCode&);
 
 // Methods to throw Errors.

Modified: trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -26,16 +26,15 @@
 #include "JSScope.h"
 #include "JSCInlines.h"
 #include "ParseInt.h"
+#include "StackFrame.h"
 #include <wtf/text/StringBuilder.h>
 
 namespace JSC {
 
-STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(ErrorInstance);
-
 const ClassInfo ErrorInstance::s_info = { "Error", &JSNonFinalObject::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(ErrorInstance) };
 
 ErrorInstance::ErrorInstance(VM& vm, Structure* structure)
-    : JSNonFinalObject(vm, structure)
+    : Base(vm, structure)
 {
 }
 
@@ -151,11 +150,11 @@
     if (!message.isNull())
         putDirect(vm, vm.propertyNames->message, jsString(&vm, message), DontEnum);
 
-    unsigned bytecodeOffset = 0;
-    CallFrame* callFrame = nullptr;
-    bool hasTrace = addErrorInfoAndGetBytecodeOffset(exec, vm, this, useCurrentFrame, callFrame, hasSourceAppender() ? &bytecodeOffset : nullptr);
-
-    if (hasTrace && callFrame && hasSourceAppender()) {
+    m_stackTrace = getStackTrace(exec, vm, this, useCurrentFrame);
+    if (m_stackTrace && !m_stackTrace->isEmpty() && hasSourceAppender()) {
+        unsigned bytecodeOffset;
+        CallFrame* callFrame;
+        getBytecodeOffset(exec, vm, m_stackTrace.get(), callFrame, bytecodeOffset);
         if (callFrame && callFrame->codeBlock()) {
             ASSERT(!callFrame->callee().isWasm());
             appendSourceToError(callFrame, this, bytecodeOffset);
@@ -227,4 +226,84 @@
     return builder.toString();
 }
 
+void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
+{
+    if (m_errorInfoMaterialized)
+        return;
+    
+    addErrorInfo(vm, m_stackTrace.get(), this);
+    m_stackTrace = nullptr;
+    
+    m_errorInfoMaterialized = true;
+}
+
+void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm, PropertyName propertyName)
+{
+    if (propertyName == vm.propertyNames->line
+        || propertyName == vm.propertyNames->column
+        || propertyName == vm.propertyNames->sourceURL
+        || propertyName == vm.propertyNames->stack)
+        materializeErrorInfoIfNeeded(vm);
+}
+
+void ErrorInstance::visitChildren(JSCell* cell, SlotVisitor& visitor)
+{
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(cell);
+    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
+    Base::visitChildren(thisObject, visitor);
+
+    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();
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(object);
+    thisObject->materializeErrorInfoIfNeeded(vm, propertyName);
+    return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
+}
+
+void ErrorInstance::getOwnNonIndexPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNameArray, EnumerationMode enumerationMode)
+{
+    VM& vm = exec->vm();
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(object);
+    thisObject->materializeErrorInfoIfNeeded(vm);
+    Base::getOwnNonIndexPropertyNames(thisObject, exec, propertyNameArray, enumerationMode);
+}
+
+void ErrorInstance::getStructurePropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNameArray, EnumerationMode enumerationMode)
+{
+    VM& vm = exec->vm();
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(object);
+    thisObject->materializeErrorInfoIfNeeded(vm);
+    Base::getStructurePropertyNames(thisObject, exec, propertyNameArray, enumerationMode);
+}
+
+bool ErrorInstance::defineOwnProperty(JSObject* object, ExecState* exec, PropertyName propertyName, const PropertyDescriptor& descriptor, bool shouldThrow)
+{
+    VM& vm = exec->vm();
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(object);
+    thisObject->materializeErrorInfoIfNeeded(vm, propertyName);
+    return Base::defineOwnProperty(thisObject, exec, propertyName, descriptor, shouldThrow);
+}
+
+bool ErrorInstance::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
+{
+    VM& vm = exec->vm();
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(cell);
+    thisObject->materializeErrorInfoIfNeeded(vm, propertyName);
+    return Base::put(thisObject, exec, propertyName, value, slot);
+}
+
+bool ErrorInstance::deleteProperty(JSCell* cell, ExecState* exec, PropertyName propertyName)
+{
+    VM& vm = exec->vm();
+    ErrorInstance* thisObject = jsCast<ErrorInstance*>(cell);
+    thisObject->materializeErrorInfoIfNeeded(vm, propertyName);
+    return Base::deleteProperty(thisObject, exec, propertyName);
+}
+
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/runtime/ErrorInstance.h (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/ErrorInstance.h	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/ErrorInstance.h	2017-09-10 19:00:03 UTC (rev 221836)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (por...@kde.org)
- *  Copyright (C) 2008, 2016 Apple Inc. All rights reserved.
+ *  Copyright (C) 2008-2017 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -20,14 +20,16 @@
 
 #pragma once
 
-#include "JSObject.h"
+#include "JSDestructibleObject.h"
 #include "RuntimeType.h"
+#include "StackFrame.h"
 
 namespace JSC {
 
-class ErrorInstance : public JSNonFinalObject {
+class ErrorInstance : public JSDestructibleObject {
 public:
-    typedef JSNonFinalObject Base;
+    typedef JSDestructibleObject Base;
+    const static unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetPropertyNames;
 
     enum SourceTextWhereErrorOccurred { FoundExactSource, FoundApproximateSource };
     typedef String (*SourceAppender) (const String& originalMessage, const String& sourceText, RuntimeType, SourceTextWhereErrorOccurred);
@@ -50,8 +52,6 @@
 
     static ErrorInstance* create(ExecState*, Structure*, JSValue message, SourceAppender = nullptr, RuntimeType = TypeNothing, bool useCurrentFrame = true);
 
-    static void addErrorInfo(ExecState*, VM&, JSObject*, bool = true);
-
     bool hasSourceAppender() const { return !!m_sourceAppender; }
     SourceAppender sourceAppender() const { return m_sourceAppender; }
     void setSourceAppender(SourceAppender appender) { m_sourceAppender = appender; }
@@ -66,16 +66,32 @@
     bool isOutOfMemoryError() const { return m_outOfMemoryError; }
 
     JS_EXPORT_PRIVATE String sanitizedToString(ExecState*);
+    
+    Vector<StackFrame>* stackTrace() { return m_stackTrace.get(); }
 
+    void materializeErrorInfoIfNeeded(VM&);
+    void materializeErrorInfoIfNeeded(VM&, PropertyName);
+
 protected:
     explicit ErrorInstance(VM&, Structure*);
 
     void finishCreation(ExecState*, VM&, const String&, bool useCurrentFrame = true);
 
+    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);
+    
     SourceAppender m_sourceAppender { nullptr };
     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

Modified: trunk/Source/_javascript_Core/runtime/Exception.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/Exception.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/Exception.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -58,6 +58,8 @@
     Base::visitChildren(thisObject, visitor);
 
     visitor.append(thisObject->m_value);
+    for (StackFrame& frame : thisObject->m_stack)
+        frame.visitChildren(visitor);
 }
 
 Exception::Exception(VM& vm)
@@ -77,7 +79,7 @@
 
     Vector<StackFrame> stackTrace;
     if (action == StackCaptureAction::CaptureStack)
-        vm.interpreter->getStackTrace(stackTrace, 0, Options::exceptionStackTraceLimit());
+        vm.interpreter->getStackTrace(this, stackTrace, 0, Options::exceptionStackTraceLimit());
     m_stack = WTFMove(stackTrace);
 }
 

Modified: trunk/Source/_javascript_Core/runtime/Exception.h (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/Exception.h	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/Exception.h	2017-09-10 19:00:03 UTC (rev 221836)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -25,15 +25,15 @@
 
 #pragma once
 
-#include "JSObject.h"
+#include "JSDestructibleObject.h"
 #include "StackFrame.h"
 #include <wtf/Vector.h>
 
 namespace JSC {
     
-class Exception : public JSNonFinalObject {
+class Exception : public JSDestructibleObject {
 public:
-    typedef JSNonFinalObject Base;
+    typedef JSDestructibleObject Base;
     static const unsigned StructureFlags = StructureIsImmortal | Base::StructureFlags;
 
     enum StackCaptureAction {

Modified: trunk/Source/_javascript_Core/runtime/StackFrame.cpp (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/StackFrame.cpp	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/StackFrame.cpp	2017-09-10 19:00:03 UTC (rev 221836)
@@ -33,6 +33,19 @@
 
 namespace JSC {
 
+StackFrame::StackFrame(VM& vm, JSCell* owner, JSCell* callee)
+    : m_callee(vm, owner, callee)
+    , m_bytecodeOffset(UINT_MAX)
+{
+}
+
+StackFrame::StackFrame(VM& vm, JSCell* owner, JSCell* callee, CodeBlock* codeBlock, unsigned bytecodeOffset)
+    : m_callee(vm, owner, callee)
+    , m_codeBlock(vm, owner, codeBlock)
+    , m_bytecodeOffset(bytecodeOffset)
+{
+}
+
 intptr_t StackFrame::sourceID() const
 {
     if (!m_codeBlock)
@@ -127,5 +140,16 @@
     return traceBuild.toString().impl();
 }
 
+void StackFrame::visitChildren(SlotVisitor& visitor)
+{
+    // FIXME: We should do something here about Wasm::IndexOrName.
+    // https://bugs.webkit.org/show_bug.cgi?id=176644
+    
+    if (m_callee)
+        visitor.append(m_callee);
+    if (m_codeBlock)
+        visitor.append(m_codeBlock);
+}
+
 } // namespace JSC
 

Modified: trunk/Source/_javascript_Core/runtime/StackFrame.h (221835 => 221836)


--- trunk/Source/_javascript_Core/runtime/StackFrame.h	2017-09-10 18:55:24 UTC (rev 221835)
+++ trunk/Source/_javascript_Core/runtime/StackFrame.h	2017-09-10 19:00:03 UTC (rev 221836)
@@ -25,8 +25,8 @@
 
 #pragma once
 
-#include "Strong.h"
 #include "WasmIndexOrName.h"
+#include "WriteBarrier.h"
 #include <limits.h>
 
 namespace JSC {
@@ -33,6 +33,7 @@
 
 class CodeBlock;
 class JSObject;
+class SlotVisitor;
 
 class StackFrame {
 public:
@@ -40,16 +41,9 @@
         : m_bytecodeOffset(UINT_MAX)
     { }
 
-    StackFrame(VM& vm, JSCell* callee)
-        : m_callee(vm, callee)
-        , m_bytecodeOffset(UINT_MAX)
-    { }
+    StackFrame(VM&, JSCell* owner, JSCell* callee);
 
-    StackFrame(VM& vm, JSCell* callee, CodeBlock* codeBlock, unsigned bytecodeOffset)
-        : m_callee(vm, callee)
-        , m_codeBlock(vm, codeBlock)
-        , m_bytecodeOffset(bytecodeOffset)
-    { }
+    StackFrame(VM&, JSCell* owner, JSCell* callee, CodeBlock*, unsigned bytecodeOffset);
 
     static StackFrame wasm(Wasm::IndexOrName indexOrName)
     {
@@ -73,11 +67,12 @@
         ASSERT(hasBytecodeOffset());
         return m_bytecodeOffset;
     }
+    
+    void visitChildren(SlotVisitor&);
 
-
 private:
-    Strong<JSCell> m_callee { };
-    Strong<CodeBlock> m_codeBlock { };
+    WriteBarrier<JSCell> m_callee { };
+    WriteBarrier<CodeBlock> m_codeBlock { };
     union {
         unsigned m_bytecodeOffset;
         Wasm::IndexOrName m_wasmFunctionIndexOrName;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to