★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/30029Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
wants to merge 1 commit into from

Conversation

@codebytere
Copy link
Member

codebytere commented Oct 18, 2019 •
edited

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
@codebytere
src: fewer uses of NODE_USE_V8_PLATFORM
Loading status checks…
0a4b335
@addaleax
addaleax approved these changes Oct 18, 2019
View changes
@cjihrig
cjihrig approved these changes Oct 18, 2019
View changes
@devsnek
devsnek approved these changes Oct 18, 2019
View changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

nodejs-github-bot commented Oct 18, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/26103/
@JungMinu
JungMinu approved these changes Oct 20, 2019
View changes
@joyeecheung
joyeecheung approved these changes Oct 20, 2019
View changes
Copy link
Member

joyeecheung left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Oct 21, 2019

@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 added a commit that referenced this pull request Oct 21, 2019
@codebytere @addaleax
src: fewer uses of NODE_USE_V8_PLATFORM
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
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

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Oct 21, 2019

Landed in 71b342f, thanks for the PR!
@addaleax addaleax closed this Oct 21, 2019
@codebytere codebytere deleted the codebytere:no-gate-inspector-v8-plat branch Oct 21, 2019
@codebytere codebytere mentioned this pull request Oct 21, 2019
Closed
3 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Oct 23, 2019
@codebytere @MylesBorins
src: fewer uses of NODE_USE_V8_PLATFORM
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
ada1aaa 
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 added a commit that referenced this pull request Oct 23, 2019
@codebytere @MylesBorins
src: fewer uses of NODE_USE_V8_PLATFORM
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
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 MylesBorins mentioned this pull request Oct 23, 2019
Merged
targos added a commit that referenced this pull request Nov 8, 2019
@codebytere @targos
src: fewer uses of NODE_USE_V8_PLATFORM
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
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 added a commit that referenced this pull request Nov 10, 2019
@codebytere @targos
src: fewer uses of NODE_USE_V8_PLATFORM
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
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 targos mentioned this pull request Nov 10, 2019
Merged
targos added a commit that referenced this pull request Nov 11, 2019
@codebytere @targos
src: fewer uses of NODE_USE_V8_PLATFORM
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax

@cjihrig cjihrig

@joyeecheung joyeecheung

@JungMinu JungMinu

@devsnek devsnek

Assignees
No one assigned
Labels
C++ author ready lib / src
Projects
None yet
Milestone
No milestone
7 participants
@codebytere @nodejs-github-bot @addaleax @cjihrig @joyeecheung @JungMinu @devsnek
You can’t perform that action at this time.