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

ensure function args outlive prepare_call helper #161

Merged
merged 1 commit into from
Jan 21, 2017
Merged

ensure function args outlive prepare_call helper #161

merged 1 commit into from
Jan 21, 2017

Conversation

jedireza
Copy link
Contributor

@jedireza jedireza commented Jan 3, 2017

We have a suspicious failing test:

  1) JsFunction call a JsFunction built in JS that implements x => x + 1:
     AssertionError: expected 1 to equal 17
      at Context.<anonymous> (lib/functions.js:14:12)

We expect the number 16 to be passed to our JavaScript function but it's mostly being passed 0. I say mostly because sometimes it's a random number. 😳

I've also witnessed random segmentation faults when running tests such as:

[1]    18077 segmentation fault (core dumped)  npm t

It turned out that when we pass args to the prepare_call helper, that values were being freed at the end of it's scope. The docs for as_mut_ptr have a clue about this:

The caller must ensure that the slice outlives the pointer this function returns, or else it will end up pointing to garbage.

With the help of @alexcrichton, we now pass prepare_call a reference instead which allows the value to live long enough.

The segmentation fault seems to be resolved too, which makes sense. I haven't seen it since implementing this change.


Update: I believe this bug was introduced in 7fc8d98 when some logic in JsFunction::call was abstracted out into prepare_call to support #61.

@jedireza jedireza requested a review from dherman January 3, 2017 20:52
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

r+

Looks perfect – thanks for chasing this down!

@dherman dherman merged commit e25a392 into neon-bindings:master Jan 21, 2017
@jedireza jedireza deleted the function-call-fix branch February 17, 2017 16:50
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