src: tighten handle scopes for stream operations

Put `HandleScope`s and `Context::Scope`s where they are used,
and don’t create one for native stream callbacks automatically.

This is slightly less convenient but means that stream listeners
that don’t actually call back into JS don’t have to pay the
(small) cost of setting these up.

PR-URL: https://github.com/nodejs/node/pull/18936
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

5 files changed, 38 insertions, 37 deletions

diff --git a/src/node_http2.cc b/src/node_http2.cc
index d683b075bc..939e0011bd 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1132,6 +1132,8 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
   Http2Stream* stream = static_cast<Http2Stream*>(stream_);
   Http2Session* session = stream->session();
   Environment* env = stream->env();
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
 
   if (nread < 0) {
     PassReadErrorToPreviousListener(nread);
@@ -1422,6 +1424,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
 void Http2Session::MaybeScheduleWrite() {
   CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0);
   if (session_ != nullptr && nghttp2_session_want_write(session_)) {
+    HandleScope handle_scope(env()->isolate());
     DEBUG_HTTP2SESSION(this, "scheduling write");
     flags_ |= SESSION_STATE_WRITE_SCHEDULED;
     env()->SetImmediate([](Environment* env, void* data) {
@@ -1632,6 +1635,8 @@ inline Http2Stream* Http2Session::SubmitRequest(
 
 // Callback used to receive inbound data from the i/o stream
 void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
+  HandleScope handle_scope(env()->isolate());
+  Context::Scope context_scope(env()->context());
   Http2Scope h2scope(this);
   CHECK_NE(stream_, nullptr);
   DEBUG_HTTP2SESSION2(this, "receiving %d bytes", nread);
@@ -1661,8 +1666,6 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
     CHECK_LE(static_cast<size_t>(nread), stream_buf_.len);
 
     Isolate* isolate = env()->isolate();
-    HandleScope scope(isolate);
-    Context::Scope context_scope(env()->context());
 
     // Create an array buffer for the read data. DATA frames will be emitted
     // as slices of this array buffer to avoid having to copy memory.
diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
index 1534dcd1d5..c87393e6fc 100644
--- a/src/stream_base-inl.h
+++ b/src/stream_base-inl.h
@@ -106,22 +106,33 @@ inline void StreamResource::RemoveStreamListener(StreamListener* listener) {
   listener->previous_listener_ = nullptr;
 }
 
-
 inline uv_buf_t StreamResource::EmitAlloc(size_t suggested_size) {
+#ifdef DEBUG
+  v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
+#endif
   return listener_->OnStreamAlloc(suggested_size);
 }
 
 inline void StreamResource::EmitRead(ssize_t nread, const uv_buf_t& buf) {
+#ifdef DEBUG
+  v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
+#endif
   if (nread > 0)
     bytes_read_ += static_cast<uint64_t>(nread);
   listener_->OnStreamRead(nread, buf);
 }
 
 inline void StreamResource::EmitAfterWrite(WriteWrap* w, int status) {
+#ifdef DEBUG
+  v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
+#endif
   listener_->OnStreamAfterWrite(w, status);
 }
 
 inline void StreamResource::EmitAfterShutdown(ShutdownWrap* w, int status) {
+#ifdef DEBUG
+  v8::SealHandleScope handle_scope(v8::Isolate::GetCurrent());
+#endif
   listener_->OnStreamAfterShutdown(w, status);
 }
 
@@ -133,29 +144,6 @@ inline Environment* StreamBase::stream_env() const {
   return env_;
 }
 
-inline void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
-  AfterRequest(req_wrap, [&]() {
-    EmitAfterWrite(req_wrap, status);
-  });
-}
-
-inline void StreamBase::AfterShutdown(ShutdownWrap* req_wrap, int status) {
-  AfterRequest(req_wrap, [&]() {
-    EmitAfterShutdown(req_wrap, status);
-  });
-}
-
-template<typename Wrap, typename EmitEvent>
-inline void StreamBase::AfterRequest(Wrap* req_wrap, EmitEvent emit) {
-  Environment* env = stream_env();
-
-  v8::HandleScope handle_scope(env->isolate());
-  v8::Context::Scope context_scope(env->context());
-
-  emit();
-  req_wrap->Dispose();
-}
-
 inline int StreamBase::Shutdown(v8::Local<v8::Object> req_wrap_obj) {
   Environment* env = stream_env();
   if (req_wrap_obj.IsEmpty()) {
@@ -387,7 +375,8 @@ void StreamBase::JSMethod(const FunctionCallbackInfo<Value>& args) {
 
 
 inline void ShutdownWrap::OnDone(int status) {
-  stream()->AfterShutdown(this, status);
+  stream()->EmitAfterShutdown(this, status);
+  Dispose();
 }
 
 inline void WriteWrap::SetAllocatedStorage(char* data, size_t size) {
@@ -405,7 +394,8 @@ inline size_t WriteWrap::StorageSize() const {
 }
 
 inline void WriteWrap::OnDone(int status) {
-  stream()->AfterWrite(this, status);
+  stream()->EmitAfterWrite(this, status);
+  Dispose();
 }
 
 inline void StreamReq::Done(int status, const char* error_str) {
diff --git a/src/stream_base.cc b/src/stream_base.cc
index 1d1d324841..8838a1a6df 100644
--- a/src/stream_base.cc
+++ b/src/stream_base.cc
@@ -387,6 +387,8 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished(
   StreamBase* stream = static_cast<StreamBase*>(stream_);
   Environment* env = stream->stream_env();
   AsyncWrap* async_wrap = req_wrap->GetAsyncWrap();
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
   Local<Object> req_wrap_obj = async_wrap->object();
 
   Local<Value> argv[] = {
diff --git a/src/stream_base.h b/src/stream_base.h
index 8af05059f4..6962648650 100644
--- a/src/stream_base.h
+++ b/src/stream_base.h
@@ -61,7 +61,8 @@ class ShutdownWrap : public StreamReq {
                v8::Local<v8::Object> req_wrap_obj)
     : StreamReq(stream, req_wrap_obj) { }
 
-  void OnDone(int status) override;  // Just calls stream()->AfterShutdown()
+  // Call stream()->EmitAfterShutdown() and dispose of this request wrap.
+  void OnDone(int status) override;
 };
 
 class WriteWrap : public StreamReq {
@@ -78,7 +79,8 @@ class WriteWrap : public StreamReq {
     free(storage_);
   }
 
-  void OnDone(int status) override;  // Just calls stream()->AfterWrite()
+  // Call stream()->EmitAfterWrite() and dispose of this request wrap.
+  void OnDone(int status) override;
 
  private:
   char* storage_ = nullptr;
@@ -306,13 +308,6 @@ class StreamBase : public StreamResource {
   Environment* env_;
   EmitToJSStreamListener default_listener_;
 
-  // These are called by the respective {Write,Shutdown}Wrap class.
-  void AfterShutdown(ShutdownWrap* req, int status);
-  void AfterWrite(WriteWrap* req, int status);
-
-  template <typename Wrap, typename EmitEvent>
-  void AfterRequest(Wrap* req_wrap, EmitEvent emit);
-
   friend class WriteWrap;
   friend class ShutdownWrap;
 };
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index 1cc5478bb5..cddef66c44 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -220,6 +220,8 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) {
   SSL* ssl = const_cast<SSL*>(ssl_);
   TLSWrap* c = static_cast<TLSWrap*>(SSL_get_app_data(ssl));
   Environment* env = c->env();
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
   Local<Object> object = c->object();
 
   if (where & SSL_CB_HANDSHAKE_START) {
@@ -289,6 +291,8 @@ void TLSWrap::EncOut() {
   NODE_COUNT_NET_BYTES_SENT(write_size_);
 
   if (!res.async) {
+    HandleScope handle_scope(env()->isolate());
+
     // Simulate asynchronous finishing, TLS cannot handle this at the moment.
     env()->SetImmediate([](Environment* env, void* data) {
       static_cast<TLSWrap*>(data)->OnStreamAfterWrite(nullptr, 0);
@@ -427,6 +431,7 @@ void TLSWrap::ClearOut() {
   // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0.
   // See node#1642 and SSL_read(3SSL) for details.
   if (read <= 0) {
+    HandleScope handle_scope(env()->isolate());
     int err;
     Local<Value> arg = GetSSLError(read, &err, nullptr);
 
@@ -477,6 +482,9 @@ bool TLSWrap::ClearIn() {
   }
 
   // Error or partial write
+  HandleScope handle_scope(env()->isolate());
+  Context::Scope context_scope(env()->context());
+
   int err;
   std::string error_str;
   Local<Value> arg = GetSSLError(written, &err, &error_str);
@@ -814,6 +822,9 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
   if (servername == nullptr)
     return SSL_TLSEXT_ERR_OK;
 
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
+
   // Call the SNI callback and use its return value as context
   Local<Object> object = p->object();
   Local<Value> ctx = object->Get(env->sni_context_string());