http2: refactor ping + settings object lifetime management

Have clearer ownership relations between the `Http2Ping`,
`Http2Settings` and `Http2Session` objects.

Ping and Settings objects are now owned by the `Http2Session`
instance, and deleted along with it, so neither type of object
refers to the session after it is gone.
In the case of `Http2Ping`s, that deletion is slightly delayed,
so we explicitly reset its `session_` property.

Fixes: https://github.com/nodejs/node/issues/28088

PR-URL: https://github.com/nodejs/node/pull/28150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

5 files changed, 111 insertions, 55 deletions

diff --git a/src/base_object-inl.h b/src/base_object-inl.h
index 50d8aba4a9..af69084f4a 100644
--- a/src/base_object-inl.h
+++ b/src/base_object-inl.h
@@ -40,9 +40,8 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
   env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
 }
 
-
 BaseObject::~BaseObject() {
-  env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
+  RemoveCleanupHook();
 
   if (persistent_handle_.IsEmpty()) {
     // This most likely happened because the weak callback below cleared it.
@@ -55,6 +54,9 @@ BaseObject::~BaseObject() {
   }
 }
 
+void BaseObject::RemoveCleanupHook() {
+  env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
+}
 
 v8::Global<v8::Object>& BaseObject::persistent() {
   return persistent_handle_;
diff --git a/src/base_object.h b/src/base_object.h
index f616108a1d..53bd4b00d6 100644
--- a/src/base_object.h
+++ b/src/base_object.h
@@ -83,6 +83,12 @@ class BaseObject : public MemoryRetainer {
                                v8::Local<v8::Value> value,
                                const v8::PropertyCallbackInfo<void>& info);
 
+ protected:
+  // Can be used to avoid the automatic object deletion when the Environment
+  // exits, for example when this object is owned and deleted by another
+  // BaseObject at that point.
+  inline void RemoveCleanupHook();
+
  private:
   v8::Local<v8::Object> WrappedObject() const override;
   static void DeleteMe(void* data);
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 3a7591f31a..9047171e11 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -237,6 +237,7 @@ Http2Session::Http2Settings::Http2Settings(Environment* env,
     : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS),
       session_(session),
       startTime_(start_time) {
+  RemoveCleanupHook();  // This object is owned by the Http2Session.
   Init();
 }
 
@@ -311,12 +312,11 @@ void Http2Session::Http2Settings::Done(bool ack) {
   uint64_t end = uv_hrtime();
   double duration = (end - startTime_) / 1e6;
 
-  Local<Value> argv[2] = {
+  Local<Value> argv[] = {
     Boolean::New(env()->isolate(), ack),
     Number::New(env()->isolate(), duration)
   };
   MakeCallback(env()->ondone_string(), arraysize(argv), argv);
-  delete this;
 }
 
 // The Http2Priority class initializes an appropriate nghttp2_priority_spec
@@ -746,11 +746,14 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
   // If there are outstanding pings, those will need to be canceled, do
   // so on the next iteration of the event loop to avoid calling out into
   // javascript since this may be called during garbage collection.
-  while (!outstanding_pings_.empty()) {
-    Http2Session::Http2Ping* ping = PopPing();
-    env()->SetImmediate([](Environment* env, void* data) {
-      static_cast<Http2Session::Http2Ping*>(data)->Done(false);
-    }, static_cast<void*>(ping));
+  while (std::unique_ptr<Http2Ping> ping = PopPing()) {
+    ping->DetachFromSession();
+    env()->SetImmediate(
+        [](Environment* env, void* data) {
+          std::unique_ptr<Http2Ping> ping{static_cast<Http2Ping*>(data)};
+          ping->Done(false);
+        },
+        static_cast<void*>(ping.release()));
   }
 
   statistics_.end_time = uv_hrtime();
@@ -1435,9 +1438,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
   Local<Value> arg;
   bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
   if (ack) {
-    Http2Ping* ping = PopPing();
+    std::unique_ptr<Http2Ping> ping = PopPing();
 
-    if (ping == nullptr) {
+    if (!ping) {
       // PING Ack is unsolicited. Treat as a connection error. The HTTP/2
       // spec does not require this, but there is no legitimate reason to
       // receive an unsolicited PING ack on a connection. Either the peer
@@ -1470,8 +1473,8 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
 
   // If this is an acknowledgement, we should have an Http2Settings
   // object for it.
-  Http2Settings* settings = PopSettings();
-  if (settings != nullptr) {
+  std::unique_ptr<Http2Settings> settings = PopSettings();
+  if (settings) {
     settings->Done(true);
     return;
   }
@@ -2775,15 +2778,12 @@ void Http2Session::Ping(const FunctionCallbackInfo<Value>& args) {
   }
   if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing())
     return;
-  Http2Session::Http2Ping* ping = new Http2Ping(session, obj);
 
+  Http2Ping* ping = session->AddPing(std::make_unique<Http2Ping>(session, obj));
   // To prevent abuse, we strictly limit the number of unacknowledged PING
   // frames that may be sent at any given time. This is configurable in the
   // Options when creating a Http2Session.
-  if (!session->AddPing(ping)) {
-    ping->Done(false);
-    return args.GetReturnValue().Set(false);
-  }
+  if (ping == nullptr) return args.GetReturnValue().Set(false);
 
   // The Ping itself is an Async resource. When the acknowledgement is received,
   // the callback will be invoked and a notification sent out to JS land. The
@@ -2808,60 +2808,67 @@ void Http2Session::Settings(const FunctionCallbackInfo<Value>& args) {
   if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing())
     return;
 
-  Http2Session::Http2Settings* settings =
-      new Http2Settings(session->env(), session, obj, 0);
-  if (!session->AddSettings(settings)) {
-    settings->Done(false);
-    return args.GetReturnValue().Set(false);
-  }
+  Http2Session::Http2Settings* settings = session->AddSettings(
+      std::make_unique<Http2Settings>(session->env(), session, obj, 0));
+  if (settings == nullptr) return args.GetReturnValue().Set(false);
 
   settings->Send();
   args.GetReturnValue().Set(true);
 }
 
-
-Http2Session::Http2Ping* Http2Session::PopPing() {
-  Http2Ping* ping = nullptr;
+std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
+  std::unique_ptr<Http2Ping> ping;
   if (!outstanding_pings_.empty()) {
-    ping = outstanding_pings_.front();
+    ping = std::move(outstanding_pings_.front());
     outstanding_pings_.pop();
     DecrementCurrentSessionMemory(sizeof(*ping));
   }
   return ping;
 }
 
-bool Http2Session::AddPing(Http2Session::Http2Ping* ping) {
-  if (outstanding_pings_.size() == max_outstanding_pings_)
-    return false;
-  outstanding_pings_.push(ping);
+Http2Session::Http2Ping* Http2Session::AddPing(
+    std::unique_ptr<Http2Session::Http2Ping> ping) {
+  if (outstanding_pings_.size() == max_outstanding_pings_) {
+    ping->Done(false);
+    return nullptr;
+  }
+  Http2Ping* ptr = ping.get();
+  outstanding_pings_.emplace(std::move(ping));
   IncrementCurrentSessionMemory(sizeof(*ping));
-  return true;
+  return ptr;
 }
 
-Http2Session::Http2Settings* Http2Session::PopSettings() {
-  Http2Settings* settings = nullptr;
+std::unique_ptr<Http2Session::Http2Settings> Http2Session::PopSettings() {
+  std::unique_ptr<Http2Settings> settings;
   if (!outstanding_settings_.empty()) {
-    settings = outstanding_settings_.front();
+    settings = std::move(outstanding_settings_.front());
     outstanding_settings_.pop();
     DecrementCurrentSessionMemory(sizeof(*settings));
   }
   return settings;
 }
 
-bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) {
-  if (outstanding_settings_.size() == max_outstanding_settings_)
-    return false;
-  outstanding_settings_.push(settings);
+Http2Session::Http2Settings* Http2Session::AddSettings(
+    std::unique_ptr<Http2Session::Http2Settings> settings) {
+  if (outstanding_settings_.size() == max_outstanding_settings_) {
+    settings->Done(false);
+    return nullptr;
+  }
+  Http2Settings* ptr = settings.get();
+  outstanding_settings_.emplace(std::move(settings));
   IncrementCurrentSessionMemory(sizeof(*settings));
-  return true;
+  return ptr;
 }
 
 Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local<Object> obj)
     : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING),
       session_(session),
-      startTime_(uv_hrtime()) {}
+      startTime_(uv_hrtime()) {
+  RemoveCleanupHook();  // This object is owned by the Http2Session.
+}
 
 void Http2Session::Http2Ping::Send(const uint8_t* payload) {
+  CHECK_NOT_NULL(session_);
   uint8_t data[8];
   if (payload == nullptr) {
     memcpy(&data, &startTime_, arraysize(data));
@@ -2872,8 +2879,12 @@ void Http2Session::Http2Ping::Send(const uint8_t* payload) {
 }
 
 void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) {
-  session_->statistics_.ping_rtt = uv_hrtime() - startTime_;
-  double duration = session_->statistics_.ping_rtt / 1e6;
+  uint64_t duration_ns = uv_hrtime() - startTime_;
+  double duration_ms = duration_ns / 1e6;
+  if (session_ != nullptr) session_->statistics_.ping_rtt = duration_ns;
+
+  HandleScope handle_scope(env()->isolate());
+  Context::Scope context_scope(env()->context());
 
   Local<Value> buf = Undefined(env()->isolate());
   if (payload != nullptr) {
@@ -2882,15 +2893,17 @@ void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) {
                        8).ToLocalChecked();
   }
 
-  Local<Value> argv[3] = {
+  Local<Value> argv[] = {
     Boolean::New(env()->isolate(), ack),
-    Number::New(env()->isolate(), duration),
+    Number::New(env()->isolate(), duration_ms),
     buf
   };
   MakeCallback(env()->ondone_string(), arraysize(argv), argv);
-  delete this;
 }
 
+void Http2Session::Http2Ping::DetachFromSession() {
+  session_ = nullptr;
+}
 
 void nghttp2_stream_write::MemoryInfo(MemoryTracker* tracker) const {
   if (req_wrap != nullptr)
diff --git a/src/node_http2.h b/src/node_http2.h
index c8ffa69f2e..d9636628c2 100644
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -803,11 +803,11 @@ class Http2Session : public AsyncWrap, public StreamListener {
     return env()->event_loop();
   }
 
-  Http2Ping* PopPing();
-  bool AddPing(Http2Ping* ping);
+  std::unique_ptr<Http2Ping> PopPing();
+  Http2Ping* AddPing(std::unique_ptr<Http2Ping> ping);
 
-  Http2Settings* PopSettings();
-  bool AddSettings(Http2Settings* settings);
+  std::unique_ptr<Http2Settings> PopSettings();
+  Http2Settings* AddSettings(std::unique_ptr<Http2Settings> settings);
 
   void IncrementCurrentSessionMemory(uint64_t amount) {
     current_session_memory_ += amount;
@@ -976,10 +976,10 @@ class Http2Session : public AsyncWrap, public StreamListener {
   v8::Local<v8::ArrayBuffer> stream_buf_ab_;
 
   size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
-  std::queue<Http2Ping*> outstanding_pings_;
+  std::queue<std::unique_ptr<Http2Ping>> outstanding_pings_;
 
   size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS;
-  std::queue<Http2Settings*> outstanding_settings_;
+  std::queue<std::unique_ptr<Http2Settings>> outstanding_settings_;
 
   std::vector<nghttp2_stream_write> outgoing_buffers_;
   std::vector<uint8_t> outgoing_storage_;
@@ -1086,12 +1086,11 @@ class Http2Session::Http2Ping : public AsyncWrap {
 
   void Send(const uint8_t* payload);
   void Done(bool ack, const uint8_t* payload = nullptr);
+  void DetachFromSession();
 
  private:
   Http2Session* session_;
   uint64_t startTime_;
-
-  friend class Http2Session;
 };
 
 // The Http2Settings class is used to parse the settings passed in for
diff --git a/test/parallel/test-http2-ping-settings-heapdump.js b/test/parallel/test-http2-ping-settings-heapdump.js
new file mode 100644
index 0000000000..78b3c8cd74
--- /dev/null
+++ b/test/parallel/test-http2-ping-settings-heapdump.js
@@ -0,0 +1,36 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const http2 = require('http2');
+const v8 = require('v8');
+
+// Regression test for https://github.com/nodejs/node/issues/28088:
+// Verify that Http2Settings and Http2Ping objects don't reference the session
+// after it is destroyed, either because they are detached from it or have been
+// destroyed themselves.
+
+for (const variant of ['ping', 'settings']) {
+  const server = http2.createServer();
+  server.on('session', common.mustCall((session) => {
+    if (variant === 'ping') {
+      session.ping(common.expectsError({
+        code: 'ERR_HTTP2_PING_CANCEL'
+      }));
+    } else {
+      session.settings(undefined, common.mustNotCall());
+    }
+
+    session.on('close', common.mustCall(() => {
+      v8.getHeapSnapshot().resume();
+      server.close();
+    }));
+    session.destroy();
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    http2.connect(`http://localhost:${server.address().port}`,
+                  common.mustCall());
+  }));
+}