Skip to content

Commit 67abaaa

Browse files
kmiller68kmiller68
committed
ThrowScopes should not be able to clear termination exceptions
https://bugs.webkit.org/show_bug.cgi?id=305496 rdar://168161400 Reviewed by Yusuke Suzuki. Right now when folks call clearException on a ThrowScope they often end up unintentionally clearing termination exceptions. This is a bit of a footgun. With this change it's now only possible to tryClearException, which fails when there's a termination exception pending. Long term, the goal is to have all "interior" code use ThrowScopes as they have to support propagating termination exceptions. I fixed a few places in this patch such as IteratorOperations and will fix the remaining ones when renaming CatchScope in a follow up change. No new tests, no behavior change. Canonical link: https://commits.webkit.org/305675@main
1 parent ff4078f commit 67abaaa

30 files changed

Lines changed: 117 additions & 129 deletions

‎Source/JavaScriptCore/interpreter/Interpreter.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -895,7 +895,7 @@ static void sanitizeRemoteFunctionException(VM& vm, JSRemoteFunction* remoteFunc
895895

896896
JSGlobalObject* globalObject = remoteFunction->globalObject();
897897
JSValue exceptionValue = exception->value();
898-
scope.clearException();
898+
TRY_CLEAR_EXCEPTION(scope, void());
899899

900900
// Avoid user-observable ToString()
901901
String exceptionString;

‎Source/JavaScriptCore/jit/JITOperations.cpp‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -248,22 +248,18 @@ JSC_DEFINE_JIT_OPERATION(operationThrowRemoteFunctionException, EncodedJSValue,
248248

249249
// We should only be here when "rethrowing" an exception
250250
RELEASE_ASSERT(exception);
251+
JSValue exceptionValue = exception->value();
251252

252-
if (vm.isTerminationException(exception)) [[unlikely]] {
253+
if (!scope.tryClearException()) [[unlikely]] {
253254
scope.release();
254255
OPERATION_RETURN(scope, encodedJSValue());
255256
}
256257

257-
JSValue exceptionValue = exception->value();
258-
scope.clearException();
259-
260258
String exceptionString = exceptionValue.toWTFString(globalObject);
261-
Exception* toStringException = scope.exception();
262-
if (toStringException && vm.isTerminationException(toStringException)) [[unlikely]] {
259+
if (!scope.tryClearException()) [[unlikely]] {
263260
scope.release();
264261
OPERATION_RETURN(scope, encodedJSValue());
265262
}
266-
scope.clearException();
267263

268264
if (exceptionString.length())
269265
OPERATION_RETURN(scope, throwVMTypeError(globalObject, scope, exceptionString));
@@ -5214,15 +5210,15 @@ JSC_DEFINE_NOEXCEPT_JIT_OPERATION(operationRetrieveAndClearExceptionIfCatchable,
52145210
RELEASE_ASSERT(!!scope.exception());
52155211

52165212
Exception* exception = scope.exception();
5217-
if (vm.isTerminationException(exception)) [[unlikely]] {
5218-
genericUnwind(vm, callFrame);
5219-
return nullptr;
5220-
}
52215213

52225214
// We want to clear the exception here rather than in the catch prologue
52235215
// JIT code because clearing it also entails clearing a bit in an Atomic
52245216
// bit field in VMTraps.
5225-
scope.clearException();
5217+
if (!scope.tryClearException()) [[unlikely]] {
5218+
genericUnwind(vm, callFrame);
5219+
return nullptr;
5220+
}
5221+
52265222
return exception;
52275223
}
52285224

‎Source/JavaScriptCore/llint/LLIntSlowPaths.cpp‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2468,13 +2468,12 @@ LLINT_SLOW_PATH_DECL(slow_path_retrieve_and_clear_exception_if_catchable)
24682468
RELEASE_ASSERT(!!throwScope.exception());
24692469

24702470
Exception* exception = throwScope.exception();
2471-
if (vm.isTerminationException(exception))
2472-
LLINT_RETURN_TWO(pc, nullptr);
2473-
24742471
// We want to clear the exception here rather than in the catch prologue
24752472
// JIT code because clearing it also entails clearing a bit in an Atomic
24762473
// bit field in VMTraps.
2477-
throwScope.clearException();
2474+
if (!throwScope.tryClearException())
2475+
LLINT_RETURN_TWO(pc, nullptr);
2476+
24782477
LLINT_RETURN_TWO(pc, exception);
24792478
}
24802479

‎Source/JavaScriptCore/runtime/ExceptionScope.h‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ class ExceptionScope {
6969
const void* stackPosition() const { return this; }
7070
#endif
7171

72+
[[nodiscard]] inline bool tryClearException();
73+
7274
protected:
7375
ExceptionScope(VM&, ExceptionEventLocation);
7476
ExceptionScope(const ExceptionScope&) = delete;
@@ -99,6 +101,8 @@ class ExceptionScope {
99101
ALWAYS_INLINE void assertNoExceptionExceptTermination() { ASSERT(!exception() || m_vm.hasPendingTerminationException()); }
100102
ALWAYS_INLINE void releaseAssertNoExceptionExceptTermination() { RELEASE_ASSERT(!exception() || m_vm.hasPendingTerminationException()); }
101103

104+
[[nodiscard]] ALWAYS_INLINE bool tryClearException();
105+
102106
protected:
103107
ALWAYS_INLINE ExceptionScope(VM& vm)
104108
: m_vm(vm)
@@ -113,6 +117,17 @@ class ExceptionScope {
113117

114118
#endif // ENABLE(EXCEPTION_SCOPE_VERIFICATION)
115119

120+
bool ExceptionScope::tryClearException()
121+
{
122+
SUPPRESS_FORWARD_DECL_ARG Exception* exception = this->exception();
123+
124+
SUPPRESS_FORWARD_DECL_ARG if (exception && m_vm.isTerminationException(exception)) [[unlikely]]
125+
return false;
126+
127+
m_vm.clearException();
128+
return true;
129+
}
130+
116131
#define RETURN_IF_EXCEPTION(scope__, value__) do { \
117132
SUPPRESS_UNCOUNTED_LOCAL JSC::VM& vm = (scope__).vm(); \
118133
EXCEPTION_ASSERT(!!(scope__).exception() == vm.traps().needHandling(JSC::VMTraps::NeedExceptionHandling)); \
@@ -122,6 +137,12 @@ class ExceptionScope {
122137
} \
123138
} while (false)
124139

140+
141+
#define TRY_CLEAR_EXCEPTION(scope__, value__) do { \
142+
if (!(scope__).tryClearException()) [[unlikely]] \
143+
return value__; \
144+
} while (false)
145+
125146
#define RETURN_IF_EXCEPTION_WITH_TRAPS_DEFERRED(scope__, value__) do { \
126147
if ((scope__).exception()) [[unlikely]] \
127148
return value__; \

‎Source/JavaScriptCore/runtime/FunctionConstructor.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ JSObject* constructFunction(JSGlobalObject* globalObject, const ArgList& args, c
200200

201201
if (!globalObject->evalEnabled()) [[unlikely]] {
202202
if (globalObject->trustedTypesEnforcement() != TrustedTypesEnforcement::EnforcedWithEvalEnabled) {
203-
scope.clearException();
203+
TRY_CLEAR_EXCEPTION(scope, nullptr);
204204
globalObject->globalObjectMethodTable()->reportViolationForUnsafeEval(globalObject, !code.isNull() ? WTF::move(code) : nullString());
205205
throwException(globalObject, scope, createEvalError(globalObject, globalObject->evalDisabledErrorMessage()));
206206
return nullptr;

‎Source/JavaScriptCore/runtime/IteratorOperations.cpp‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -127,34 +127,30 @@ JSValue iteratorStepWithCachedCall(JSGlobalObject* globalObject, IterationRecord
127127
void iteratorClose(JSGlobalObject* globalObject, JSValue iterator)
128128
{
129129
VM& vm = globalObject->vm();
130-
auto throwScope = DECLARE_THROW_SCOPE(vm);
131-
auto catchScope = DECLARE_CATCH_SCOPE(vm);
130+
auto scope = DECLARE_THROW_SCOPE(vm);
132131

133-
Exception* exception = nullptr;
134-
if (catchScope.exception()) [[unlikely]] {
135-
exception = catchScope.exception();
136-
catchScope.clearException();
137-
}
132+
Exception* exception = scope.exception();
133+
TRY_CLEAR_EXCEPTION(scope, void());
138134

139135
JSValue returnFunction = iterator.get(globalObject, vm.propertyNames->returnKeyword);
140-
if (throwScope.exception()) [[unlikely]] {
136+
if (scope.exception()) [[unlikely]] {
141137
if (exception)
142-
throwException(globalObject, throwScope, exception);
138+
throwException(globalObject, scope, exception);
143139
return;
144140
}
145141

146142
if (returnFunction.isUndefinedOrNull()) {
147143
if (exception)
148-
throwException(globalObject, throwScope, exception);
144+
throwException(globalObject, scope, exception);
149145
return;
150146
}
151147

152148
auto returnFunctionCallData = JSC::getCallDataInline(returnFunction);
153149
if (returnFunctionCallData.type == CallData::Type::None) {
154150
if (exception)
155-
throwException(globalObject, throwScope, exception);
151+
throwException(globalObject, scope, exception);
156152
else
157-
throwTypeError(globalObject, throwScope);
153+
throwTypeError(globalObject, scope);
158154
return;
159155
}
160156

@@ -163,14 +159,14 @@ void iteratorClose(JSGlobalObject* globalObject, JSValue iterator)
163159
JSValue innerResult = call(globalObject, returnFunction, returnFunctionCallData, iterator, returnFunctionArguments);
164160

165161
if (exception) {
166-
throwException(globalObject, throwScope, exception);
162+
throwException(globalObject, scope, exception);
167163
return;
168164
}
169165

170-
RETURN_IF_EXCEPTION(throwScope, void());
166+
RETURN_IF_EXCEPTION(scope, void());
171167

172168
if (!innerResult.isObject()) {
173-
throwTypeError(globalObject, throwScope, "Iterator result interface is not an object."_s);
169+
throwTypeError(globalObject, scope, "Iterator result interface is not an object."_s);
174170
return;
175171
}
176172
}

‎Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ JSC_DEFINE_HOST_FUNCTION(internalPromiseConstructorFuncInternalAll, (JSGlobalObj
9090
auto callReject = [&]() -> void {
9191
Exception* exception = scope.exception();
9292
ASSERT(exception);
93-
scope.clearException();
93+
TRY_CLEAR_EXCEPTION(scope, void());
9494
scope.release();
9595
promise->reject(vm, globalObject, exception);
9696
};

‎Source/JavaScriptCore/runtime/JSIteratorPrototype.cpp‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,8 @@ JSC_DEFINE_HOST_FUNCTION(iteratorProtoFuncForEach, (JSGlobalObject* globalObject
164164

165165
JSValue callbackArg = callFrame->argument(0);
166166
if (!callbackArg.isCallable()) {
167-
{
168-
auto catchScope = DECLARE_CATCH_SCOPE(vm);
169-
iteratorClose(globalObject, thisValue);
170-
catchScope.clearException();
171-
}
167+
iteratorClose(globalObject, thisValue);
168+
TRY_CLEAR_EXCEPTION(scope, { });
172169
return throwVMTypeError(globalObject, scope, "Iterator.prototype.forEach requires the callback argument to be callable."_s);
173170
}
174171

‎Source/JavaScriptCore/runtime/JSPromise.cpp‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,7 @@ JSPromise* JSPromise::rejectWithCaughtException(JSGlobalObject* globalObject, Th
239239
VM& vm = globalObject->vm();
240240
Exception* exception = scope.exception();
241241
ASSERT(exception);
242-
if (vm.isTerminationException(exception)) [[unlikely]] {
243-
scope.release();
244-
return this;
245-
}
246-
scope.clearException();
242+
TRY_CLEAR_EXCEPTION(scope, nullptr);
247243
scope.release();
248244
reject(vm, globalObject, exception->value());
249245
return this;

‎Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ static JSObject* promiseRaceSlow(JSGlobalObject* globalObject, CallFrame* callFr
169169
auto callRejectWithScopeException = [&]() -> void {
170170
Exception* exception = scope.exception();
171171
ASSERT(exception);
172-
scope.clearException();
172+
TRY_CLEAR_EXCEPTION(scope, void());
173173
callReject(exception->value());
174174
};
175175

@@ -252,7 +252,7 @@ JSC_DEFINE_HOST_FUNCTION(promiseConstructorFuncRace, (JSGlobalObject* globalObje
252252
auto callReject = [&]() -> void {
253253
Exception* exception = scope.exception();
254254
ASSERT(exception);
255-
scope.clearException();
255+
TRY_CLEAR_EXCEPTION(scope, void());
256256
scope.release();
257257
promise->reject(vm, globalObject, exception);
258258
};
@@ -318,7 +318,7 @@ static JSObject* promiseAllSlow(JSGlobalObject* globalObject, CallFrame* callFra
318318
auto callRejectWithScopeException = [&]() -> void {
319319
Exception* exception = scope.exception();
320320
ASSERT(exception);
321-
scope.clearException();
321+
TRY_CLEAR_EXCEPTION(scope, void());
322322
callReject(exception->value());
323323
};
324324

@@ -451,7 +451,7 @@ JSC_DEFINE_HOST_FUNCTION(promiseConstructorFuncAll, (JSGlobalObject* globalObjec
451451
auto callReject = [&]() -> void {
452452
Exception* exception = scope.exception();
453453
ASSERT(exception);
454-
scope.clearException();
454+
TRY_CLEAR_EXCEPTION(scope, void());
455455
scope.release();
456456
promise->reject(vm, globalObject, exception);
457457
};
@@ -639,7 +639,7 @@ static JSObject* promiseAllSettledSlow(JSGlobalObject* globalObject, CallFrame*
639639
auto callRejectWithScopeException = [&]() -> void {
640640
Exception* exception = scope.exception();
641641
ASSERT(exception);
642-
scope.clearException();
642+
TRY_CLEAR_EXCEPTION(scope, void());
643643
callReject(exception->value());
644644
};
645645

@@ -777,7 +777,7 @@ JSC_DEFINE_HOST_FUNCTION(promiseConstructorFuncAllSettled, (JSGlobalObject* glob
777777
auto callReject = [&]() -> void {
778778
Exception* exception = scope.exception();
779779
ASSERT(exception);
780-
scope.clearException();
780+
TRY_CLEAR_EXCEPTION(scope, void());
781781
scope.release();
782782
promise->reject(vm, globalObject, exception);
783783
};
@@ -1122,7 +1122,7 @@ static JSObject* promiseAnySlow(JSGlobalObject* globalObject, CallFrame* callFra
11221122
auto callRejectWithScopeException = [&]() -> void {
11231123
Exception* exception = scope.exception();
11241124
ASSERT(exception);
1125-
scope.clearException();
1125+
TRY_CLEAR_EXCEPTION(scope, void());
11261126
callReject(exception->value());
11271127
};
11281128

@@ -1250,7 +1250,7 @@ JSC_DEFINE_HOST_FUNCTION(promiseConstructorFuncAny, (JSGlobalObject* globalObjec
12501250
auto callReject = [&]() -> void {
12511251
Exception* exception = scope.exception();
12521252
ASSERT(exception);
1253-
scope.clearException();
1253+
TRY_CLEAR_EXCEPTION(scope, void());
12541254
scope.release();
12551255
promise->reject(vm, globalObject, exception);
12561256
};

0 commit comments

Comments
 (0)