FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fewer uses of NODE_USE_V8_PLATFORM by codebytere · Pull Request #30029 · nodejs/node · GitHub

/ node Public

src: fewer uses of NODE_USE_V8_PLATFORM - #30029

Closed
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:no-gate-inspector-v8-plat
ConversationCommits (1)ChecksFiles changed
Closed

src: fewer uses of NODE_USE_V8_PLATFORM#30029
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:no-gate-inspector-v8-plat

Conversation

codebytere commented Oct 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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:

Details 
/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 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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 18, 2019

nodejs-github-bot commented Oct 18, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26103/

joyeecheung approved these changes Oct 20, 2019
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax commented Oct 21, 2019

Copy link
Copy Markdown
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
src: fewer uses of NODE_USE_V8_PLATFORM
71b342f 
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 commented Oct 21, 2019

Copy link
Copy Markdown
Member

Landed in 71b342f, thanks for the PR!

addaleax closed this Oct 21, 2019
codebytere deleted the no-gate-inspector-v8-plat branch October 21, 2019 15:47
codebytere mentioned this pull request Oct 21, 2019
Closed
3 tasks
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
src: fewer uses of NODE_USE_V8_PLATFORM
0c88dc1 
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 mentioned this pull request Oct 23, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
src: fewer uses of NODE_USE_V8_PLATFORM
ad57e9f 
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
src: fewer uses of NODE_USE_V8_PLATFORM
94d6719 
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 mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
src: fewer uses of NODE_USE_V8_PLATFORM
4b57088 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
JungMinu JungMinu approved these changes
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL