Bump node-addon-api to fix memory leak #2436

CHANGELOG.md

@@ -8,8 +8,10 @@ project adheres to [Semantic Versioning](http://semver.org/).
 (Unreleased)
 ==================
 ### Changed
+* Upgrade node-addon-api from 7.x to 8.x
 ### Added
 ### Fixed
+* Fix memory leak caused by N-API weak reference callbacks being deferred to SetImmediate instead of running during GC. Enabled `NAPI_EXPERIMENTAL` to use `node_api_nogc_finalize` for ObjectWrap destructor. (#2436)
 
 3.2.3
 ==================

binding.gyp

@@ -58,7 +58,7 @@
     {
       'target_name': 'canvas',
       'include_dirs': ["<!(node -p \"require('node-addon-api').include_dir\")"],
-      'defines': [ 'NAPI_DISABLE_CPP_EXCEPTIONS', 'NODE_ADDON_API_ENABLE_MAYBE' ],
+      'defines': [ 'NAPI_DISABLE_CPP_EXCEPTIONS', 'NODE_ADDON_API_ENABLE_MAYBE', 'NAPI_EXPERIMENTAL' ],
       'sources': [
         'src/bmp/BMPParser.cc',
         'src/Canvas.cc',

package.json

@@ -27,7 +27,7 @@
     "prebenchmark": "node-gyp build",
     "benchmark": "node benchmarks/run.js",
     "lint": "standard examples/*.js test/server.js test/public/*.js benchmarks/run.js lib/context2d.js util/has_lib.js browser.js index.js",
-    "test": "mocha test/*.test.js",
+    "test": "mocha --node-option expose-gc test/*.test.js",
     "pretest-server": "node-gyp build",
     "test-server": "node test/server.js",
     "generate-wpt": "node ./test/wpt/generate.js",
@@ -45,7 +45,7 @@
     "util/"
   ],
   "dependencies": {
-    "node-addon-api": "^7.0.0",
+    "node-addon-api": "^8.7.0",
     "prebuild-install": "^7.1.3"
   },
   "devDependencies": {

src/Canvas.cc

@@ -991,11 +991,11 @@ Canvas::ensureSurface() {
 void
 Canvas::destroySurface() {
   if (_surface) {
-    // flush any operations that may use the closure that is freed below
-    cairo_surface_finish(_surface);
     if (type == CANVAS_TYPE_IMAGE) {
       Napi::MemoryManagement::AdjustExternalMemory(env, -(int64_t)approxBytesPerPixel() * width * height);
     }
+    // flush any operations that may use the closure that is freed below
+    cairo_surface_finish(_surface);
     cairo_surface_destroy(_surface);
     _surface = nullptr;
   }

src/CanvasPattern.cc

@@ -64,6 +64,7 @@ Pattern::Pattern(const Napi::CallbackInfo& info) : ObjectWrap<Pattern>(info), en
     return;
   }
   _pattern = cairo_pattern_create_for_surface(surface);
+  _source = Napi::Persistent(obj);
 
   if (info[1].IsString()) {
     if ("no-repeat" == info[1].As<Napi::String>().Utf8Value()) {
@@ -127,3 +128,7 @@ repeat_type_t Pattern::get_repeat_type_for_cairo_pattern(cairo_pattern_t *patter
 Pattern::~Pattern() {
   if (_pattern) cairo_pattern_destroy(_pattern);
 }
+
+void Pattern::Finalize(Napi::Env env) {
+  _source.Reset();
+}

src/CanvasPattern.h

@@ -26,8 +26,10 @@ class Pattern : public Napi::ObjectWrap<Pattern> {
     static repeat_type_t get_repeat_type_for_cairo_pattern(cairo_pattern_t *pattern);
     inline cairo_pattern_t *pattern(){ return _pattern; }
     ~Pattern();
+    void Finalize(Napi::Env env);
     Napi::Env env;
   private:
     cairo_pattern_t *_pattern;
+    Napi::Reference<Napi::Object> _source;
     repeat_type_t _repeat = REPEAT;
 };

src/CanvasRenderingContext2d.cc

@@ -244,6 +244,9 @@ Context2d::Context2d(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Context2
 Context2d::~Context2d() {
   if (_layout) g_object_unref(_layout);
   if (_context) cairo_destroy(_context);
+}
+
+void Context2d::Finalize(Napi::Env env) {
   _resetPersistentHandles();
 }
 

src/CanvasRenderingContext2d.h

@@ -214,6 +214,7 @@ class Context2d : public Napi::ObjectWrap<Context2d> {
     void resetState();
     inline PangoLayout *layout(){ return _layout; }
     ~Context2d();
+    void Finalize(Napi::Env env);
     Napi::Env env;
 
   private:

test/memory.test.js

@@ -0,0 +1,36 @@
+/* eslint-env mocha */
+
+'use strict'
+
+// These tests require --expose-gc. Skip gracefully if not available.
+const gcAvailable = typeof gc === 'function'
+
+const assert = require('assert')
+const { createCanvas } = require('../')
+
+describe('Memory management', function () {
+  before(function () {
+    if (!gcAvailable) this.skip()
+  })
+
+  it('Canvas objects are freed by GC', function () {
+    const ITERATIONS = 100
+    const SIZE = 1024 // 1024x1024 ARGB = 4 MiB per canvas
+
+    for (let i = 0; i < ITERATIONS; i++) {
+      const canvas = createCanvas(SIZE, SIZE)
+      const ctx = canvas.getContext('2d')
+      ctx.fillStyle = 'red'
+      ctx.fillRect(0, 0, SIZE, SIZE)
+      gc()
+    }
+
+    gc()
+    gc()
+
+    // 100 canvases x 4 MiB = 400 MiB if leaked.
+    // With proper GC, RSS should stay well under 150 MiB.
+    const rssMiB = process.memoryUsage().rss / 1024 / 1024
+    assert(rssMiB < 150, `RSS is ${rssMiB.toFixed(0)} MiB, expected < 150 MiB`)
+  })
+})