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]>

Author: ywave620

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();
  } elseif (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
+
+constcommon=require('../common');
+if(!common.hasCrypto)
+common.skip('missing crypto');
+consthttp2=require('http2');
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+constassert=require('assert');
+
+constregistry=newFinalizationRegistry(common.mustCall((name)=>{
+assert(name,'session');
+}));
+
+constserver=http2.createSecureServer({
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+});
+
+letfirstServerStream;
+
+
+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();
+});
+
+
+consth2fstStream=[
+'UFJJICogSFRUUC8yLjANCg0KU00NCg0K',
+// http message (1st stream:)
+'AAAABAAAAAAA',
+'AAAPAQUAAAABhIJBiqDkHROdCbjwHgeG',
+];
+functionclient(){
+constclient=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);
+});
+}