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

src: fewer uses of NODE_USE_V8_PLATFORM #30029

Closed

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 18, 2019

This PR removes some uses of NODE_USE_V8_PLATFORM in conjunction with HAVE_INSPECTOR.

Electron makes use of HAVE_INSPECTOR but does not run with NODE_USE_V8_PLATFORM so this prevented some Inspector code from running properly on our end; specifically, we were seeing a crash in test/parallel/test-inspector-connect-main-thread.js:

/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/MacOS/Electron[77672]: ../../third_party/electron_node/src/inspector_agent.cc:834:std::unique_ptr<InspectorSession> node::inspector::Agent::ConnectToMainThread(std::unique_ptr<InspectorSessionDelegate>, bool): Assertion `(parent_handle_) != nullptr' failed.
 1: 0x122cb7c35 node::Abort() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 2: 0x122cb79bf node::Assert(node::AssertionInfo const&) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 3: 0x122d72a3d node::inspector::Agent::ConnectToMainThread(std::__1::unique_ptr<node::inspector::InspectorSessionDelegate, std::__1::default_delete<node::inspector::InspectorSessionDelegate> >, bool) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 4: 0x122d7baeb node::inspector::(anonymous namespace)::JSBindingsConnection<node::inspector::(anonymous namespace)::MainThreadConnection>::New(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 5: 0x11a93c05f v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 6: 0x11a8d19ee v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 7: 0x11a8d0882 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 8: 0x11a8d037c v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 9: 0x11b838da0 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
10: 0x11b603557 Builtins_JSBuiltinsConstructStub [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
/Users/codebytere/build-tools/nix/commands/node.sh: line 8: 77672 Abort trap: 6           ELECTRON_RUN_AS_NODE=1 "$ELECTRON_EXEC" "$@"

cc @joyeecheung (who touched some of this recently) and @addaleax

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 18, 2019
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM, though I wonder who uses NODE_USE_V8_PLATFORM if Electron does not use it?

@addaleax
Copy link
Member

@joyeecheung If I understood @codebytere correctly, this is because they have to use the same platform instance for the Chromium parts as well; I think for most other embedders using NODE_USE_V8_PLATFORM would make sense.

addaleax pushed a commit that referenced this pull request Oct 21, 2019
PR-URL: #30029
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@addaleax
Copy link
Member

Landed in 71b342f, thanks for the PR!

@addaleax addaleax closed this Oct 21, 2019
@codebytere codebytere deleted the no-gate-inspector-v8-plat branch October 21, 2019 15:47
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
PR-URL: #30029
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
PR-URL: #30029
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
targos pushed a commit that referenced this pull request Nov 8, 2019
PR-URL: #30029
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
PR-URL: #30029
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2019
PR-URL: #30029
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants