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 regression: Leaked C++ object when calling from C++ to JS #22575

Open
stbergmann opened this issue Sep 16, 2024 · 1 comment
Open

Embind regression: Leaked C++ object when calling from C++ to JS #22575

stbergmann opened this issue Sep 16, 2024 · 1 comment
Labels

Comments

@stbergmann
Copy link
Contributor

I originally mentioned this in a comment at #21022 (comment) "embind: Don't automatically destroy arguments passed into val calls. (#21022)", but it likely got missed there:

Since 6e54690 "embind: Don't automatically destroy arguments passed into val calls. (#21022)" was merged into 3.1.52, the below test program behaves differently than before, and I wonder whether that's intentional. The test setup is

$ cat test.cc
#include <iostream>
#include <memory>

#include "emscripten.h"
#include "emscripten/bind.h"

class Smart {
public:
  Smart(): id_(counter++) { std::cout << "create " << id_ << "\n"; }
  Smart(Smart const & other): id_(counter++) {
    std::cout << "copy " << other.id() << " to " << id_ << "\n";
  }
  ~Smart() { std::cout << "destroy " << id_ << "\n"; }
  int id() const { return id_; }
private:
  void operator =(Smart) = delete;
  static unsigned counter;
  int const id_;
};

unsigned Smart::counter = 0;

std::shared_ptr<Smart> getSmart() {
  return std::make_shared<Smart>();
}

struct Interface {
  virtual void memFun(Smart const &) const {};
};

struct Wrapper: emscripten::wrapper<Interface> {
public:
  EMSCRIPTEN_WRAPPER(Wrapper);
  void memFun(Smart const & smart) const override {
      call<void>("memFun", smart);
  }
};

void useInterface(Interface const & ifc) {
  std::shared_ptr<Smart> s(getSmart());
  ifc.memFun(*s);
}

EM_JS(void, jsRun, (), {
  const Impl = Module.Interface.extend('Interface', {
    memFun(smart) { console.log('use ' + smart.id()); }
  });
  const impl = new Impl();
  const s = Module.getSmart();
  impl.memFun(s);
  impl.memFun(s);
  s.delete();
  Module.useInterface(impl);
});

EMSCRIPTEN_BINDINGS(test) {
  emscripten::class_<Smart>("Smart")
    .smart_ptr<std::shared_ptr<Smart>>("SmartPtr")
    .function("id", &Smart::id);
  emscripten::class_<Interface>("Interface")
    .function("memFun", &Interface::memFun)
    .allow_subclass<Wrapper>("Wrapper");
  emscripten::function("getSmart", &getSmart);
  emscripten::function("useInterface", &useInterface);
}

int main() {
  jsRun();
}

and

$ em++ test.cc -lembind -o test.html && emrun test.html

Until 3.1.51 (also in 3.1.47 and 3.1.48, i.e., irrespective of 12777ca "[emval] Reuse method calling optimisation in regular calls (#20383)" that was merged into 3.1.48 and motivated this change), the browser console logs were

create 0
use 0
use 0
destroy 0
create 1
copy 1 to 2
use 2
destroy 2
destroy 1

That is., the instance of Smart used in the call to JS ifc.memFun(*s); from C++ useInterface is properly destroyed.

Since 3.1.52, the browser console logs instead are

create 0
use 0
use 0
destroy 0
create 1
copy 1 to 2
use 2
destroy 1

That is, the instance of Smart used in the call to JS ifc.memFun(*s); from C++ useInterface is leaked now. But even if the new behavior is intentional: Where should that instance be destroyed? I think this commit assumes that the JS implementation

    memFun(smart) { console.log('use ' + smart.id()); }

should smart.delete(); it now. But then, that would apparently break the sequence of direct JS to JS calls

  impl.memFun(s);
  impl.memFun(s);

from within jsRun.

@brendandahl
Copy link
Collaborator

Thanks for the nice reproducer. I don't think we considered this specific case when making the changes above (nor do we have a test case that really covers it). The old behavior in this case is much nicer, but I'd still like operator() and call() to be consistent in how they handle argument lifetimes. A few things we could potentially do:

  • Add an option to call to auto delete all arguments.
  • Add an option per argument on how it should be handled.
  • Auto delete objects that were copy constructed when going from C++->JS. I think this would probably have issues like the previous automatic behavior though.

stbergmann added a commit to allotropia/emscripten that referenced this issue Oct 1, 2024
...due to <emscripten-core#22575> "Embind
regression: Leaked C++ object when calling from C++ to JS"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants