Skip to content

Conversation

@mrsharpoblunto
Copy link
Contributor

I noticed an issue where an object wrapped via v8pp::reference_external was having its destructor called after the wrapper was garbage collected. This caused crashes due to double freeing in my app because I was also calling the object destructor.

This seems to be because in the SetWeak callback, all objects get their destructor called, regardless of the call_dtor param in the wrap_object method. This PR queries the internal call_dtor field and only calls the destructor if the wrap_object method specified that this should be the case.

@mrsharpoblunto
Copy link
Contributor Author

Whoops, didn't realize that v8::WeakCallbackType::kInternalFields only passes the first 2 internal fields to the callback - so the destructor never gets called. I'll rethink how this should work. Just wondering would the reference/unreference_external api's be necessary if a new unowned_ptr trait was added instead that allowed storing a raw pointer but never called the objects destructor?

@pmed
Copy link
Owner

pmed commented Jun 5, 2019

Hi Glenn,

Thank you for the contribution.

Could you please supply a test case that demonstrates the issue? As far as I remember, there was a similar issue #60, and I tried to fix it by placing the call_dtor flag into the 3rd JS object field.

Currently this call_dor is being extracted in reset_object() only when the object's persistent handle IsNearToDeath(). Maybe this flag should be extracted always.

So we only need a single bit, in order to mark the object for deletion with its destructor. Such a flag could also be stored together with Global handle in object_registry::objects_ mapping.

@pmed
Copy link
Owner

pmed commented Jun 23, 2019

Hi @mrsharpoblunto

Since V8 version 7.5 IsNearDeath() member function in persistent handle class was removed. So in a591866 I store the call_dtor bool flag along with the weak persistent handle for a wrapped C++ object in a wrapped_object struct.

Please check the issue is gone on your side after this fix. I could close this pull request, if it would be fine.

@mrsharpoblunto
Copy link
Contributor Author

Hi @pmed everything working with your latest changes. Thanks!

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