Skip to content

Commit a487167

Browse files
tzikCommit Bot
tzik
authored and
Commit Bot
committed
Cancel EnqueueMicrotask on detached contexts
Context::microtask_context can be null after v8::Context::DetachGlobal is called, and that should cancel microtasks that are associated to the detached context. However, there are several callers left without the null check to the microtask queue, and that causes crashes. This CL adds the null check and cancellation as the crash fix. Bug: chromium:937784 Change-Id: Ie8d107f28f200cee6e75798e3f72c5ed7a2a461c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1545139 Commit-Queue: Taiju Tsuiki <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#60623}
1 parent e87e3b1 commit a487167

5 files changed

Lines changed: 58 additions & 7 deletions

File tree

‎src/api.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8555,7 +8555,8 @@ void Isolate::EnqueueMicrotask(Local<Function> v8_function) {
85558555
if (!i::JSReceiver::GetContextForMicrotask(function).ToHandle(
85568556
&handler_context))
85578557
handler_context = isolate->native_context();
8558-
handler_context->microtask_queue()->EnqueueMicrotask(this, v8_function);
8558+
MicrotaskQueue* microtask_queue = handler_context->microtask_queue();
8559+
if (microtask_queue) microtask_queue->EnqueueMicrotask(this, v8_function);
85598560
}
85608561

85618562
void Isolate::EnqueueMicrotask(MicrotaskCallback callback, void* data) {

‎src/objects.cc‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6112,7 +6112,9 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
61126112
promise)
61136113
.Check();
61146114
}
6115-
isolate->native_context()->microtask_queue()->EnqueueMicrotask(*task);
6115+
MicrotaskQueue* microtask_queue =
6116+
isolate->native_context()->microtask_queue();
6117+
if (microtask_queue) microtask_queue->EnqueueMicrotask(*task);
61166118

61176119
// 13. Return undefined.
61186120
return isolate->factory()->undefined_value();
@@ -6202,8 +6204,11 @@ Handle<Object> JSPromise::TriggerPromiseReactions(Isolate* isolate,
62026204
PromiseRejectReactionJobTask::kPromiseOrCapabilityOffset));
62036205
}
62046206

6205-
handler_context->microtask_queue()->EnqueueMicrotask(
6206-
*Handle<PromiseReactionJobTask>::cast(task));
6207+
MicrotaskQueue* microtask_queue = handler_context->microtask_queue();
6208+
if (microtask_queue) {
6209+
microtask_queue->EnqueueMicrotask(
6210+
*Handle<PromiseReactionJobTask>::cast(task));
6211+
}
62076212
}
62086213

62096214
return isolate->factory()->undefined_value();

‎src/objects/js-promise.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ class JSPromise : public JSObject {
5353
void set_status(Promise::PromiseState status);
5454

5555
// ES section #sec-fulfillpromise
56-
static Handle<Object> Fulfill(Handle<JSPromise> promise,
57-
Handle<Object> value);
56+
V8_EXPORT_PRIVATE static Handle<Object> Fulfill(Handle<JSPromise> promise,
57+
Handle<Object> value);
5858
// ES section #sec-rejectpromise
5959
static Handle<Object> Reject(Handle<JSPromise> promise, Handle<Object> reason,
6060
bool debug_event = true);

‎src/runtime/runtime-promise.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,9 @@ RUNTIME_FUNCTION(Runtime_EnqueueMicrotask) {
7979

8080
Handle<CallableTask> microtask = isolate->factory()->NewCallableTask(
8181
function, handle(function->native_context(), isolate));
82-
function->native_context()->microtask_queue()->EnqueueMicrotask(*microtask);
82+
MicrotaskQueue* microtask_queue =
83+
function->native_context()->microtask_queue();
84+
if (microtask_queue) microtask_queue->EnqueueMicrotask(*microtask);
8385
return ReadOnlyRoots(isolate).undefined_value();
8486
}
8587

‎test/unittests/microtask-queue-unittest.cc‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ class WithFinalizationGroupMixin : public TMixin {
4040
save_flags_ = new SaveFlags();
4141
FLAG_harmony_weak_refs = true;
4242
FLAG_expose_gc = true;
43+
FLAG_allow_natives_syntax = true;
4344
TMixin::SetUpTestCase();
4445
}
4546

@@ -541,5 +542,47 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_Chain) {
541542
Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsTrue());
542543
}
543544

545+
TEST_F(MicrotaskQueueTest, DetachGlobal_InactiveHandler) {
546+
Local<v8::Context> sub_context = v8::Context::New(v8_isolate());
547+
Utils::OpenHandle(*sub_context)
548+
->native_context()
549+
->set_microtask_queue(microtask_queue());
550+
551+
Handle<JSArray> result;
552+
Handle<JSFunction> stale_handler;
553+
Handle<JSPromise> stale_promise;
554+
{
555+
v8::Context::Scope scope(sub_context);
556+
result = RunJS<JSArray>("var result = [false, false]; result");
557+
stale_handler = RunJS<JSFunction>("() => { result[0] = true; }");
558+
stale_promise = RunJS<JSPromise>(
559+
"var stale_promise = new Promise(()=>{});"
560+
"stale_promise");
561+
RunJS("stale_promise.then(() => { result [1] = true; });");
562+
}
563+
sub_context->DetachGlobal();
564+
sub_context.Clear();
565+
566+
// The context of |stale_handler| and |stale_promise| is detached at this
567+
// point.
568+
// Ensure that resolution handling for |stale_handler| is cancelled without
569+
// crash. Also, the resolution of |stale_promise| is also cancelled.
570+
571+
SetGlobalProperty("stale_handler", Utils::ToLocal(stale_handler));
572+
RunJS("%EnqueueMicrotask(stale_handler)");
573+
574+
v8_isolate()->EnqueueMicrotask(Utils::ToLocal(stale_handler));
575+
576+
JSPromise::Fulfill(
577+
stale_promise,
578+
handle(ReadOnlyRoots(isolate()).undefined_value(), isolate()));
579+
580+
microtask_queue()->RunMicrotasks(isolate());
581+
EXPECT_TRUE(
582+
Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsFalse());
583+
EXPECT_TRUE(
584+
Object::GetElement(isolate(), result, 1).ToHandleChecked()->IsFalse());
585+
}
586+
544587
} // namespace internal
545588
} // namespace v8

0 commit comments

Comments
 (0)