util: fix isInsideNodeModules() check

[codebytere]

This PR reverts away from use of overrideStackTrace in isInsideNodeModules() introduced in #29777.

At the moment, Electron uses the v8 version of Error.prepareStackTrace as defined in v7.9.74 (where https://crbug.com/v8/7848 has been fixed) and not the one polyfilled here: #23926. As a result, we were experiencing failures in parallel/test-buffer-constructor-outside-node-modules.js because the polyfilled prepareStackTrace was not being run and thus the following code inside that function would never be executed:

  if (overrideStackTrace.has(error)) {
    const f = overrideStackTrace.get(error);
    overrideStackTrace.delete(error);
    return f(error, trace);
  }

so isInsideNodeModules() would always return false.

This change still allows for isInsideNodeModules() to be tested correctly while retaining correct functionality for Electron. Since isInsideNodeModules() is only ever invoked when determining whether to print a deprecation warning for new Buffer(), this change should have no effects on end users.

cc @devsnek @loc

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Co-authored-by: Andy Locascio [email protected]

[devsnek]

I made this change so that isInsideNodeModules continues to work in the presence of --enable-source-maps, which ignores Error.prepareStackTrace.

I think it would serve better for electron to figure out a way to use node's prepareStackTrace. I think you can manually call env->prepare_stack_trace_function()?

[addaleax]

I think is has become clear in #29994 that a non-functional Error.prepareStackTrace is not an option at the moment, so this LGTM.

[addaleax]

I’d also be good with reverting 70c2444 as an option in general.

[codebytere]

@addaleax happy to help out with whatever the end-solution is here - the repl changes are fine for us so for our purposes we're happy with as thinly-scoped a PR as possible i think!

[devsnek]

I'm not so sure that it's okay to consider a node environment valid if the embedder overrides our hooks into V8. @codebytere do you know why electron is overriding our hook in the first place?

[codebytere]

@devsnek but it's not a hook into v8 as much as a mutable JS prototype, right? Since the current Node.js code relies on a v8 cross-context pollution (linked in my PR body), you all would hit this same issue when you upgraded to our current version of v8 (v7.9.74) that we're currently using in Electron 🤔

[devsnek]

Our current hook runs for everything within an isolate, it isn't context specific. From there, it does context-specific dispatch (the line with globalThis.Error.prepareStackTrace). After that, it also checks the old behaviour, (MainContextError.prepareStackTrace), because random code in the ecosystem relies on that. Our canary builds run on the latest v8 lkgr, and they haven't hit any issues here (as far as I know), so I'm still not 100% sure what the issue is.

[codebytere]

@devsnek right - the issue here is that the test relies on Node's version of Error.prepareStackTrace being called, which Node.js took control of (to my knowledge) owing to the since-fixed v8 bug i linked above.

It seems like a fair assumption that we'd want to have behavior parity between the two versions, which is no longer the case with the change that was made to cause this issue.

[devsnek]

@codebytere node's version is identical except that it also falls back to the old v8 behaviour, which is needed for a lot of packages in the npm ecosystem. I assume electron wants to support those too? Additionally, I'm not sure what setup electron has where its both running node tests but doesn't have a proper node environment, so I can't really provide any good advice to that point.

[codebytere]

The overrideStackTrace WeakMap in Node's version doesn't exist though - making the test such that it'd never pass on the regular version, would it? The behavior otherwise is the same, sure, but the WeakMap would never be populated since that's specific to this version it seems?

[devsnek]

@codebytere well yes, but if you're running node tests outside of a node environment with that callback, having failures seems expected to me.

[codebytere]

@devsnek i'm not married to this solution - i'd just like to see if we can find a solution that allows both to work. I'm super happy to switch up my approach in this PR, my goal is just to have isInsideNodeModules()work for us as well :)

[devsnek]

I still don't understand why electron has disabled node's prepareStackTrace callback.

[codebytere]

@devsnek we don't explicitly disable it; this is at its root because we don't call Node.js' exposed NewIsolate(). Instead we create the isolate ourselves with v8::Isolate::Allocate().

As a result, SetIsolateUpForNode() does not get called, which is where Node has chosen to take control of Error.prepareStackTrace via

isolate->SetPrepareStackTraceCallback(PrepareStackTraceCallback);

[addaleax]

I’ve been wondering for a while whether we should maybe expose the V8 callbacks that we set through our public API … maybe this would be a good start? That would also potentially allow embedders to provide per-Context behaviour when that applies, which would be good imo.

If not, I think we should expose the two-argument variant of SetIsolateUpForNode() and make the second argument at least as fine-grained as Electron would need it.

[codebytere]

Also to elaborate a bit - we need to use this same isolate to set up gin (in Chromium - a V8 utility set), so we explicitly don't want to run all the other functionality inside the bespoke NewIsolate().

[codebytere]

@devsnek given my explanation above, what are your thoughts on a path forward here?

[devsnek]

if electron doesn't set up the isolate correctly, for whatever reason, things will fail to work correctly. On the node side, maybe we can expose SetUpIsolateForNode?

[codebytere]

Closing this given that we went the allow-custom-callbacks route in the end.