Commit 916f2c5
src: avoid using v8 on Isolate termination
Fix multiple instances of those uncovered while running the tests on
debug builds.
Fixes: nodejs/node-v8#227
PR-URL: #44669
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>1 parent 1308e68 commit 916f2c5
3 files changed
Lines changed: 31 additions & 18 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -162,14 +162,17 @@ bool AsyncHooks::pop_async_context(double async_id) { | |||
| 162 | 162 | } | |
| 163 | 163 | ||
| 164 | 164 | void AsyncHooks::clear_async_id_stack() { | |
| 165 | - Isolate* isolate = env()->isolate(); | ||
| 166 | - HandleScope handle_scope(isolate); | ||
| 167 | - if (!js_execution_async_resources_.IsEmpty()) { | ||
| 168 | - USE(PersistentToLocal::Strong(js_execution_async_resources_) | ||
| 169 | - ->Set(env()->context(), | ||
| 170 | - env()->length_string(), | ||
| 171 | - Integer::NewFromUnsigned(isolate, 0))); | ||
| 165 | + if (env()->can_call_into_js()) { | ||
| 166 | + Isolate* isolate = env()->isolate(); | ||
| 167 | + HandleScope handle_scope(isolate); | ||
| 168 | + if (!js_execution_async_resources_.IsEmpty()) { | ||
| 169 | + USE(PersistentToLocal::Strong(js_execution_async_resources_) | ||
| 170 | + ->Set(env()->context(), | ||
| 171 | + env()->length_string(), | ||
| 172 | + Integer::NewFromUnsigned(isolate, 0))); | ||
| 173 | + } | ||
| 172 | 174 | } | |
| 175 | + | ||
| 173 | 176 | native_execution_async_resources_.clear(); | |
| 174 | 177 | native_execution_async_resources_.shrink_to_fit(); | |
| 175 | 178 | ||
@@ -1157,7 +1160,13 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) { | |||
| 1157 | 1160 | TRACE_EVENT0(TRACING_CATEGORY_NODE1(environment), | |
| 1158 | 1161 | "RunAndClearNativeImmediates"); | |
| 1159 | 1162 | HandleScope handle_scope(isolate_); | |
| 1160 | - InternalCallbackScope cb_scope(this, Object::New(isolate_), { 0, 0 }); | ||
| 1163 | + // In case the Isolate is no longer accessible just use an empty Local. This | ||
| 1164 | + // is not an issue for InternalCallbackScope as this case is already handled | ||
| 1165 | + // in its constructor but we avoid calls into v8 which can crash the process | ||
| 1166 | + // in debug builds. | ||
| 1167 | + Local<Object> obj = | ||
| 1168 | + can_call_into_js() ? Object::New(isolate_) : Local<Object>(); | ||
| 1169 | + InternalCallbackScope cb_scope(this, obj, {0, 0}); | ||
| 1161 | 1170 | ||
| 1162 | 1171 | size_t ref_count = 0; | |
| 1163 | 1172 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1120,13 +1120,14 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, | |||
| 1120 | 1120 | // It is possible for the stream close to occur before the stream is | |
| 1121 | 1121 | // ever passed on to the javascript side. If that happens, the callback | |
| 1122 | 1122 | // will return false. | |
| 1123 | - Local<Value> arg = Integer::NewFromUnsigned(isolate, code); | ||
| 1124 | - MaybeLocal<Value> answer = | ||
| 1125 | - stream->MakeCallback(env->http2session_on_stream_close_function(), | ||
| 1126 | - 1, &arg); | ||
| 1127 | - if (answer.IsEmpty() || answer.ToLocalChecked()->IsFalse()) { | ||
| 1128 | - // Skip to destroy | ||
| 1129 | - stream->Destroy(); | ||
| 1123 | + if (env->can_call_into_js()) { | ||
| 1124 | + Local<Value> arg = Integer::NewFromUnsigned(isolate, code); | ||
| 1125 | + MaybeLocal<Value> answer = stream->MakeCallback( | ||
| 1126 | + env->http2session_on_stream_close_function(), 1, &arg); | ||
| 1127 | + if (answer.IsEmpty() || answer.ToLocalChecked()->IsFalse()) { | ||
| 1128 | + // Skip to destroy | ||
| 1129 | + stream->Destroy(); | ||
| 1130 | + } | ||
| 1130 | 1131 | } | |
| 1131 | 1132 | return 0; | |
| 1132 | 1133 | } | |
@@ -1629,9 +1630,11 @@ void Http2Session::MaybeScheduleWrite() { | |||
| 1629 | 1630 | ||
| 1630 | 1631 | // Sending data may call arbitrary JS code, so keep track of | |
| 1631 | 1632 | // async context. | |
| 1632 | - HandleScope handle_scope(env->isolate()); | ||
| 1633 | - InternalCallbackScope callback_scope(this); | ||
| 1634 | - SendPendingData(); | ||
| 1633 | + if (env->can_call_into_js()) { | ||
| 1634 | + HandleScope handle_scope(env->isolate()); | ||
| 1635 | + InternalCallbackScope callback_scope(this); | ||
| 1636 | + SendPendingData(); | ||
| 1637 | + } | ||
| 1635 | 1638 | }); | |
| 1636 | 1639 | } | |
| 1637 | 1640 | } | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -401,6 +401,7 @@ int NodePlatform::NumberOfWorkerThreads() { | |||
| 401 | 401 | } | |
| 402 | 402 | ||
| 403 | 403 | void PerIsolatePlatformData::RunForegroundTask(std::unique_ptr<Task> task) { | |
| 404 | + if (isolate_->IsExecutionTerminating()) return task->Run(); | ||
| 404 | 405 | DebugSealHandleScope scope(isolate_); | |
| 405 | 406 | Environment* env = Environment::GetCurrent(isolate_); | |
| 406 | 407 | if (env != nullptr) { | |
0 commit comments