Skip to content

Commit 12203e0

Browse files
verwaestV8 LUCI CQ
verwaest
authored and
V8 LUCI CQ
committed
[exceptions] Unify pending and scheduled exceptions
The difference wasn't very well understood, and actually leads to wonky/broken semantics. Scheduled exceptions were embedder-side exceptions, while pending exceptions were used inside of v8. Conversions were done on the boundaries. Pending exceptions could be cleared locally without try/catch since we'd only propagate exceptions to external try/catch on Promotion. An embedder-side TryCatch could be reused without v8::TryCatch::Reset() first. If there already was an exception and a new one was thrown, the exception was simply overwritten. Or that was the theory. V8 is reentrant. When the embedder returns to V8 a scheduled exception had to be turned into a pending exception (Isolate::PromoteScheduledException()). When V8 returns to the embedder, a pending exception is turned into a scheduled exception (Isolate::OptionalRescheduleException()) to make sure that exceptions can be returned through multiple JS/embedder stack segments. This CL drops the distinction between scheduled and pending exceptions. A possible pending exception is dropped on rentry in V8, meaning the embedder will always be able to call into V8 again despite being given an error. This slightly changes the semantics: if an embedder calls into V8 while a pending exception was sitting around, it won't be "rethrown" on exit to the caller of the embedder. However, this case (rentering V8 without an explicit TryCatch) would already have been partially broken: if V8 would call out to the embedder again, on return to V8 the exception would have been throw there, instead of only when returning to the first call to V8. Embedder code may need to be fixed to account for this change. A similar change was needed in V8 in src/d8/async-hooks-wrapper.cc: Multiple calls to SET_HOOK_FN needed to complete before a possible exception was propagated to the caller. The solution is to wrap the calls in TryCatch with ReThrow. In a follow-up I'll look into not clearing Isolate::pending_exception on entry at all, and instead always clear it when the exception is propagated to TryCatch (as opposed to JS). This would change DCHECKs in V8 that guarantee that we exit V8 when we have a pending exception. I'll support those by adding debugmode only flags. Bug: v8:7235 Change-Id: Iff7c8fb8ff2f0bddbac0d84ce832086fba330dd1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5050065 Commit-Queue: Toon Verwaest <[email protected]> Reviewed-by: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/heads/main@{#91288}
1 parent 62eb262 commit 12203e0

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

68 files changed

+439
-736
lines changed

‎include/v8-exception.h

-1
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,6 @@ class V8_EXPORT TryCatch {
213213
bool can_continue_ : 1;
214214
bool capture_message_ : 1;
215215
bool rethrow_ : 1;
216-
bool has_terminated_ : 1;
217216

218217
friend class internal::Isolate;
219218
friend class internal::ThreadLocalTop;

‎include/v8-internal.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -661,7 +661,7 @@ class Internals {
661661
static const int kBuiltinTier0EntryTableSize = 7 * kApiSystemPointerSize;
662662
static const int kBuiltinTier0TableSize = 7 * kApiSystemPointerSize;
663663
static const int kLinearAllocationAreaSize = 3 * kApiSystemPointerSize;
664-
static const int kThreadLocalTopSize = 30 * kApiSystemPointerSize;
664+
static const int kThreadLocalTopSize = 28 * kApiSystemPointerSize;
665665
static const int kHandleScopeDataSize =
666666
2 * kApiSystemPointerSize + 2 * kApiInt32Size;
667667

‎src/api/api-inl.h

+12-14
Original file line numberDiff line numberDiff line change
@@ -181,15 +181,14 @@ class V8_NODISCARD CallDepthScope {
181181
: isolate_(isolate),
182182
context_(context),
183183
did_enter_context_(false),
184-
escaped_(false),
185184
safe_for_termination_(isolate->next_v8_call_is_safe_for_termination()),
186185
interrupts_scope_(isolate_, i::StackGuard::TERMINATE_EXECUTION,
187186
isolate_->only_terminate_in_safe_scope()
188187
? (safe_for_termination_
189188
? i::InterruptsScope::kRunInterrupts
190189
: i::InterruptsScope::kPostponeInterrupts)
191190
: i::InterruptsScope::kNoop) {
192-
isolate_->thread_local_top()->IncrementCallDepth(this);
191+
isolate_->thread_local_top()->IncrementCallDepth<do_callback>(this);
193192
isolate_->set_next_v8_call_is_safe_for_termination(false);
194193
if (!context.IsEmpty()) {
195194
i::DisallowGarbageCollection no_gc;
@@ -215,7 +214,16 @@ class V8_NODISCARD CallDepthScope {
215214
i::Handle<i::Context> env = Utils::OpenHandle(*context_);
216215
microtask_queue = env->native_context()->microtask_queue();
217216
}
218-
if (!escaped_) isolate_->thread_local_top()->DecrementCallDepth(this);
217+
isolate_->thread_local_top()->DecrementCallDepth(this);
218+
// Clear the pending exception when exiting V8 to avoid memory leaks.
219+
// Also clear termination exceptions iff there's no TryCatch handler.
220+
// TODO(verwaest): Drop this once we propagate exceptions to external
221+
// TryCatch on Throw. This should be debug-only.
222+
if (isolate_->thread_local_top()->CallDepthIsZero() &&
223+
(isolate_->thread_local_top()->try_catch_handler_ == nullptr ||
224+
!isolate_->is_execution_terminating())) {
225+
isolate_->clear_pending_exception();
226+
}
219227
if (do_callback) isolate_->FireCallCompletedCallback(microtask_queue);
220228
#ifdef DEBUG
221229
if (do_callback) {
@@ -233,16 +241,6 @@ class V8_NODISCARD CallDepthScope {
233241
CallDepthScope(const CallDepthScope&) = delete;
234242
CallDepthScope& operator=(const CallDepthScope&) = delete;
235243

236-
void Escape() {
237-
DCHECK(!escaped_);
238-
escaped_ = true;
239-
auto thread_local_top = isolate_->thread_local_top();
240-
thread_local_top->DecrementCallDepth(this);
241-
bool clear_exception = thread_local_top->CallDepthIsZero() &&
242-
thread_local_top->try_catch_handler_ == nullptr;
243-
isolate_->OptionalRescheduleException(clear_exception);
244-
}
245-
246244
private:
247245
#ifdef DEBUG
248246
bool CheckKeptObjectsClearedAfterMicrotaskCheckpoint(
@@ -259,8 +257,8 @@ class V8_NODISCARD CallDepthScope {
259257

260258
i::Isolate* const isolate_;
261259
Local<Context> context_;
260+
262261
bool did_enter_context_ : 1;
263-
bool escaped_ : 1;
264262
bool safe_for_termination_ : 1;
265263
i::InterruptsScope interrupts_scope_;
266264
i::Address previous_stack_height_;

‎src/api/api-macros.h

+6-15
Original file line numberDiff line numberDiff line change
@@ -61,15 +61,11 @@
6161
i::VMState<v8::OTHER> __state__((i_isolate)); \
6262
bool has_pending_exception = false
6363

64-
#define PREPARE_FOR_EXECUTION_WITH_CONTEXT(context, class_name, function_name, \
65-
HandleScopeClass, do_callback) \
66-
auto i_isolate = reinterpret_cast<i::Isolate*>(context->GetIsolate()); \
67-
ENTER_V8_HELPER_INTERNAL(i_isolate, context, class_name, function_name, \
68-
HandleScopeClass, do_callback);
69-
70-
#define PREPARE_FOR_EXECUTION(context, class_name, function_name) \
71-
PREPARE_FOR_EXECUTION_WITH_CONTEXT(context, class_name, function_name, \
72-
InternalEscapableScope, false)
64+
#define PREPARE_FOR_EXECUTION(context, class_name, function_name) \
65+
auto i_isolate = reinterpret_cast<i::Isolate*>(context->GetIsolate()); \
66+
i_isolate->clear_pending_exception(); \
67+
ENTER_V8_HELPER_INTERNAL(i_isolate, context, class_name, function_name, \
68+
InternalEscapableScope, false);
7369

7470
#define ENTER_V8(i_isolate, context, class_name, function_name, \
7571
HandleScopeClass) \
@@ -113,12 +109,7 @@
113109
#endif // DEBUG
114110

115111
#define EXCEPTION_BAILOUT_CHECK_SCOPED_DO_NOT_USE(i_isolate, value) \
116-
do { \
117-
if (has_pending_exception) { \
118-
call_depth_scope.Escape(); \
119-
return value; \
120-
} \
121-
} while (false)
112+
if (has_pending_exception) return value;
122113

123114
#define RETURN_ON_FAILED_EXECUTION(T) \
124115
EXCEPTION_BAILOUT_CHECK_SCOPED_DO_NOT_USE(i_isolate, MaybeLocal<T>())

0 commit comments

Comments
 (0)