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

Already on GitHub? Sign in to your account

Use `rvagg/nan` #19

Merged
merged 20 commits into from Nov 18, 2013

Conversation

Projects
None yet
3 participants
Owner

TooTallNate commented Nov 17, 2013

Big refactor.

Works beautifully on v0.8.x and v0.10.x :)

Doesn't quite work properly yet on v0.11.x. It compiles, but segfaults in the Create c++ function :(

/cc @rvagg @kkoopa @Raynos @trevnorris

kkoopa commented Nov 17, 2013

Is this 0.11.8 you've tested with? If it works in 0.11.7, it is likely that a reference becomes weak and gets deallocated before its time. Something was changed there in the transition from 0.11.7 to 0.11.8.

@rvagg rvagg commented on the diff Nov 17, 2013

src/weakref.cc
- return scope.Close(GetCallbacks(proxy));
+NAN_METHOD(SetCallback) {
@kkoopa

kkoopa Nov 17, 2013

Shouldn't hurt, but on first glance, there doesn't seem to be any stack manipulation, so it might not be strictly necessary. However, if this is changed, it will come back to haunt you for hours of debugging.

@kkoopa kkoopa commented on the diff Nov 17, 2013

src/weakref.cc
proxy_container *cont = (proxy_container *)
malloc(sizeof(proxy_container));
- cont->target = Persistent<Object>::New(args[0]->ToObject());
- cont->callbacks = Persistent<Array>::New(Array::New());
-
- cont->proxy = Persistent<Object>::New(proxyClass->NewInstance());
-#if NODE_VERSION_AT_LEAST(0, 11, 0)
- cont->proxy->SetAlignedPointerInInternalField(0, cont);
-#else
- cont->proxy->SetPointerInInternalField(0, cont);
-#endif
+ Local<Object> proxy = NanPersistentToLocal(proxyClass)->NewInstance();
+ NanAssignPersistent(Object, cont->proxy, proxy);
@kkoopa

kkoopa Nov 17, 2013

This is where it segfaults. If this line is removed, the next one that accesses cont fails.

@rvagg

rvagg Nov 17, 2013

Ah, because the empty Persistents aren't initialised. You now can't just do a Persistent::New but you do have to start off with an empty Persistent in order to be able to assign to it. The malloc allocation above isn't going to suffice here. I'd probably change proxy_container to a class and new it and you ought to end up with what's needed (then delete instead of free of course). Is there another way here?

@kkoopa

kkoopa Nov 17, 2013

Persistent::Reset() should work.

But it seems it doesn't.

@TooTallNate

TooTallNate Nov 18, 2013

Owner

@rvagg I'm not sure I understand how turning proxy_container into a class, from a struct, would make any difference in this case. Can you elaborate?

@kkoopa kkoopa and 1 other commented on an outdated diff Nov 17, 2013

src/weakref.cc
proxy_container *cont = (proxy_container *)
malloc(sizeof(proxy_container));
- cont->target = Persistent<Object>::New(args[0]->ToObject());
- cont->callbacks = Persistent<Array>::New(Array::New());
-
- cont->proxy = Persistent<Object>::New(proxyClass->NewInstance());
-#if NODE_VERSION_AT_LEAST(0, 11, 0)
- cont->proxy->SetAlignedPointerInInternalField(0, cont);
-#else
- cont->proxy->SetPointerInInternalField(0, cont);
-#endif
+ Local<Object> proxy = NanPersistentToLocal(proxyClass)->NewInstance();
+ NanAssignPersistent(Object, cont->proxy, proxy);
+ NanAssignPersistent(Object, cont->target, args[0]->ToObject());
+ NanAssignPersistent(Object, cont->emitter, args[1]->ToObject());
@kkoopa

kkoopa Nov 17, 2013

Here it has already been asserted that args[0] is an object, so args[0].As<Object>() can be used and is faster. But, why is there no assertion on args[1]?

@TooTallNate

TooTallNate Nov 18, 2013

Owner

At this point, _create is an internal function which will always be passed something as the first arg, and an EventEmitter instance as the second arg. Hence no check necessary. I'll update to use .As<Object>() though.

Owner

TooTallNate commented Nov 18, 2013

Thanks to @rvagg for fixing the segfault in #20. Merging this bad-boy to master now that tests are looking good on v0.8, v0.10, and v0.11.

@TooTallNate TooTallNate added a commit that referenced this pull request Nov 18, 2013

@TooTallNate TooTallNate Merge pull request #19 from TooTallNate/refactor/nan
Use `rvagg/nan`
64ecb63

@TooTallNate TooTallNate merged commit 64ecb63 into master Nov 18, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details

@TooTallNate TooTallNate deleted the refactor/nan branch Nov 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment