Environment can be null

[farhaven]

In 478815b5dab8c1a7d0fa505eed005c2465440810 the return type of IsolateEnvironment::GetCurrent() was changed from a pointer to an environment to a reference to an environment, with the reasoning that the return value is always dereferenced and hence should be a reference.

The problem is that in C++, references can not be null, but IsolateEnvironment::GetCurrent() calls Executor::GetCurrentEnvironment() which has the following body:

    return current_executor == nullptr ? nullptr : &current_executor->env;

and thus can return a null pointer, making the behaviour undefined (since in valid C++, references can't be null).

This seems to happen if GC timing is unfortunate during teardown of a NodeJS worker thread that runs code in a separate isolate, at least that's what our CI suggests.

At any rate, the assertion in https://github.com/laverdet/isolated-vm/blob/3f13ecac00eaac57224222fef692a55aa10bf960/src/isolate/environment.h#L202 triggers.

[laverdet]

A stack trace would be helpful if you can provide it. The change 478815b5dab8c1a7d0fa505eed005c2465440810 was legit because without the assertion we otherwise would be dereferencing a null pointer. The assertion fails faster and louder.

[jpshack-at-palomar]

I believe I am seeing this w/ node 20.10.0 and isolated-vm 5.0.0 on linux but not MacOS. I have an exit hander that tries to clean up when the process ends. I get a clean exit on MacOS but on linux after all test has passed successfully at the very end I get: node: ../src/isolate/environment.h:202: static ivm::IsolateEnvironment& ivm::IsolateEnvironment::GetCurrent(): Assertion `environment != nullptr' failed and non zero exit code.

Happy to provide any additional information if you can tell me how to get it.

   static async initializeIsolate() {
        if (!EvalAction.isolate) {
            EvalAction.isolate = new ivm.Isolate();

            process.on('exit', (code) => {
                this.debug(`The process is terminating with with code: ${code}`);
                this.debug(`Disposing of VM isolate and context...`);
                if (EvalAction.isolate) {
                    try {
                        EvalAction.isolate.dispose();
                        EvalAction.isolate = null;
                        EvalAction.context = null;
                    } catch {
                        // no op
                        // tests are passing but in ci I see this error after the test suite has run:
                        // node: ../src/isolate/environment.h:202: 
                        // static ivm::IsolateEnvironment& ivm::IsolateEnvironment::GetCurrent(): 
                        // Assertion `environment != nullptr' failed.
                    }
                }

                this.debug('Cleanup complete.');
            });

            EvalAction.context = await EvalAction.isolate!.createContext();

            // Expose globals
            await EvalAction.initGlobals();

        }
    }

[laverdet]

It's a known issue that sometimes isolated-vm will uncleanly exit. The order in which the isolates tear themselves down is not well defined. Disposing your isolates explicitly on process exit is not necessary. The operating system cleans those resources up for you.