Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/Node.js 18 -> Assertion failed: (object->InternalFieldCount() > 0) #2133

Merged
merged 12 commits into from
Oct 30, 2022

Conversation

TheDadi
Copy link
Contributor

@TheDadi TheDadi commented Oct 11, 2022

This PR fixes the error Assertion failed: (object->InternalFieldCount() > 0) when building with node-canvas with a Node.js version > 18

  • Have you updated CHANGELOG.md?

@TheDadi TheDadi changed the title Bugfix/Assertion failed: (object->InternalFieldCount() > 0) Bugfix/Node.js 18 -> Assertion failed: (object->InternalFieldCount() > 0) Oct 11, 2022
src/Canvas.cc Outdated Show resolved Hide resolved
@TheDadi
Copy link
Contributor Author

TheDadi commented Oct 12, 2022

@zbjornson I reimplemented everything on the prototype and added some custom checks if the current this is a valid instance of the given constructor since the way you used to prevent the invoke of the accessor on an invalid this, with passing the AccessorSignature to the Nan:SetAccessor is deprecated and was removed in NodeJS 18 as you can see in this commit nodejs/nan@7f9ceb8. https://github.com/nodejs/nan/blob/7f9ceb80fbc45c9ba1a10e6591ccbef9e8dee6b4/nan.h#L2553

@zbjornson
Copy link
Collaborator

What a pain to have to do this in every method 🥴 but this looks correct, thank you! Will review closely soon.

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read through the V8 change and this indeed seems correct.

Looks good but we can save a lot of lines with a macro (see below). I'm hoping to work on prebuilds this weekend since v18 is becoming LTS soon. If you don't have time, I might make this style change tonight (Pacific) or tomorrow.

Thanks again!

Comment on lines +147 to +150
if (!Canvas::constructor.Get(info.GetIsolate())->HasInstance(info.This())) {
Nan::ThrowTypeError("Method Canvas.GetType called on incompatible receiver");
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can save quite a few lines of code by doing:

#define CHECK_RECEIVER(prop) \
  if (!Canvas::constructor.Get(info.GetIsolate())->HasInstance(info.This())) {
    Nan::ThrowTypeError("Method " #prop " called on incompatible receiver");
    return;
  }
NAN_GETTER(Canvas::GetType) {
  CHECK_RECEIVER(GetType)
}

package.json Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@zbjornson zbjornson mentioned this pull request Oct 22, 2022
1 task
@joeyguerra
Copy link

Looking forward for this.

@SavageFRVR

This comment was marked as off-topic.

@Sophan-Developer

This comment was marked as off-topic.

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again! I'll do the macro change after merging.

@zbjornson zbjornson merged commit 2876b6e into Automattic:master Oct 30, 2022
phipla pushed a commit to e-cervo/node-canvas that referenced this pull request Sep 7, 2023
* commit '4c276e07f570ee6a727f88dcc67251823895c671':
  fix incorrect text width with newer (1.43?) Pango
  fix macos CI
  Add canvas property to CanvasRenderingContext2D type
  v2.11.0
  use tailored types instead of extending DOM
  v2.10.2
  src: shorten receiver checks
  src: shorten copy assignment operator decl for Point
  Bugfix/Node.js 18 -> Assertion failed: (object->InternalFieldCount() > 0) (Automattic#2133)

# Conflicts:
#	package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants