Skip to content

Commit 0efb14c

Browse files
rkirslingrkirsling
committed
[JSC] Fix invalid exception checks after recent ErrorInstance changes
https://bugs.webkit.org/show_bug.cgi?id=225565 Reviewed by Alexey Shvayka. r277221 and r277224 each introduced issues under validateExceptionChecks=1; this patch fixes them. Of particular note: The earlier patch sought to consolidate Error#cause logic under ErrorInstance::finishCreation. This part must be undone as it is crucial that non-user-thrown errors be able to bypass that logic (otherwise throwException itself would need to be exception-checked). * runtime/AggregateError.cpp: (JSC::createAggregateError): * runtime/ArrayPrototype.cpp: (JSC::getProperty): * runtime/Error.cpp: (JSC::createError): (JSC::createEvalError): (JSC::createRangeError): (JSC::createReferenceError): (JSC::createSyntaxError): (JSC::createTypeError): (JSC::createURIError): (JSC::createGetterTypeError): * runtime/ErrorInstance.cpp: (JSC::ErrorInstance::create): (JSC::ErrorInstance::finishCreation): * runtime/ErrorInstance.h: (JSC::ErrorInstance::create): * runtime/JSObject.h: * runtime/JSObjectInlines.h: (JSC::JSObject::getIfPropertyExists): * runtime/NullSetterFunction.cpp: (JSC::NullSetterFunctionInternal::JSC_DEFINE_HOST_FUNCTION): * wasm/js/JSWebAssemblyCompileError.cpp: (JSC::createJSWebAssemblyCompileError): * wasm/js/JSWebAssemblyLinkError.cpp: (JSC::createJSWebAssemblyLinkError): * wasm/js/JSWebAssemblyRuntimeError.cpp: (JSC::createJSWebAssemblyRuntimeError): Canonical link: https://commits.webkit.org/237507@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@277238 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 44a6823 commit 0efb14c

12 files changed

Lines changed: 81 additions & 39 deletions

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,47 @@
1+
2021-05-08 Ross Kirsling <ross.kirsling@sony.com>
2+
3+
[JSC] Fix invalid exception checks after recent ErrorInstance changes
4+
https://bugs.webkit.org/show_bug.cgi?id=225565
5+
6+
Reviewed by Alexey Shvayka.
7+
8+
r277221 and r277224 each introduced issues under validateExceptionChecks=1; this patch fixes them.
9+
10+
Of particular note:
11+
The earlier patch sought to consolidate Error#cause logic under ErrorInstance::finishCreation.
12+
This part must be undone as it is crucial that non-user-thrown errors be able to bypass that logic
13+
(otherwise throwException itself would need to be exception-checked).
14+
15+
* runtime/AggregateError.cpp:
16+
(JSC::createAggregateError):
17+
* runtime/ArrayPrototype.cpp:
18+
(JSC::getProperty):
19+
* runtime/Error.cpp:
20+
(JSC::createError):
21+
(JSC::createEvalError):
22+
(JSC::createRangeError):
23+
(JSC::createReferenceError):
24+
(JSC::createSyntaxError):
25+
(JSC::createTypeError):
26+
(JSC::createURIError):
27+
(JSC::createGetterTypeError):
28+
* runtime/ErrorInstance.cpp:
29+
(JSC::ErrorInstance::create):
30+
(JSC::ErrorInstance::finishCreation):
31+
* runtime/ErrorInstance.h:
32+
(JSC::ErrorInstance::create):
33+
* runtime/JSObject.h:
34+
* runtime/JSObjectInlines.h:
35+
(JSC::JSObject::getIfPropertyExists):
36+
* runtime/NullSetterFunction.cpp:
37+
(JSC::NullSetterFunctionInternal::JSC_DEFINE_HOST_FUNCTION):
38+
* wasm/js/JSWebAssemblyCompileError.cpp:
39+
(JSC::createJSWebAssemblyCompileError):
40+
* wasm/js/JSWebAssemblyLinkError.cpp:
41+
(JSC::createJSWebAssemblyLinkError):
42+
* wasm/js/JSWebAssemblyRuntimeError.cpp:
43+
(JSC::createJSWebAssemblyRuntimeError):
44+
145
2021-05-08 Chris Dumez <cdumez@apple.com>
246

347
Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem

‎Source/JavaScriptCore/runtime/AggregateError.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ ErrorInstance* createAggregateError(JSGlobalObject* globalObject, VM& vm, Struct
4949
RETURN_IF_EXCEPTION(scope, nullptr);
5050

5151
auto* error = ErrorInstance::create(globalObject, structure, message, options, appender, type, ErrorType::AggregateError, useCurrentFrame);
52+
RETURN_IF_EXCEPTION(scope, nullptr);
5253
error->putDirect(vm, vm.propertyNames->errors, array, static_cast<unsigned>(PropertyAttribute::DontEnum));
5354
return error;
5455
}

‎Source/JavaScriptCore/runtime/ArrayPrototype.cpp‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,16 +145,11 @@ void ArrayPrototype::finishCreation(VM& vm, JSGlobalObject* globalObject)
145145

146146
static ALWAYS_INLINE JSValue getProperty(JSGlobalObject* globalObject, JSObject* object, uint64_t index)
147147
{
148-
VM& vm = globalObject->vm();
149-
auto scope = DECLARE_THROW_SCOPE(vm);
150-
151148
if (JSValue result = object->tryGetIndexQuickly(index))
152149
return result;
153150

154151
// Don't return undefined if the property is not found.
155-
auto property = object->getIfPropertyExists(globalObject, Identifier::from(vm, index));
156-
RETURN_IF_EXCEPTION(scope, { });
157-
return property.valueOr(JSValue());
152+
return object->getIfPropertyExists(globalObject, Identifier::from(globalObject->vm(), index));
158153
}
159154

160155
static ALWAYS_INLINE void setLength(JSGlobalObject* globalObject, VM& vm, JSObject* obj, uint64_t value)

‎Source/JavaScriptCore/runtime/Error.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,37 +35,37 @@ namespace JSC {
3535
JSObject* createError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender)
3636
{
3737
ASSERT(!message.isEmpty());
38-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(), message, jsUndefined(), appender, TypeNothing, ErrorType::Error, true);
38+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(), message, JSValue(), appender, TypeNothing, ErrorType::Error, true);
3939
}
4040

4141
JSObject* createEvalError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender)
4242
{
4343
ASSERT(!message.isEmpty());
44-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::EvalError), message, jsUndefined(), appender, TypeNothing, ErrorType::EvalError, true);
44+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::EvalError), message, JSValue(), appender, TypeNothing, ErrorType::EvalError, true);
4545
}
4646

4747
JSObject* createRangeError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender)
4848
{
4949
ASSERT(!message.isEmpty());
50-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::RangeError), message, jsUndefined(), appender, TypeNothing, ErrorType::RangeError, true);
50+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::RangeError), message, JSValue(), appender, TypeNothing, ErrorType::RangeError, true);
5151
}
5252

5353
JSObject* createReferenceError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender)
5454
{
5555
ASSERT(!message.isEmpty());
56-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::ReferenceError), message, jsUndefined(), appender, TypeNothing, ErrorType::ReferenceError, true);
56+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::ReferenceError), message, JSValue(), appender, TypeNothing, ErrorType::ReferenceError, true);
5757
}
5858

5959
JSObject* createSyntaxError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender)
6060
{
6161
ASSERT(!message.isEmpty());
62-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::SyntaxError), message, jsUndefined(), appender, TypeNothing, ErrorType::SyntaxError, true);
62+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::SyntaxError), message, JSValue(), appender, TypeNothing, ErrorType::SyntaxError, true);
6363
}
6464

6565
JSObject* createTypeError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender, RuntimeType type)
6666
{
6767
ASSERT(!message.isEmpty());
68-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::TypeError), message, jsUndefined(), appender, type, ErrorType::TypeError, true);
68+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::TypeError), message, JSValue(), appender, type, ErrorType::TypeError, true);
6969
}
7070

7171
JSObject* createNotEnoughArgumentsError(JSGlobalObject* globalObject, ErrorInstance::SourceAppender appender)
@@ -76,7 +76,7 @@ JSObject* createNotEnoughArgumentsError(JSGlobalObject* globalObject, ErrorInsta
7676
JSObject* createURIError(JSGlobalObject* globalObject, const String& message, ErrorInstance::SourceAppender appender)
7777
{
7878
ASSERT(!message.isEmpty());
79-
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::URIError), message, jsUndefined(), appender, TypeNothing, ErrorType::URIError, true);
79+
return ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::URIError), message, JSValue(), appender, TypeNothing, ErrorType::URIError, true);
8080
}
8181

8282
JSObject* createError(JSGlobalObject* globalObject, ErrorType errorType, const String& message)
@@ -113,7 +113,7 @@ JSObject* createError(JSGlobalObject* globalObject, ErrorTypeWithExtension error
113113
static JSObject* createGetterTypeError(JSGlobalObject* globalObject, const String& message)
114114
{
115115
ASSERT(!message.isEmpty());
116-
auto* error = ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::TypeError), message, jsUndefined(), nullptr, TypeNothing, ErrorType::TypeError);
116+
auto* error = ErrorInstance::create(globalObject, globalObject->vm(), globalObject->errorStructure(ErrorType::TypeError), message, JSValue(), nullptr, TypeNothing, ErrorType::TypeError);
117117
error->setNativeGetterTypeError();
118118
return error;
119119
}

‎Source/JavaScriptCore/runtime/ErrorInstance.cpp‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,14 @@ ErrorInstance* ErrorInstance::create(JSGlobalObject* globalObject, Structure* st
5252
String messageString = message.isUndefined() ? String() : message.toWTFString(globalObject);
5353
RETURN_IF_EXCEPTION(scope, nullptr);
5454

55-
return create(globalObject, vm, structure, messageString, options, appender, type, errorType, useCurrentFrame);
55+
JSValue cause;
56+
if (options.isObject()) {
57+
// Since `throw undefined;` is valid, we need to distinguish the case where `cause` is an explicit undefined.
58+
cause = asObject(options)->getIfPropertyExists(globalObject, vm.propertyNames->cause);
59+
RETURN_IF_EXCEPTION(scope, nullptr);
60+
}
61+
62+
return create(globalObject, vm, structure, messageString, cause, appender, type, errorType, useCurrentFrame);
5663
}
5764

5865
static String appendSourceToErrorMessage(CallFrame* callFrame, ErrorInstance* exception, BytecodeIndex bytecodeIndex, const String& message)
@@ -107,11 +114,10 @@ static String appendSourceToErrorMessage(CallFrame* callFrame, ErrorInstance* ex
107114
return appender(message, codeBlock->source().provider()->getRange(start, stop).toString(), type, ErrorInstance::FoundApproximateSource);
108115
}
109116

110-
void ErrorInstance::finishCreation(VM& vm, JSGlobalObject* globalObject, const String& message, JSValue options, SourceAppender appender, RuntimeType type, bool useCurrentFrame)
117+
void ErrorInstance::finishCreation(VM& vm, JSGlobalObject* globalObject, const String& message, JSValue cause, SourceAppender appender, RuntimeType type, bool useCurrentFrame)
111118
{
112119
Base::finishCreation(vm);
113120
ASSERT(inherits(vm, info()));
114-
auto scope = DECLARE_THROW_SCOPE(vm);
115121

116122
m_sourceAppender = appender;
117123
m_runtimeTypeForCause = type;
@@ -135,13 +141,8 @@ void ErrorInstance::finishCreation(VM& vm, JSGlobalObject* globalObject, const S
135141
if (!messageWithSource.isNull())
136142
putDirect(vm, vm.propertyNames->message, jsString(vm, messageWithSource), static_cast<unsigned>(PropertyAttribute::DontEnum));
137143

138-
if (options.isObject()) {
139-
// Since `throw undefined;` is valid, we need to distinguish the case where `cause` is an explicit undefined.
140-
auto cause = asObject(options)->getIfPropertyExists(globalObject, vm.propertyNames->cause);
141-
RETURN_IF_EXCEPTION(scope, void());
142-
if (cause)
143-
putDirect(vm, vm.propertyNames->cause, cause.value(), static_cast<unsigned>(PropertyAttribute::DontEnum));
144-
}
144+
if (!cause.isEmpty())
145+
putDirect(vm, vm.propertyNames->cause, cause, static_cast<unsigned>(PropertyAttribute::DontEnum));
145146
}
146147

147148
// Based on ErrorPrototype's errorProtoFuncToString(), but is modified to

‎Source/JavaScriptCore/runtime/ErrorInstance.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,10 @@ class ErrorInstance : public JSNonFinalObject {
5454
return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info());
5555
}
5656

57-
static ErrorInstance* create(JSGlobalObject* globalObject, VM& vm, Structure* structure, const String& message, JSValue options, SourceAppender appender = nullptr, RuntimeType type = TypeNothing, ErrorType errorType = ErrorType::Error, bool useCurrentFrame = true)
57+
static ErrorInstance* create(JSGlobalObject* globalObject, VM& vm, Structure* structure, const String& message, JSValue cause, SourceAppender appender = nullptr, RuntimeType type = TypeNothing, ErrorType errorType = ErrorType::Error, bool useCurrentFrame = true)
5858
{
5959
ErrorInstance* instance = new (NotNull, allocateCell<ErrorInstance>(vm.heap)) ErrorInstance(vm, structure, errorType);
60-
instance->finishCreation(vm, globalObject, message, options, appender, type, useCurrentFrame);
60+
instance->finishCreation(vm, globalObject, message, cause, appender, type, useCurrentFrame);
6161
return instance;
6262
}
6363

@@ -94,7 +94,7 @@ class ErrorInstance : public JSNonFinalObject {
9494
protected:
9595
explicit ErrorInstance(VM&, Structure*, ErrorType);
9696

97-
void finishCreation(VM&, JSGlobalObject*, const String& message, JSValue options, SourceAppender = nullptr, RuntimeType = TypeNothing, bool useCurrentFrame = true);
97+
void finishCreation(VM&, JSGlobalObject*, const String& message, JSValue cause, SourceAppender = nullptr, RuntimeType = TypeNothing, bool useCurrentFrame = true);
9898

9999
static bool getOwnPropertySlot(JSObject*, JSGlobalObject*, PropertyName, PropertySlot&);
100100
static void getOwnSpecialPropertyNames(JSObject*, JSGlobalObject*, PropertyNameArray&, DontEnumPropertiesMode);

‎Source/JavaScriptCore/runtime/JSObject.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ class JSObject : public JSCell {
162162
template<typename CallbackWhenNoException> typename std::result_of<CallbackWhenNoException(bool, PropertySlot&)>::type getPropertySlot(JSGlobalObject*, PropertyName, CallbackWhenNoException) const;
163163
template<typename CallbackWhenNoException> typename std::result_of<CallbackWhenNoException(bool, PropertySlot&)>::type getPropertySlot(JSGlobalObject*, PropertyName, PropertySlot&, CallbackWhenNoException) const;
164164

165-
Optional<JSValue> getIfPropertyExists(JSGlobalObject*, PropertyName);
165+
JSValue getIfPropertyExists(JSGlobalObject*, PropertyName);
166166

167167
private:
168168
static bool getOwnPropertySlotImpl(JSObject*, JSGlobalObject*, PropertyName, PropertySlot&);

‎Source/JavaScriptCore/runtime/JSObjectInlines.h‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -191,21 +191,22 @@ inline bool JSObject::getOwnPropertySlotInline(JSGlobalObject* globalObject, Pro
191191
return JSObject::getOwnPropertySlot(this, globalObject, propertyName, slot);
192192
}
193193

194-
inline Optional<JSValue> JSObject::getIfPropertyExists(JSGlobalObject* globalObject, PropertyName propertyName)
194+
inline JSValue JSObject::getIfPropertyExists(JSGlobalObject* globalObject, PropertyName propertyName)
195195
{
196196
VM& vm = globalObject->vm();
197197
auto scope = DECLARE_THROW_SCOPE(vm);
198198

199199
PropertySlot slot(this, PropertySlot::InternalMethodType::HasProperty);
200200
bool hasProperty = getPropertySlot(globalObject, propertyName, slot);
201-
EXCEPTION_ASSERT_UNUSED(scope, !scope.exception() || !hasProperty);
202-
201+
RETURN_IF_EXCEPTION(scope, { });
203202
if (!hasProperty)
204-
return WTF::nullopt;
203+
return { };
204+
205+
scope.release();
206+
if (UNLIKELY(slot.isTaintedByOpaqueObject()))
207+
return get(globalObject, propertyName);
205208

206-
return UNLIKELY(slot.isTaintedByOpaqueObject())
207-
? get(globalObject, propertyName)
208-
: slot.getValue(globalObject, propertyName);
209+
return slot.getValue(globalObject, propertyName);
209210
}
210211

211212
inline bool JSObject::mayInterceptIndexedAccesses(VM& vm)

‎Source/JavaScriptCore/runtime/NullSetterFunction.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ JSC_DEFINE_HOST_FUNCTION(callThrowError, (JSGlobalObject* globalObject, CallFram
9494
auto scope = DECLARE_THROW_SCOPE(vm);
9595
// This function is only called from IC. And we do not want to include this frame in Error's stack.
9696
constexpr bool useCurrentFrame = false;
97-
throwException(globalObject, scope, ErrorInstance::create(globalObject, vm, globalObject->errorStructure(ErrorType::TypeError), ReadonlyPropertyWriteError, jsUndefined(), nullptr, TypeNothing, ErrorType::TypeError, useCurrentFrame));
97+
throwException(globalObject, scope, ErrorInstance::create(globalObject, vm, globalObject->errorStructure(ErrorType::TypeError), ReadonlyPropertyWriteError, JSValue(), nullptr, TypeNothing, ErrorType::TypeError, useCurrentFrame));
9898
return { };
9999
}
100100

‎Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace JSC {
3535
JSObject* createJSWebAssemblyCompileError(JSGlobalObject* globalObject, VM& vm, const String& message)
3636
{
3737
ASSERT(!message.isEmpty());
38-
return ErrorInstance::create(globalObject, vm, globalObject->webAssemblyCompileErrorStructure(), message, jsUndefined(), defaultSourceAppender, TypeNothing, ErrorType::Error, true);
38+
return ErrorInstance::create(globalObject, vm, globalObject->webAssemblyCompileErrorStructure(), message, JSValue(), defaultSourceAppender, TypeNothing, ErrorType::Error, true);
3939
}
4040

4141
} // namespace JSC

0 commit comments

Comments
 (0)