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

embind: Val call invokers delete object arguments #21016

Closed
brendandahl opened this issue Jan 4, 2024 · 2 comments · Fixed by #21022
Closed

embind: Val call invokers delete object arguments #21016

brendandahl opened this issue Jan 4, 2024 · 2 comments · Fixed by #21022

Comments

@brendandahl
Copy link
Collaborator

brendandahl commented Jan 4, 2024

This is a regression from #20383. Before the patch the destructor of objects passed into a val functions were not automatically run, but now they are. While this is a regression, I think it's actually just copying the strange(broken?) behavior of the other code. There was another issue filed about this as #20095 related to val::call which used the optimized code that val::operator() now uses.

I did a bit of digging and it appears that automatically destroying the object arguments was the originally intended behavior in this patch and that the user should call clone to avoid this.

I'm still thinking about the best approach here, but my initial thought is that the user should handle explicitly deleting the object if they need to.

Output before:

>>> constructor
>>> before callback
branch [object Object]
>>> after callback
>>> destructor

After:

>>> constructor
>>> before callback
branch [object Object]
>>> destructor
>>> after callback
>>> destructor
#include <stdio.h>
#include <emscripten.h>
#include <emscripten/bind.h>

using namespace emscripten;

struct Branch {
  Branch() {
    printf(">>> constructor\n");
  }
  ~Branch() {
    printf(">>> destructor\n");
  }
};

class Editor {
public:
  void WithBranch(std::function<void(Branch*)> callback) {
    Branch b;
    printf(">>> before callback\n");
    callback(&b);  // This pointer should not be deleted!
    printf(">>> after callback\n");
  }
};

void EditorWithBranch(Editor& editor, val callback) {
  editor.WithBranch([callback](Branch* branch) { callback(branch); });
}

int main() {
EM_ASM(
  var editor = new Module.Editor();
  editor.withBranch((branch) => {
    console.log("branch " + branch);
  });
);
  
}

EMSCRIPTEN_BINDINGS(xxx) {
  class_<Editor>("Editor").constructor<>().function("withBranch", &EditorWithBranch);
  class_<Branch>("Branch");
}
@brendandahl
Copy link
Collaborator Author

cc @RReverser

@RReverser
Copy link
Collaborator

Hm this is a tricky one, especially because there's no way to express "pass by reference" vs "pass by value" in dynamic JS calls...

I tend to agree that passing by reference by default and leaving destructors to C++ is likely the better choice though.

brendandahl added a commit to brendandahl/emscripten that referenced this issue Jan 5, 2024
Before PR emscripten-core#20383 val's `call` and `operator()` had different behaviors where
one would automatically destroy arguments after the call and the other
didn't. After that PR, they both called destroy. The automatic destruction
wasn't really documented anywhere and and led to some unexpected behavior.

This changes it so neither automatically destroys the arguments which I
think is more inline with the rest of embind where it's up to the user
to handle object lifetimes.

Fixes: emscripten-core#21016 and emscripten-core#20095
brendandahl added a commit to brendandahl/emscripten that referenced this issue Jan 8, 2024
Before PR emscripten-core#20383 val's `call` and `operator()` had different behaviors where
one would automatically destroy arguments after the call and the other
didn't. After that PR, they both called destroy. The automatic destruction
wasn't really documented anywhere and and led to some unexpected behavior.

This changes it so neither automatically destroys the arguments which I
think is more inline with the rest of embind where it's up to the user
to handle object lifetimes.

Fixes: emscripten-core#21016 and emscripten-core#20095
brendandahl added a commit to brendandahl/emscripten that referenced this issue Jan 11, 2024
Before PR emscripten-core#20383 val's `call` and `operator()` had different behaviors where
one would automatically destroy arguments after the call and the other
didn't. After that PR, they both called destroy. The automatic destruction
wasn't really documented anywhere and and led to some unexpected behavior.

This changes it so neither automatically destroys the arguments which I
think is more inline with the rest of embind where it's up to the user
to handle object lifetimes.

Fixes: emscripten-core#21016 and emscripten-core#20095
brendandahl added a commit that referenced this issue Jan 11, 2024
…21022)

Before PR #20383 val's `call` and `operator()` had different behaviors where
one would automatically destroy arguments after the call and the other
didn't. After that PR, they both called destroy. The automatic destruction
wasn't really documented anywhere and and led to some unexpected behavior.

This changes it so neither automatically destroys the arguments which I
think is more inline with the rest of embind where it's up to the user
to handle object lifetimes.

Fixes: #21016 and #20095
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 a pull request may close this issue.

2 participants