Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

calling JsAddRef/JsRelease for data passed to SetEmbedderData #374

Conversation

mike-kaufman
Copy link
Contributor

@mike-kaufman mike-kaufman commented Aug 18, 2017

chakrashim: ref count data sent to SetEmbedderData

JS Objects passed to SetEmbedderData need to have JsAddRef/JsRelease
called, otherwise they could be GC'd prematurely.

Fixes: #97

Affected core subsystem(s)

chakrashim

@mike-kaufman mike-kaufman self-assigned this Aug 18, 2017
@MSLaguana
Copy link
Contributor

Is it safe to pass a non-javascript object to JsAddRef? Does the JSRT layer determine whether a given pointer is part of the GC or not?

@mike-kaufman
Copy link
Contributor Author

Is it safe to pass a non-javascript object to JsAddRef? Does the JSRT layer determine whether a given pointer is part of the GC or not?

These lines should "do the right thing" if a non-GC'able pointer is passed into JsAddRef/JsRelease? Let me know if not the case.

https://github.com/nodejs/node-chakracore/blob/master/deps/chakrashim/core/lib/Jsrt/Jsrt.cpp#L583-L586
https://github.com/nodejs/node-chakracore/blob/master/deps/chakrashim/core/lib/Jsrt/Jsrt.cpp#L656-L659

@MSLaguana
Copy link
Contributor

Ah yep, looks like Recycler::IsValidObject checks to see if it's part of recycler owned memory, so I think this is fine.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Will this fix any unit test?

@mike-kaufman
Copy link
Contributor Author

Will this fix any unit test?

Not sure. How would I know?

@kunalspathak
Copy link
Member

How did you find out this issue? Was there some module that was breaking because of this?

@mike-kaufman
Copy link
Contributor Author

How did you find out this issue?

You opened a bug on it: #97. :)

@kunalspathak
Copy link
Member

Ah, its been a year....Moreover didn't read your PR description. Never mind.

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

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

:shipit:

@kfarnung
Copy link
Contributor

Kicked off a CI: https://ci.nodejs.org/job/chakracore-test-pull-request/141/

@mike-kaufman We don't use the "Merge pull request" button to merge changes, please sync up with me for the process.

JS Objects passed to SetEmbedderData need to have JsAddRef/JsRelease
called, otherwise they could be GC'd prematurely.

Fixes: nodejs#97

PR-URL: nodejs#374
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Kunal Pathak <[email protected]>
Reviewed-By: Jimmy Thomson <[email protected]>
@mike-kaufman mike-kaufman force-pushed the mkaufman-call-add-ref-for-set-embedder-data branch from 4d04cbb to 5e01643 Compare August 21, 2017 21:25
@mike-kaufman mike-kaufman merged commit 5e01643 into nodejs:master Aug 21, 2017
MSLaguana pushed a commit to MSLaguana/node-chakracore that referenced this pull request Sep 25, 2017
JS Objects passed to SetEmbedderData need to have JsAddRef/JsRelease
called, otherwise they could be GC'd prematurely.

Fixes: nodejs#97

PR-URL: nodejs#374
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Kunal Pathak <[email protected]>
Reviewed-By: Jimmy Thomson <[email protected]>
@mike-kaufman mike-kaufman deleted the mkaufman-call-add-ref-for-set-embedder-data branch August 25, 2018 18:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants