Fix memory leak when img CBs reference img

Storing the onerror/onload callbacks in Persistent handles means they will never be GC'ed. If those functions have references to the image, then the image will never be GC'ed either. Fixes Automattic#785 Fixes Automattic#150 (for real)
zbjornson · Sep 16, 2017 · 1c525d4 · 1c525d4
1 parent 74a5302
commit 1c525d4
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 99 deletions.
diff --git a/src/Image.cc b/src/Image.cc
@@ -51,8 +51,6 @@ Image::Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target) {
   Nan::SetAccessor(proto, Nan::New("height").ToLocalChecked(), GetHeight, SetHeight);
   Nan::SetAccessor(proto, Nan::New("naturalWidth").ToLocalChecked(), GetNaturalWidth);
   Nan::SetAccessor(proto, Nan::New("naturalHeight").ToLocalChecked(), GetNaturalHeight);
-  Nan::SetAccessor(proto, Nan::New("onload").ToLocalChecked(), GetOnload, SetOnload);
-  Nan::SetAccessor(proto, Nan::New("onerror").ToLocalChecked(), GetOnerror, SetOnerror);
 #if CAIRO_VERSION_MINOR >= 10
   Nan::SetAccessor(proto, Nan::New("dataMode").ToLocalChecked(), GetDataMode, SetDataMode);
   ctor->Set(Nan::New("MODE_IMAGE").ToLocalChecked(), Nan::New<Number>(DATA_IMAGE));
@@ -73,6 +71,8 @@ NAN_METHOD(Image::New) {
   Image *img = new Image;
   img->data_mode = DATA_IMAGE;
   img->Wrap(info.This());
+  info.This()->Set(Nan::New("onload").ToLocalChecked(), Nan::Null());
+  info.This()->Set(Nan::New("onerror").ToLocalChecked(), Nan::Null());
   info.GetReturnValue().Set(info.This());
 }
 
@@ -232,9 +232,17 @@ NAN_SETTER(Image::SetSource) {
 
   // check status
   if (status) {
-    img->error(Canvas::Error(status));
+    Local<Value> onerrorFn = info.This()->Get(Nan::New("onerror").ToLocalChecked());
+    if (onerrorFn->IsFunction()) {
+      Local<Value> argv[1] = { Canvas::Error(status) };
+      onerrorFn.As<Function>()->Call(Isolate::GetCurrent()->GetCurrentContext()->Global(), 1, argv);
+    }
   } else {
     img->loaded();
+    Local<Value> onloadFn = info.This()->Get(Nan::New("onload").ToLocalChecked());
+    if (onloadFn->IsFunction()) {
+      onloadFn.As<Function>()->Call(Isolate::GetCurrent()->GetCurrentContext()->Global(), 0, NULL);
+    }
   }
 }
 
@@ -304,66 +312,6 @@ Image::readPNG(void *c, uint8_t *data, unsigned int len) {
   return CAIRO_STATUS_SUCCESS;
 }
 
-/*
- * Get onload callback.
- */
-
-NAN_GETTER(Image::GetOnload) {
-  Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
-  if (img->onload) {
-    info.GetReturnValue().Set(img->onload->GetFunction());
-  } else {
-    info.GetReturnValue().SetNull();
-  }
-}
-
-/*
- * Set onload callback.
- */
-
-NAN_SETTER(Image::SetOnload) {
-  if (value->IsFunction()) {
-    Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
-    img->onload = new Nan::Callback(value.As<Function>());
-  } else if (value->IsNull()) {
-    Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
-    if (img->onload) {
-      delete img->onload;
-    }
-    img->onload = NULL;
-  }
-}
-
-/*
- * Get onerror callback.
- */
-
-NAN_GETTER(Image::GetOnerror) {
-  Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
-  if (img->onerror) {
-    info.GetReturnValue().Set(img->onerror->GetFunction());
-  } else {
-    info.GetReturnValue().SetNull();
-  }
-}
-
-/*
- * Set onerror callback.
- */
-
-NAN_SETTER(Image::SetOnerror) {
-  if (value->IsFunction()) {
-    Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
-    img->onerror = new Nan::Callback(value.As<Function>());
-  } else if (value->IsNull()) {
-    Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
-    if (img->onerror) {
-        delete img->onerror;
-    }
-    img->onerror = NULL;
-  }
-}
-
 /*
  * Initialize a new Image.
  */
@@ -376,8 +324,6 @@ Image::Image() {
   width = height = 0;
   naturalWidth = naturalHeight = 0;
   state = DEFAULT;
-  onload = NULL;
-  onerror = NULL;
 #ifdef HAVE_RSVG
   _rsvg = NULL;
   _is_svg = false;
@@ -391,16 +337,6 @@ Image::Image() {
 
 Image::~Image() {
   clearData();
-
-  if (onerror) {
-    delete onerror;
-    onerror = NULL;
-  }
-
-  if (onload) {
-    delete onload;
-    onload = NULL;
-  }
 }
 
 /*
@@ -417,7 +353,7 @@ Image::load() {
 }
 
 /*
- * Invoke onload (when assigned) and assign dimensions.
+ * Set state, assign dimensions.
  */
 
 void
@@ -429,23 +365,6 @@ Image::loaded() {
   height = naturalHeight = cairo_image_surface_get_height(_surface);
   _data_len = naturalHeight * cairo_image_surface_get_stride(_surface);
   Nan::AdjustExternalMemory(_data_len);
-
-  if (onload != NULL) {
-    onload->Call(0, NULL);
-  }
-}
-
-/*
- * Invoke onerror (when assigned) with the given err.
- */
-
-void
-Image::error(Local<Value> err) {
-  Nan::HandleScope scope;
-  if (onerror != NULL) {
-    Local<Value> argv[1] = { err };
-    onerror->Call(1, argv);
-  }
 }
 
 /*

diff --git a/src/Image.h b/src/Image.h
@@ -40,23 +40,17 @@ class Image: public Nan::ObjectWrap {
     char *filename;
     int width, height;
     int naturalWidth, naturalHeight;
-    Nan::Callback *onload;
-    Nan::Callback *onerror;
     static Nan::Persistent<FunctionTemplate> constructor;
     static void Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target);
     static NAN_METHOD(New);
     static NAN_GETTER(GetSource);
-    static NAN_GETTER(GetOnload);
-    static NAN_GETTER(GetOnerror);
     static NAN_GETTER(GetComplete);
     static NAN_GETTER(GetWidth);
     static NAN_GETTER(GetHeight);
     static NAN_GETTER(GetNaturalWidth);
     static NAN_GETTER(GetNaturalHeight);
     static NAN_GETTER(GetDataMode);
     static NAN_SETTER(SetSource);
-    static NAN_SETTER(SetOnload);
-    static NAN_SETTER(SetOnerror);
     static NAN_SETTER(SetDataMode);
     static NAN_SETTER(SetWidth);
     static NAN_SETTER(SetHeight);