Fix napi_create_reference to work correctly with integers #380
Comments
Marking test_general/test flaky until nodejs#380 is fixed.
|
It looks like test_general/test has continued to evolve and is broken for an additional reason now: It attempts to create an object, wrap it, lose the JS reference to the object, force a GC, and ensure that the finalizer passed when wrapping was called. At the moment it looks like we might be mid-gc when the JS tries to force a gc, and so we complete that GC but it doesn't delete the only-just-now-created object. Other than that, there is some discussion about whether it makes sense to create references to primitive values at nodejs/node-addon-api#131 |
|
Honestly, I would prefer that ChakraCore be able to create references to
primitive values, if only because there might be some obscure case out
there in the wild where this is happening.
Yet, if it's not feasible, I can submit a PR that modifies test_general to
not attempt to create references to primitives. After all, that is not the
purpose of that test. The reference to the primitive is used only as a test
payload for the object wrap.
…On Thu, Sep 7, 2017 at 9:30 PM, Jimmy Thomson ***@***.***> wrote:
It looks like test_general/test has continued to evolve and is broken for
an additional reason now: It attempts to create an object, wrap it, lose
the JS reference to the object, force a GC, and ensure that the finalizer
passed when wrapping was called. At the moment it looks like we might be
mid-gc when the JS tries to force a gc, and so we complete that GC but it
doesn't delete the only-just-now-created object.
Other than that, there is some discussion about whether it makes sense to
create references to primitive values at nodejs/node-addon-api#131
<nodejs/node-addon-api#131>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#380 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA7k0cZ3HoGrA4_-UfTEj4Ai-L-pNU8Fks5sgDZCgaJpZM4PDVIA>
.
|
|
@gabrielschulhof semantically, what does a reference to a primitive mean if a primitive is not heap allocated? ChakraCore's JSRT returns JsValueRefs that are opaque (can be either pointers to heap allocated objects or primitive values)- are you suggesting in the case of primitive values, we always return boxed (heap allocated) values? That seems to be a rather expensive perf cost for supporting obscure cases, no? Alternatively, if you're not suggesting that, then when you say that we create references to primitive values, are you suggesting that we box the value as part of the CreateWeakReference api? That also seems fraught since that value would get collected at the next GC since no one would have a strong handle to that, just a weak one. So it's unclear what the desired behavior should be. My instinct is that if napi has a chance to define this behavior more concretely right now, we should just say that |
|
I think it's pretty safe to return We might even go so far as to document that only objects and functions can be retained as |
|
In fact, we document that the |
The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. Re: nodejs/node-chakracore#380
The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. PR-URL: #15289 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Re: nodejs/node-chakracore#380
The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. PR-URL: nodejs#15289 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Re: nodejs/node-chakracore#380
The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. PR-URL: #15289 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Re: nodejs/node-chakracore#380
The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. PR-URL: nodejs#15289 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Re: nodejs/node-chakracore#380
The binding testing napi_wrap() creates references to primitives passed into the binding in its second parameter. This is unnecessary and not at all the point of the test. Additionally, creating persistent references to primitive values may not be supported by all VMs, since primitives are best persisted in their native form. Instead, the point of the test is to make sure that the finalize callback gets called when it should get called, that it gets called with the correct pointer, and that it does not get called when it should not get called. Creating persistent references is not necessary for verifying this. Backport-PR-URL: #19447 PR-URL: #15289 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Re: nodejs/node-chakracore#380
The N-API implementation of napi_create_reference uses JsCreateWeakReference. If a reference is created for an integer then there is an access violation during the GC update of the weak reference map. I proposed a possible fix in chakra-core/ChakraCore#3591, but once an approved fix is found we'll need to re-enable test_general/test.
The text was updated successfully, but these errors were encountered: