Changeset 264160 in webkit

Timestamp:

Jul 9, 2020, 1:23:36 AM (6 years ago)

Author:

Alexey Shvayka

Message:

ErrorInstance::finishCreation() puts "message" twice, with different attributes
​https://bugs.webkit.org/show_bug.cgi?id=214089

Reviewed by Yusuke Suzuki.

JSTests:

• microbenchmarks/error-source-appender.js: Added.

Source/JavaScriptCore:

This change refactors appendSourceToError() to return new message, making it almost pure
and eliminating extra JSString -> String -> JSString conversion and {get,put}Direct() calls
from ErrorInstance::finishCreation(). Also, moves null message check as early as possible.

Removed putDirect() call didn't pass PropertyAttribute::DontEnum. An implementation detail,
that is about to change in ​https://webkit.org/b/142933, prevented "message" property from
beind redefined with PropertyAttribute::None.

No behavior change. Advances provided microbenchmark by 5%.

• runtime/ErrorInstance.cpp:

(JSC::appendSourceToErrorMessage):
(JSC::ErrorInstance::finishCreation):
(JSC::appendSourceToError): Deleted.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/error-source-appender.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ErrorInstance.cpp (modified) (6 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-07-09  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        ErrorInstance::finishCreation() puts "message" twice, with different attributes
+        https://bugs.webkit.org/show_bug.cgi?id=214089
+
+        Reviewed by Yusuke Suzuki.
+
+        * microbenchmarks/error-source-appender.js: Added.
+
 2020-07-07  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-07-09  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        ErrorInstance::finishCreation() puts "message" twice, with different attributes
+        https://bugs.webkit.org/show_bug.cgi?id=214089
+
+        Reviewed by Yusuke Suzuki.
+
+        This change refactors appendSourceToError() to return new message, making it almost pure
+        and eliminating extra JSString -> String -> JSString conversion and {get,put}Direct() calls
+        from ErrorInstance::finishCreation(). Also, moves null message check as early as possible.
+
+        Removed putDirect() call didn't pass PropertyAttribute::DontEnum. An implementation detail,
+        that is about to change in https://webkit.org/b/142933, prevented "message" property from
+        beind redefined with PropertyAttribute::None.
+
+        No behavior change. Advances provided microbenchmark by 5%.
+
+        * runtime/ErrorInstance.cpp:
+        (JSC::appendSourceToErrorMessage):
+        (JSC::ErrorInstance::finishCreation):
+        (JSC::appendSourceToError): Deleted.
+
 2020-07-08  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ErrorInstance.cpp

@@ -49 +49 @@
 }
 
-static void appendSourceToError(JSGlobalObject* globalObject, CallFrame* callFrame, ErrorInstance* exception, BytecodeIndex bytecodeIndex)
+static String appendSourceToErrorMessage(CallFrame* callFrame, ErrorInstance* exception, BytecodeIndex bytecodeIndex, const String& message)
 {
     ErrorInstance::SourceAppender appender = exception->sourceAppender();
@@ -56 +56 @@
     exception->clearRuntimeTypeForCause();
 
-    if (!callFrame->codeBlock()->hasExpressionInfo())
-        return;
+    if (!callFrame->codeBlock()->hasExpressionInfo() || message.isNull())
+        return message;
     
     int startOffset = 0;
@@ -79 +79 @@
     StringView sourceString = codeBlock->source().provider()->source();
     if (!expressionStop || expressionStart > static_cast<int>(sourceString.length()))
-        return;
+        return message;
     
-    VM& vm = globalObject->vm();
-    JSValue jsMessage = exception->getDirect(vm, vm.propertyNames->message);
-    if (!jsMessage || !jsMessage.isString())
-        return;
-    
-    String message = asString(jsMessage)->value(globalObject);
     if (expressionStart < expressionStop)
-        message = appender(message, codeBlock->source().provider()->getRange(expressionStart, expressionStop).toString(), type, ErrorInstance::FoundExactSource);
-    else {
-        // No range information, so give a few characters of context.
-        int dataLength = sourceString.length();
-        int start = expressionStart;
-        int stop = expressionStart;
-        // Get up to 20 characters of context to the left and right of the divot, clamping to the line.
-        // Then strip whitespace.
-        while (start > 0 && (expressionStart - start < 20) && sourceString[start - 1] != '\n')
-            start--;
-        while (start < (expressionStart - 1) && isStrWhiteSpace(sourceString[start]))
-            start++;
-        while (stop < dataLength && (stop - expressionStart < 20) && sourceString[stop] != '\n')
-            stop++;
-        while (stop > expressionStart && isStrWhiteSpace(sourceString[stop - 1]))
-            stop--;
-        message = appender(message, codeBlock->source().provider()->getRange(start, stop).toString(), type, ErrorInstance::FoundApproximateSource);
-    }
-    exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message));
-
+        return appender(message, codeBlock->source().provider()->getRange(expressionStart, expressionStop).toString(), type, ErrorInstance::FoundExactSource);
+
+    // No range information, so give a few characters of context.
+    int dataLength = sourceString.length();
+    int start = expressionStart;
+    int stop = expressionStart;
+    // Get up to 20 characters of context to the left and right of the divot, clamping to the line.
+    // Then strip whitespace.
+    while (start > 0 && (expressionStart - start < 20) && sourceString[start - 1] != '\n')
+        start--;
+    while (start < (expressionStart - 1) && isStrWhiteSpace(sourceString[start]))
+        start++;
+    while (stop < dataLength && (stop - expressionStart < 20) && sourceString[stop] != '\n')
+        stop++;
+    while (stop > expressionStart && isStrWhiteSpace(sourceString[stop - 1]))
+        stop--;
+    return appender(message, codeBlock->source().provider()->getRange(start, stop).toString(), type, ErrorInstance::FoundApproximateSource);
 }
 
@@ -117 +108 @@
     m_sourceAppender = appender;
     m_runtimeTypeForCause = type;
-
-    if (!message.isNull())
-        putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum));
 
     std::unique_ptr<Vector<StackFrame>> stackTrace = getStackTrace(globalObject, vm, this, useCurrentFrame);
@@ -128 +116 @@
     vm.heap.writeBarrier(this);
 
+    String messageWithSource = message;
     if (m_stackTrace && !m_stackTrace->isEmpty() && hasSourceAppender()) {
         BytecodeIndex bytecodeIndex;
@@ -133 +122 @@
         getBytecodeIndex(vm, vm.topCallFrame, m_stackTrace.get(), callFrame, bytecodeIndex);
         if (callFrame && callFrame->codeBlock() && !callFrame->callee().isWasm())
-            appendSourceToError(globalObject, callFrame, this, bytecodeIndex);
-    }
+            messageWithSource = appendSourceToErrorMessage(callFrame, this, bytecodeIndex, message);
+    }
+
+    if (!messageWithSource.isNull())
+        putDirect(vm, vm.propertyNames->message, jsString(vm, messageWithSource), static_cast<unsigned>(PropertyAttribute::DontEnum));
 }