Fix accessing prototypes of Canvas, Image, Context2d #808

zbjornson · 2016-09-10T20:40:22Z

Fixes #803
Fixes #847
Fixes #76
Fixes #855
Fixes nodejs/node#15099
Fixes #568

zbjornson · 2016-09-11T03:10:58Z

Urg, no real desire to debug node 0.10 and 0.8.

The Canvas 2.0 proposal drops 0.8, at least...

zbjornson · 2016-11-22T02:25:55Z

@LinusU I just made a change to allow node v0.8 and v0.10 to skip the tests that I added in this PR. None of the existing tests fail in v0.10 so this seems harmless to 0.10 and beneficial to 0.12+. What do you think of merging this?

Jack-Works

Um... I think this fix is not work. Though it is not throw a error anymore, it will let Node exit at present.
Try the code in REPL: require('canvas').prototype
Then REPL exits.

zbjornson · 2016-12-12T19:27:28Z

I can reproduce that, but the access violation is in libcairo-2.dll, not node-canvas. I can't see enough of the call stack to tell if it's a bad usage of libcairo or an actual libcairo bug.

@chearon does your msys2 build use a newer version of cairo? I don't have the msys2 build chain setup, but it might be informative to test this branch with newer cairo.

chearon · 2016-12-12T19:52:13Z

I think the GTK.zip on here is from 2010 so the MSYS version should almost certainly be much newer. If it helps I made a wiki documenting how I got it working, although I need to update it for Pango

basvanmeurs · 2016-12-20T15:59:12Z

Although the error message seems to disappear, nodejs still exits.
If the access violation is in cairo, I wonder why the debugger braiks even on just inspecting the module root? gles2 is my own C++ module root for comparison, which works just fine.

  7 var gl = gles2.init(options);
  8 
> 9 debugger;
 10 
 11 var img = new Image();
repl
Press Ctrl + C to leave debug repl
> gles2
{ init: [Function: val], blit: [Function: val] }
> Canvas
program terminated

zbjornson · 2016-12-20T16:34:00Z

@basvanmeurs what OS are you running? gles2 doesn't link to Cairo, does it?

LinusU · 2017-05-03T21:48:27Z

@zbjornson Would you mind rebasing this on latest master as soon as I have merged #915?

Also, I think it's better to fix History.md when merging so that we don't get conflicts in the PRs, what do you think?

zbjornson · 2017-05-03T21:58:32Z

Would you mind rebasing

Done!

better to fix History.md when merging

Sure, dropped that change.

LinusU · 2017-05-03T22:00:25Z

Segmentation fault (core dumped)

Hmm 🤔 seems like something isn't quite right...

zbjornson · 2017-05-03T22:01:37Z

Boo. Gonna take me a bit longer to look into that, won't get to it today...

LinusU · 2017-05-03T22:02:16Z

No problem, I'll try and have a look if I have time as well, but the clock just turned midnight here and I need to get up to work tomorrow :)

mightyiam · 2017-08-07T19:01:22Z

It doesn't work for me from this branch. I still get a crash when inspecting.

zbjornson · 2017-08-07T19:10:35Z

I need to work on this more sometime. I think this PR is a correct change to make based on some feedback from a core node.js dev, but another change is definitely needed to fully fix the error. I came across this node.js core issue, which looks similar and might provide some hints: nodejs/node-v0.x-archive#9028

zbjornson · 2017-09-03T22:35:18Z

I think this is finally fixed, but I posed a small question to the node.js devs that would be good to know before landing.

TimothyGu · 2017-09-04T03:28:51Z

src/Canvas.cc

@@ -51,16 +52,17 @@ Canvas::Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target) {

  // Prototype
  Local<ObjectTemplate> proto = ctor->PrototypeTemplate();
+  proto->SetInternalFieldCount(1);


Hey @zbjornson, thanks for linking to this PR.

While this is just fine as a Band-Aid for the problem, I'd like to note several potential problems that would stem from this implementation. First, philosophically, the prototype object should not be an instance of the class -- that is, it shouldn't have internal slots like instances do. JavaScript got this wrong with Array.prototype and Function.prototype (Array.isArray(Array.prototype) and typeof Function.prototype === 'function' both return true), but they have rectified this problem with future additions to the language with a note in the spec:

The Array prototype object is specified to be an Array exotic object to ensure compatibility with ECMAScript code that was created prior to the ECMAScript 2015 specification.

More concretely, although accessing Canvas.prototype.type will not throw, other code fragments e.g. Object.create(Canvas.prototype).type will still throw, I believe. Even worse (but more unlikely), Object.setPrototypeOf(anotherObjectWithInternalFields, Canvas.prototype).type may cause memory issues, as the type of the object pointer wrapped in V8 object is untyped.

If a Band-Aid is all that is sought here, then please disregard my suggestions below to make this more robust. Either suggestion will unfortunately incur quite some work, so it's totally up to you regarding what's best for this project.

Write a pure JS wrapper around the JS object created in C++, and implement the property getters/setters as ordinary functions (e.g. getLength(), setLength()). This means that you would do all necessary type checking in JS, so in C++ you can feel free to assert() on predicates. The C++ wrapper would also be hidden in a symbol, so we can tell any user that get curious and call the prototype methods w/o context, well, don't try to access things you are not supposed to like hidden properties. This is generally what we do in Node.js.

Check if info.Holder() is an instance of the FunctionTemplate, and throw an error if it is not. This is what Chrome does for DOM, but it would require holding a reference to the FunctionTemplate, always. If you were to go down this route, I would put the Persistent reference to the FunctionTemplate in a std::unordered_map keyed on Isolate*.

A lot of this is based on speculation, so please let me know if you found a better solution to all of this!

Thanks so much for the detailed and helpful info!

the prototype object should not be an instance of the class -- that is, it shouldn't have internal slots like instances do

Oops, proto->SetInternalFieldCount(1) was leftover from before I added the signatures to the SetAccessors, and no value is set for the slot anyway. Will remove in a moment.

As far as the proto being an instance of the class: my understanding is that internal slots are usable for anything and have no effect on the behavior of the object (they're just C++-accessible storage). Canvas.prototype instanceof Canvas === false, so I think we're okay?

Object.create(Canvas.prototype).type will still throw, I believe. ... Object.setPrototypeOf(anotherObjectWithInternalFields, Canvas.prototype).type may cause memory issues

Both of these throw the "incompatible receiver" error (as desired; same as Canvas.prototype.type), and neither have the assertion failure.

Check if info.Holder() is an instance of the FunctionTemplate

I think this is what v8 does actually, when SetAccessor is used with an AccessorSignature:

The signature describes valid receivers for the accessor and is used to perform implicit instance checks against them. If the receiver is incompatible (i.e. is not an instance of the constructor as defined by FunctionTemplate::HasInstance()), an implicit TypeError is thrown and no callback is invoked.

Am I reading that right? If so, then I think this PR is actually a sturdy route forward.

I wasn't familiar with how AccessorSignature worked... and it looks like that's it! This looks good to me.

Thank you very much for reviewing!

zbjornson · 2017-09-07T05:36:24Z

@LinusU this should be ready to go 🚀. bnoordhuis answered my question in the linked thread, no change needed.

LinusU · 2017-09-09T21:52:45Z

Awesome!!

@zbjornson mind rebasing and fixing the conflicts? ❤️

Accessors should not attempt to unwrap the non-existent 'this' when accessed via the prototype. Fixes Automattic#803 Fixes Automattic#847 Fixes Automattic#885 Fixes nodejs/node#15099

zbjornson · 2017-09-11T07:14:20Z

Done! :)

LinusU · 2017-09-11T07:56:55Z

Super!

Jack-Works · 2017-09-11T07:58:38Z

Wait for 1 year! 🤣🤣

LinusU · 2017-09-11T08:07:11Z

...and one day 😆

LinusU mentioned this pull request Oct 30, 2016

Is it time to drop Node.js 0.10 support (in 2.x) #832

Closed

zbjornson mentioned this pull request Nov 19, 2016

C++ Runtime Error when debuging #847

Closed

zbjornson force-pushed the prototype-fix branch 2 times, most recently from 50ef012 to fb60ca1 Compare November 22, 2016 02:22

zbjornson mentioned this pull request Dec 6, 2016

Expression: object->InternalFieldCount() > 0 #855

Closed

Jack-Works suggested changes Dec 12, 2016

View reviewed changes

zbjornson mentioned this pull request Dec 20, 2016

.prototype segfaults #76

Closed

LinusU added the Changes requested label May 3, 2017

zbjornson force-pushed the prototype-fix branch from fb60ca1 to 5e71742 Compare May 3, 2017 21:58

zbjornson force-pushed the prototype-fix branch 2 times, most recently from a8f4289 to bdec7a1 Compare September 3, 2017 22:30

zbjornson mentioned this pull request Sep 3, 2017

Breakpoint after use of node-canvas lets node crash nodejs/node#15099

Closed

TimothyGu reviewed Sep 4, 2017

View reviewed changes

zbjornson force-pushed the prototype-fix branch from bdec7a1 to e16303e Compare September 4, 2017 18:31

Fix assertion failure from accessors

d4b6f67

Accessors should not attempt to unwrap the non-existent 'this' when accessed via the prototype. Fixes Automattic#803 Fixes Automattic#847 Fixes Automattic#885 Fixes nodejs/node#15099

zbjornson force-pushed the prototype-fix branch from e16303e to d4b6f67 Compare September 11, 2017 07:10

LinusU merged commit 0987357 into Automattic:master Sep 11, 2017

zbjornson deleted the prototype-fix branch October 12, 2017 18:50

zbjornson restored the prototype-fix branch October 12, 2017 18:50

zbjornson deleted the prototype-fix branch April 21, 2018 07:12

LinusU mentioned this pull request Jul 31, 2018

Inspecting canvas object in node-inspector crashes node #258

Closed

zbjornson mentioned this pull request Oct 5, 2019

Object.getOwnPropertyDescriptor(CanvasRenderingContext2D.prototype, "strokeStyle"); #1460

Open

Ayase-252 mentioned this pull request Aug 27, 2021

Error [ERR_INTERNAL_ASSERTION]: TypeError: Method height called on incompatible receiver #<Canvas> nodejs/node#39866

Closed

Fix accessing prototypes of Canvas, Image, Context2d #808

Fix accessing prototypes of Canvas, Image, Context2d #808

Uh oh!

Conversation

zbjornson commented Sep 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zbjornson commented Sep 11, 2016

Uh oh!

zbjornson commented Nov 22, 2016

Uh oh!

Jack-Works left a comment

Choose a reason for hiding this comment

Uh oh!

zbjornson commented Dec 12, 2016

Uh oh!

chearon commented Dec 12, 2016

Uh oh!

basvanmeurs commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zbjornson commented Dec 20, 2016

Uh oh!

LinusU commented May 3, 2017

Uh oh!

zbjornson commented May 3, 2017

Uh oh!

LinusU commented May 3, 2017

Uh oh!

zbjornson commented May 3, 2017

Uh oh!

LinusU commented May 3, 2017

Uh oh!

mightyiam commented Aug 7, 2017

Uh oh!

zbjornson commented Aug 7, 2017

Uh oh!

zbjornson commented Sep 3, 2017

Uh oh!

TimothyGu Sep 4, 2017

Choose a reason for hiding this comment

Uh oh!

zbjornson Sep 4, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

zbjornson Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

zbjornson commented Sep 7, 2017

Uh oh!

LinusU commented Sep 9, 2017

Uh oh!

zbjornson commented Sep 11, 2017

Uh oh!

LinusU commented Sep 11, 2017

Uh oh!

Jack-Works commented Sep 11, 2017

Uh oh!

LinusU commented Sep 11, 2017

Uh oh!

Uh oh!

zbjornson commented Sep 10, 2016 •

edited

Loading

basvanmeurs commented Dec 20, 2016 •

edited

Loading