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

Pass Holder instead of This to ObjectWrap::Unwrap #524

Merged
merged 4 commits into from
Dec 14, 2015

Conversation

gagern
Copy link
Contributor

@gagern gagern commented Dec 14, 2015

This is after reading Christian 'Little Jim' Plesner's answer to “What is the difference between Arguments::Holder() and Arguments::This()?” in the context of my Stack Overflow question “This vs. Holder for ObjectWrap::Unwrap”. Without the fix in my second commit, the test case from my first commit can lead to a failed assertion on Node 0.10. The change is also in line with the node::ObjectWrap documentation.

On Node 0.10.40, the test case will now fail with SIGABRT due to a failed
assertion:

node: ../../nan_object_wrap.h:33:
static T* Nan::ObjectWrap::Unwrap(v8::Local<v8::Object>)
[with T = MyObject]:
Assertion `object->InternalFieldCount() > 0' failed.

This is because This is not an instance of the type in question.
Node >=0.12 seem to report a TypeError: Illegal invocation instead.
This is neccessary on Node 0.10.40 at least, where the Signature is not
sufficient to ensure that This is actually an instance created from the
instance template of this ObjectWrap-derived class.
To ensure that the Holder of an accessor is indeed an instance of the corret
type, we can't attach the holders to the prototype but have to attach them
to the object itself.
@kkoopa
Copy link
Collaborator

kkoopa commented Dec 14, 2015

I know there is a difference between Holder and This, and I roughly know what said difference is. While using Holder probably is what is intended in most cases, this PR only changes tests, which need not necessarily reflect the best development practices with regard to V8 (although I am aware they are used as examples and references and should probably maintain some quality).

So, I'm happy with the added test case and the fix to make it pass, but I am unsure about all of the other changes.

@gagern
Copy link
Contributor Author

gagern commented Dec 14, 2015

this PR only changes tests

Nah, this PR also changes documentation. More so since my latest commit. The rest of the changes are just for the sake of consistency, but without these I hadn't spotted the issue about accessors which I had to address to make the whole test suite pass again, and which I included in my doc changes.

@gagern
Copy link
Contributor Author

gagern commented Dec 14, 2015

I wonder whether Nan::ObjectWrap should provide some combination of Unwrap and dynamic_cast: Something to take an object, check whether it has an internal field, interpret that as a pointer, check whether that pointer is of the expected type, and return the correctly-cast pointer only if all the checks succeed, returning NULL otherwise. Would that make sense for the scenario of using This in a prototype-installed accessor?

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 14, 2015

Doesn't dynamic_cast require RTTI? Node does not have that. Nan::ObjectWrap is just a clone of node::ObjectWrap from newer versions, which adds the persistent() and handle() methods, and in the Nan case changes the type to deal with Nan::Persistents instead. It is preferable to deviate as little as possible from upstream node and V8. When deviations are required, they should preferably be general and not specific and deviate as much as possible. Ideally all or nothing. If you want type checking added, propose it to core first.

Anyway, I think that documentation update was worth the price of the rest of the changes, so I'm happy to merge this.

@gagern
Copy link
Contributor Author

gagern commented Dec 14, 2015

ObjectWrap does have a virtual destructor, so it has a virtual function table, so it has RTTI. But I can gladly take this to core.

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 14, 2015

IIRC it is explicitly built with -fno-rtti.

kkoopa added a commit that referenced this pull request Dec 14, 2015
Pass Holder instead of This to ObjectWrap::Unwrap
@kkoopa kkoopa merged commit e59cf89 into nodejs:master Dec 14, 2015
@gagern gagern deleted the UnwrapHolder branch December 14, 2015 09:21
@kkoopa kkoopa mentioned this pull request Oct 21, 2024
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.

2 participants