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

util: fix isInsideNodeModules() check #30014

Closed

Conversation

codebytere
Copy link
Member

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 andy@slack-corp.com

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Oct 18, 2019
@codebytere codebytere changed the title lib: don't overrideStackTrace in isInsideNodeModules util: don't overrideStackTrace in isInsideNodeModules Oct 18, 2019
Co-authored-by: Andy Locascio <andy@slack-corp.com>
@codebytere codebytere changed the title util: don't overrideStackTrace in isInsideNodeModules util: fix isInsideNodeModules() check Oct 18, 2019
@devsnek
Copy link
Member

devsnek commented Oct 18, 2019

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()?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@codebytere
Copy link
Member Author

@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
Copy link
Member

devsnek commented Oct 18, 2019

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
Copy link
Member Author

@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
Copy link
Member

devsnek commented Oct 18, 2019

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
Copy link
Member Author

@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
Copy link
Member

devsnek commented Oct 22, 2019

@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
Copy link
Member Author

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
Copy link
Member

devsnek commented Oct 22, 2019

@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
Copy link
Member Author

codebytere commented Oct 22, 2019

@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
Copy link
Member

devsnek commented Oct 22, 2019

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

@codebytere
Copy link
Member Author

codebytere commented Oct 23, 2019

@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
Copy link
Member

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
Copy link
Member Author

codebytere commented Oct 23, 2019

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
Copy link
Member Author

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

@devsnek
Copy link
Member

devsnek commented Oct 25, 2019

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
Copy link
Member Author

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

@codebytere codebytere closed this Dec 5, 2019
@codebytere codebytere deleted the dont-use-overridestacktrace branch December 5, 2019 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants