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

Match the callback boxing of Function::new in TemplateFunction::new #5

Merged
merged 1 commit into from
Oct 7, 2016

Conversation

tcosprojects
Copy link
Contributor

@tcosprojects tcosprojects commented Oct 7, 2016

Change function tests to pass fn items instead of references.

While working on this change it occurred to me that it isn't safe for the callbacks to be closures. Once these callback functions are exposed to JS they can be referenced and held by JS code indefinitely so the function and any captured environment must live as long as the isolate.

A potential strategy for handling this would be to only allow 'static functions or closures that pass ownership of their environment to V8 through the internal object. Ownership could also be shared through Rc. This should still be convenient to work with in rust while ensuring the callbacks are safe for the duration of the isolate.

The move ownership to V8 or Rc strategy would also be useful when storing rust values in a value::Object's internal fields.

Change function tests to pass fn items instead of references
@codecov-io
Copy link

codecov-io commented Oct 7, 2016

Current coverage is 90.76% (diff: 100%)

Merging #5 into master will decrease coverage by 0.03%

@@             master         #5   diff @@
==========================================
  Files            11         11          
  Lines          1098       1094     -4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            997        993     -4   
  Misses          101        101          
  Partials          0          0          

Powered by Codecov. Last update 9dd7fd8...803d6fe

@dflemstr
Copy link
Owner

dflemstr commented Oct 7, 2016

I also thought about the lifetime stuff and I want to try to use persistent handles to properly manage the lifetime of the closure box.

Right now the library leaks memory for some other reasons too so it's not super urgent to fix I think

@@ -54,11 +54,11 @@ impl FunctionTemplate {
callback: F) -> FunctionTemplate
where F: Fn(value::FunctionCallbackInfo) -> value::Value {
let raw = unsafe {
let callback = Box::into_raw(Box::new(callback));
let callback_ptr: *mut Box<F> = Box::into_raw(Box::new(Box::new(callback)));
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for missing this! I wonder how the tests passed? I will do reviews from now on haha

@dflemstr dflemstr merged commit 60e8fda into dflemstr:master Oct 7, 2016
@tcosprojects
Copy link
Contributor Author

Here is an example of the lifetime issue:

struct Foo {
    msg: String
}

fn main() {
    let isolate = v8::Isolate::new();
    let context = v8::Context::new(&isolate);

    let f = {
        let foo = Foo {
            msg: "Hello, World!".into()
        };

        value::Function::new(&isolate, &context, 0, |_: value::FunctionCallbackInfo| {
            println!("foo = {}", &foo.msg);

            value::undefined(&isolate).into()
        })
    };

    let bar = Foo {
        msg: "Goodbye, World!".into()
    };
    let name = value::String::from_str(&isolate, "f");
    context.global().set(&context, &name, &f);

    let source = value::String::from_str(&isolate, "f();");
    let script = v8::Script::compile(&isolate, &context, &source).unwrap();
    let result = script.run(&context).unwrap();
    let result_str = result.to_string(&context);

    println!("bar = {}", bar.msg);

    println!("Script result = {}", result_str.to_string());
}

This outputs:

foo = Goodbye, Worl
bar = Goodbye, World!
Script result = undefined

Or sometimes terminates. The Function callback holds &foo after its been deallocated from the stack.

@tcosprojects tcosprojects deleted the ft_callback_boxing branch October 7, 2016 13:28
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.

3 participants