FunctionReference destructor called on wrong thread

[kmatzen]

Describe the bug
I'm occasionally seeing shared_ptr ref counts to FunctionReference's going to zero inside the TimeoutDispatcher's ThreadedFunction. This triggers the destructor for the FunctionReference which is not on the JS runtime thread. This can cause the CHECK_ENV assert to fail when calling napi_delete_reference.

To Reproduce
I think this specifically happens with functions given to setTimeout when using the Window polyfill. I haven't been able to repro it with a minimal example, but it seems to happen when I have a bunch of setTimeout events that go off near each other. e.g., If I have a bunch of dispose() calls on scene objects in my render loop for several frames in a row.

Expected behavior
It looks like most of the time, the ref count to the shared pointer goes to zero inside the thread managing the js runtime, so the call to napi_delete_reference runs correctly.

Screenshots
Here's a sample stack trace where the assert fails.

Other

• Platform: macOS

I played around with some modifications and got something to work a little more reliably. In particular, I think the std::shared_ptr is very confusing to debug when this particular object has to be destructed in a very particular location. I think a std::unique_ptr makes it much more clear how to ensure the object is destructed on the correct thread. The only reason a unique_ptr wasn't used I believe is because the dispatcher needs a std::function with a lambda capturing the smart pointer. Since std::function is copyable, it can't have a lambda that has move-captured a unique_ptr.

I instead came up with this:

    void TimeoutDispatcher::CallFunction(std::unique_ptr<Napi::FunctionReference> function)
    {
        if (function)
        {
            // Since we need a std::function and a std::function is copyable, we can't
            // just move-capture the unique_ptr when forming the lambda.  Instead, release
            // the raw pointer, capture it in the lambda, and then make sure to delete it
            // manually when we're done.
            auto ptr = function.release();
            auto fn = [ptr](Napi::Env env) {
                try {
                    ptr->Call({});
                    delete ptr;
                } catch (...) {
                    delete ptr;
                    throw;
                }
            };
            m_runtime.Dispatch(std::move(fn));
        }
    }

I also double checked the ref count on the version with the shared_ptr and it always appears to be 1 once it reaches CallFunction, even in the event that the assert is failed.

[bghgary]

Thanks for the report. I've seen this as well.