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

FinalizationRegistry.register() call missing in exported rust types that define a constructor #3854

Open
Dig-Doug opened this issue Feb 24, 2024 · 1 comment · May be fixed by #3940
Open
Labels

Comments

@Dig-Doug
Copy link

Describe the Bug

FinalizationRegistry.register() call missing in exported rust types that define a constructor, preventing cleanup.

Steps to Reproduce

  1. Add this rust code to one of the wasm-bindgen tests, e.g. test/api.rs:
#[wasm_bindgen]
pub struct ObjectWithConstructor {}

#[wasm_bindgen]
impl ObjectWithConstructor {
    #[wasm_bindgen(constructor)]
    pub fn new() -> Self {
        Self {}
    }
}
  1. Run tests to generate JS code

  2. View generated JS code:

const ObjectWithConstructorFinalization = (typeof FinalizationRegistry === 'undefined')
    ? { register: () => {}, unregister: () => {} }
    : new FinalizationRegistry(ptr => wasm.__wbg_objectwithconstructor_free(ptr >>> 0));
/**
*/
class ObjectWithConstructor {

    __destroy_into_raw() {
        const ptr = this.__wbg_ptr;
        this.__wbg_ptr = 0;
        ObjectWithConstructorFinalization.unregister(this);
        return ptr;
    }

    free() {
        const ptr = this.__destroy_into_raw();
        wasm.__wbg_objectwithconstructor_free(ptr);
    }
    /**
    */
    constructor() {
        const ret = wasm.objectwithconstructor_new();
        this.__wbg_ptr = ret >>> 0;
        return this;
    }
}
module.exports.ObjectWithConstructor = ObjectWithConstructor;

Expected Behavior

I expect there to be a line in the constructor to register, e.g. ObjectWithConstructorFinalization.register(this, this.__wbg_ptr, this);

Actual Behavior

In the ObjectWithConstructor code, there is never a call to register the object destruction callback.

If we compare to this object, which does not define a constructor:

#[wasm_bindgen]
pub struct ObjectNoConstructor {}

#[wasm_bindgen]
impl ObjectNoConstructor {
    pub fn new() -> Self {
        Self {}
    }
}

It has a register call in the __wrap function:

const ObjectNoConstructorFinalization = (typeof FinalizationRegistry === 'undefined')
    ? { register: () => {}, unregister: () => {} }
    : new FinalizationRegistry(ptr => wasm.__wbg_objectnoconstructor_free(ptr >>> 0));
/**
*/
class ObjectNoConstructor {

    constructor() {
        throw new Error('cannot invoke `new` directly');
    }

    static __wrap(ptr) {
        ptr = ptr >>> 0;
        const obj = Object.create(ObjectNoConstructor.prototype);
        obj.__wbg_ptr = ptr;
        ObjectNoConstructorFinalization.register(obj, obj.__wbg_ptr, obj);
        return obj;
    }

    __destroy_into_raw() {
        const ptr = this.__wbg_ptr;
        this.__wbg_ptr = 0;
        ObjectNoConstructorFinalization.unregister(this);
        return ptr;
    }

    free() {
        const ptr = this.__destroy_into_raw();
        wasm.__wbg_objectnoconstructor_free(ptr);
    }
    /**
    * @returns {ObjectNoConstructor}
    */
    static new() {
        const ret = wasm.objectnoconstructor_new();
        return ObjectNoConstructor.__wrap(ret);
    }
}
module.exports.ObjectNoConstructor = ObjectNoConstructor;

Additional Context

Should the register call be generated here?

@Dig-Doug Dig-Doug added the bug label Feb 24, 2024
@daxpedda
Copy link
Collaborator

I'm not familiar with this part of the code, but whats missing is a call to Context::require_class_wrap().

Happy to dig into it if a PR is provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants