http2: fix memory leak caused by premature listener removing

Http2Session should always call ondone into JS to detach the
handle. In some case, ondone is defered to be called by the
StreamListener through WriteWrap, we should be careful of this
before getting rid of the StreamListener.

PR-URL: #55966
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>

src/node_http2.cc

@@ -803,13 +803,15 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
     CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0);
     SendPendingData();
   } else if (stream_ != nullptr) {
+    // so that the previous listener of the socket, typically, JS code of a
+    // (tls) socket will be notified of any activity later
     stream_->RemoveStreamListener(this);
   }
 
   set_destroyed();
 
   // If we are writing we will get to make the callback in OnStreamAfterWrite.
-  if (!is_write_in_progress()) {
+  if (!is_write_in_progress() || !stream_) {
     Debug(this, "make done session callback");
     HandleScope scope(env()->isolate());
     MakeCallback(env()->ondone_string(), 0, nullptr);

test/parallel/test-h2leak-destroy-session-on-socket-ended.js

@@ -0,0 +1,80 @@
+'use strict';
+// Flags: --expose-gc
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+
+const registry = new FinalizationRegistry(common.mustCall((name) => {
+  assert(name, 'session');
+}));
+
+const server = http2.createSecureServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+});
+
+let firstServerStream;
+
+
+server.on('secureConnection', (s) => {
+  console.log('secureConnection');
+  s.on('end', () => {
+    console.log(s.destroyed); // false !!
+    s.destroy();
+    firstServerStream.session.destroy();
+
+    firstServerStream = null;
+
+    setImmediate(() => {
+      global.gc();
+      global.gc();
+
+      server.close();
+    });
+  });
+});
+
+server.on('session', (s) => {
+  registry.register(s, 'session');
+});
+
+server.on('stream', (stream) => {
+  console.log('stream...');
+  stream.write('a'.repeat(1024));
+  firstServerStream = stream;
+  setImmediate(() => console.log('Draining setImmediate after writing'));
+});
+
+
+server.listen(() => {
+  client();
+});
+
+
+const h2fstStream = [
+  'UFJJICogSFRUUC8yLjANCg0KU00NCg0K',
+  // http message (1st stream:)
+  'AAAABAAAAAAA',
+  'AAAPAQUAAAABhIJBiqDkHROdCbjwHgeG',
+];
+function client() {
+  const client = tls.connect({
+    port: server.address().port,
+    host: 'localhost',
+    rejectUnauthorized: false,
+    ALPNProtocols: ['h2']
+  }, () => {
+    client.end(Buffer.concat(h2fstStream.map((s) => Buffer.from(s, 'base64'))), (err) => {
+      assert.ifError(err);
+    });
+  });
+
+  client.on('error', (error) => {
+    console.error('Connection error:', error);
+  });
+}