Issue: Crash when access Canvas.prototype

[Jack-Works]

var Canvas = require('canvas');
Canvas.prototype // Crash here

Node-canvas 1.4.0 on Node JS 5.12.0 on Windows 10 (1607)

[zbjornson]

Reproduced, Win 10 and Node 6.3.1, fix in works.

[zbjornson]

This patch fixes the problem, but I'm not sure from the v8 docs if this is the right thing to do, nor am I sure why it fails on the proto. @kkoopa, @LinusU or @rvagg do you know if this is correct?

(nb: proto in this diff is Local<ObjectTemplate> proto = ctor->PrototypeTemplate())

diff --git a/src/Canvas.cc b/src/Canvas.cc
index 83de51e..4427a57 100644
--- a/src/Canvas.cc
+++ b/src/Canvas.cc
@@ -43,9 +43,10 @@ Canvas::Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target) {
 #ifdef HAVE_JPEG
   Nan::SetPrototypeMethod(ctor, "streamJPEGSync", StreamJPEGSync);
 #endif
-  Nan::SetAccessor(proto, Nan::New("type").ToLocalChecked(), GetType);
-  Nan::SetAccessor(proto, Nan::New("width").ToLocalChecked(), GetWidth, SetWidth);
-  Nan::SetAccessor(proto, Nan::New("height").ToLocalChecked(), GetHeight, SetHeight);
+  Local<ObjectTemplate> instance_t = ctor->InstanceTemplate();
+  Nan::SetAccessor(instance_t, Nan::New("type").ToLocalChecked(), GetType);
+  Nan::SetAccessor(instance_t, Nan::New("width").ToLocalChecked(), GetWidth, SetWidth);
+  Nan::SetAccessor(instance_t, Nan::New("height").ToLocalChecked(), GetHeight, SetHeight);

   Nan::SetTemplate(proto, "PNG_NO_FILTERS", Nan::New<Uint32>(PNG_NO_FILTERS));
   Nan::SetTemplate(proto, "PNG_FILTER_NONE", Nan::New<Uint32>(PNG_FILTER_NONE));
diff --git a/test/canvas.test.js b/test/canvas.test.js
index ad62344..c5f82c6 100644
--- a/test/canvas.test.js
+++ b/test/canvas.test.js
@@ -786,4 +786,8 @@ describe('Canvas', function () {
       done(err);
     });
   });
+
+  it('Allows accessing the prototype on the constructor', function () {
+    Canvas.prototype.type;
+  });
 });

Note that this same error happens for the other objects including Image, so the fix needs to be applied everywhere.

[kkoopa]

I am not entirely sure, but remember there being some difference between accessors on instance templates and prototype templates.

[zbjornson]

My patch is definitely wrong because it removes the accessors from the prototype, and browsers have them on the prototype. This diff has three better assertions and a one-line fix to make them all pass, but still not sure if it's correct:

--- a/src/Canvas.cc
+++ b/src/Canvas.cc
@@ -38,6 +38,7 @@ Canvas::Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target) {

   // Prototype
   Local<ObjectTemplate> proto = ctor->PrototypeTemplate();
+  proto->SetInternalFieldCount(1);
   Nan::SetPrototypeMethod(ctor, "toBuffer", ToBuffer);
   Nan::SetPrototypeMethod(ctor, "streamPNGSync", StreamPNGSync);
 #ifdef HAVE_JPEG
diff --git a/test/canvas.test.js b/test/canvas.test.js
index ad62344..30a67c3 100644
--- a/test/canvas.test.js
+++ b/test/canvas.test.js
@@ -786,4 +786,11 @@ describe('Canvas', function () {
       done(err);
     });
   });
+
+  it.only('Allows accessing the prototype on the constructor', function () {
+    var c = new Canvas(10, 10);
+    assert(!c.hasOwnProperty("width"));
+    assert("width" in c);
+    assert(Canvas.prototype.hasOwnProperty("width"));
+  });
 });

That means the InternalFieldCount is set both on the InstanceTemplate and the PrototypeTemplate.

Edit 4-Sep-2017: This is mostly wrong. :) See PR for proper fix.

[zbjornson]

Ben Noordhuis confirmed that the above patch is reasonable: https://groups.google.com/d/msg/v8-users/Rf9JDgPbGsw/-W2s0Mz3AgAJ

Will make a PR soon.