reearth / quickjs-emscripten-sync

Provides a way to sync objects between browser and QuickJS
MIT License
62 stars 7 forks source link

fix: add support for exposing functions that return objects #2

Closed namuol closed 2 years ago

namuol commented 2 years ago

I've been experimenting with this library and ran into a problem which is best illustrated with this simple test:

test("expose function that returns object", async () => {
  const vm = (await getQuickJS()).createVm();
  const arena = new Arena(vm);

  arena.expose({
    makeObject: () => {
      return {
        myFavoriteNumber: 42,
      };
    },
  });

  expect(arena.evalCode(`makeObject().myFavoriteNumber`)).toBe(42);

  arena.dispose();
  vm.dispose();
});

While attempting to call evalCode, a Lifetime not alive error is thrown. I traced the issue to an instance where VMMap::_mapSet was attempting to be called in the VM but was not alive.

I believe the _mapSet instance was no longer alive because the original VMMap used to marshal the return value was created ephemerally here:

https://github.com/reearth/quickjs-emscripten-sync/blob/21a7fd81803be76c7e3a605ab11c79ceffb13e04/src/index.ts#L196-L197

Then, it is merged with the Arena::map instance again before it is disposed via .dispose(), here:

https://github.com/reearth/quickjs-emscripten-sync/blob/21a7fd81803be76c7e3a605ab11c79ceffb13e04/src/index.ts#L219-L221

Then, some time after the ephemeral map is disposed of, a callback passed to .consume must be executed and at this point _mapSet has already been disposed:

https://github.com/reearth/quickjs-emscripten-sync/blob/203f8ae14f8390a0516b9380a6064a3f0dce8138/src/vmmap.ts#L87-L102

To get around this, I'm avoiding the creation of the ephemeral map entirely, and instead I'm pointing directly to the Arena::map instance.

This fixed the issue and didn't appear to cause any new test failures, but I'm not certain this change is actually safe and would love to know what you think.

Thank you for your help, and for sharing your hard work - your library has been a pleasure to work with. 🙇‍♂️

rot1024 commented 2 years ago

Thanks for your great PR. I just discovered this bug and was about to find out what caused it, but your PR saved me a lot of work! I will release a new version of quickjs-emscripten-sync as soon as possible!

rot1024 commented 2 years ago

v1.2.0 has been released!

namuol commented 2 years ago

Thank you!