zlib: fix memory leak for invalid input

Don’t toggle the weak/strong reference flag from the error
handler, that’s too confusing. Instead, always do it in the
code that handles the write call.

Fixes: #22705

PR-URL: #22713
Reviewed-By: James M Snell <[email protected]>

src/node_zlib.cc

@@ -215,8 +215,8 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork {
         ctx->write_result_[0] = ctx->strm_.avail_out;
         ctx->write_result_[1] = ctx->strm_.avail_in;
         ctx->write_in_progress_ = false;
-        ctx->Unref();
       }
+      ctx->Unref();
       return;
     }
 
@@ -364,6 +364,7 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork {
   // v8 land!
   void AfterThreadPoolWork(int status) override {
     AllocScope alloc_scope(this);
+    OnScopeLeave on_scope_leave([&]() { Unref(); });
 
     write_in_progress_ = false;
 
@@ -388,7 +389,6 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork {
                                            write_js_callback_);
     MakeCallback(cb, 0, nullptr);
 
-    Unref();
     if (pending_close_)
       Close();
   }
@@ -410,8 +410,6 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork {
     MakeCallback(env()->onerror_string(), arraysize(args), args);
 
     // no hope of rescue.
-    if (write_in_progress_)
-      Unref();
     write_in_progress_ = false;
     if (pending_close_)
       Close();

test/parallel/test-zlib-invalid-input-memory.js

@@ -0,0 +1,28 @@
+// Flags: --expose-gc
+'use strict';
+const common = require('../common');
+const onGC = require('../common/ongc');
+const assert = require('assert');
+const zlib = require('zlib');
+
+// Checks that, if a zlib context fails with an error, it can still be GC'ed:
+// Refs: https://github.com/nodejs/node/issues/22705
+
+const ongc = common.mustCall();
+
+{
+  const input = Buffer.from('foobar');
+  const strm = zlib.createInflate();
+  strm.end(input);
+  strm.once('error', common.mustCall((err) => {
+    assert(err);
+    setImmediate(() => {
+      global.gc();
+      // Keep the event loop alive for seeing the async_hooks destroy hook
+      // we use for GC tracking...
+      // TODO(addaleax): This should maybe not be necessary?
+      setImmediate(() => {});
+    });
+  }));
+  onGC(strm, { ongc });
+}