src: add buildflag to force context-aware addons #29631

codebytere · 2019-09-20T16:25:24Z

This PR adds a new cli flag --force-context-aware whose purpose is to prevent usage of native node addons that aren't context aware. In an embedder context, loading native addons ahas always been challenging due to the need to rebuild, to link to the right node.lib/dll, and to ensure the NODE_MODULE_VERSION is correct.

Since Node doesn't let you load multiple instances of a native module in the same process, even in different v8 contexts, this creates a host of problems for multi-process framework like Electron.

This new flag would allow Electron and other embedders to enforce restrictions more cleanly and prevent unexpected behavior.

More context for embedders can be found here: #18397

cc @MarshallOfSound

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

doc/api/cli.md

addaleax

This LGTM, although a test would be great.

Since Node doesn't let you load multiple instances of a native module in the same process, even in different v8 contexts, this creates a host of problems for multi-process framework like Electron.

Just to make sure there’s no misunderstanding, this is the case for only non-context-aware addons, and that is the point of this PR, right?

src/env.cc

src/node_binding.cc

src/node_options.cc

codebytere · 2019-09-20T23:43:26Z

Yes indeed! Only the case for non-context-aware addons. I'll add a test as soon as i can :)

doc/api/errors.md

src/node_binding.cc

codebytere · 2019-09-21T04:03:58Z

@addaleax i think (?) my test should cover it, it's definitely not the prettiest test i've ever written so please let me know if you think there's a better pattern for testing this kind of cli flag!

addaleax

Yeah, the test looks good to me too :)

nodejs-github-bot · 2019-09-21T11:56:07Z

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

bnoordhuis

A test that checks loading a context-aware add-on works would be nice too. You can co-opt one of the other context-aware add-on tests if you like, e.g.:

diff --git a/test/addons/zlib-binding/test.js b/test/addons/zlib-binding/test.js
index b50817aa5d..7a488c9a3b 100644
--- a/test/addons/zlib-binding/test.js
+++ b/test/addons/zlib-binding/test.js
@@ -1,3 +1,4 @@
+// Flags: --force-context-aware
 'use strict';
 
 const common = require('../../common');

doc/api/cli.md

test/addons/force-context-aware/binding.cc

test/addons/force-context-aware/test.js

nodejs-github-bot · 2019-09-24T06:06:12Z

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

test/addons/force-context-aware/binding.cc

nodejs-github-bot · 2019-09-25T04:02:40Z

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

nodejs-github-bot · 2019-09-25T05:10:36Z

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

nodejs-github-bot · 2019-09-25T05:11:06Z

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

PR-URL: #29631 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Trott · 2019-09-25T05:51:28Z

Landed in 5058c7f...f120e6d

jasnell · 2019-09-25T16:24:58Z

doc/api/errors.md

+<a id="ERR_NON_CONTEXT_AWARE_DISABLED"></a>
+### ERR_NON_CONTEXT_AWARE_DISABLED
+
+A non-context-aware native addon was loaded in a process that disallows them.


including a link here for context-aware like in the cli.md file would be helpful

jasnell · 2019-09-25T16:27:07Z

src/node_options.cc

@@ -393,6 +393,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
            "silence all process warnings",
            &EnvironmentOptions::no_warnings,
            kAllowedInEnvironment);
+  AddOption("--force-context-aware",
+            "disable loading non-context-aware addons",


extremely minor take it or leave it nit.. but the double negative here feels odd ... "disable ... non" ... perhaps "allow only context-aware addons" as an alternative?

PR-URL: #29631 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * build: * Add `--force-context-aware` flag to prevent usage of native node addons that aren't context aware #29631 * deprecations: * Add documentation-only deprecation for `process._tickCallback()` #29781 * esm: * Using JSON modules is experimental again #29754 * fs: * Introduce `opendir()` and `fs.Dir` to iterate through directories #29349 * process: * Add source-map support to stack traces by using `--source-map-support` #29564 * tls: * Honor `pauseOnConnect` option #29635 * Add option for private keys for OpenSSL engines #28973 PR-URL: #29919

See nodejs/node#29631 electron/electron#18397

nodejs/node#29631

See nodejs/node#29631 electron/electron#18397

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 Sep 20, 2019

codebytere force-pushed the warn-prevent-context-aware branch 2 times, most recently from aaa73db to 469bdbe Compare September 20, 2019 16:31

richardlau reviewed Sep 20, 2019

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

codebytere force-pushed the warn-prevent-context-aware branch from 469bdbe to 23733d3 Compare September 20, 2019 17:06

addaleax approved these changes Sep 20, 2019

View reviewed changes

src/env.cc Outdated Show resolved Hide resolved

src/node_binding.cc Outdated Show resolved Hide resolved

src/node_options.cc Outdated Show resolved Hide resolved

addaleax added addons Issues and PRs related to native addons. semver-minor PRs that contain new features and should be released in the next minor version. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 20, 2019

codebytere force-pushed the warn-prevent-context-aware branch from 23733d3 to 4d315c3 Compare September 20, 2019 23:42

addaleax approved these changes Sep 20, 2019

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

src/node_binding.cc Show resolved Hide resolved

codebytere force-pushed the warn-prevent-context-aware branch 2 times, most recently from 6c5909f to 93d2420 Compare September 21, 2019 04:02

codebytere force-pushed the warn-prevent-context-aware branch from 93d2420 to 93dc647 Compare September 21, 2019 04:04

src: add buildflag to force context-aware addons

119d31f

codebytere force-pushed the warn-prevent-context-aware branch 3 times, most recently from 3ac2c4a to c5efc2a Compare September 21, 2019 05:01

addaleax approved these changes Sep 21, 2019

View reviewed changes

bnoordhuis approved these changes Sep 23, 2019

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

test/addons/force-context-aware/binding.cc Outdated Show resolved Hide resolved

test/addons/force-context-aware/test.js Outdated Show resolved Hide resolved

codebytere force-pushed the warn-prevent-context-aware branch from c5efc2a to 094d888 Compare September 23, 2019 16:21

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2019

bnoordhuis reviewed Sep 24, 2019

View reviewed changes

test/addons/force-context-aware/binding.cc Outdated Show resolved Hide resolved

test: --force-context-aware cli flag

bcef054

codebytere force-pushed the warn-prevent-context-aware branch from 094d888 to bcef054 Compare September 24, 2019 10:59

bnoordhuis approved these changes Sep 24, 2019

View reviewed changes

Trott pushed a commit that referenced this pull request Sep 25, 2019

src: add buildflag to force context-aware addons

3c17f16

PR-URL: #29631 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Trott pushed a commit that referenced this pull request Sep 25, 2019

test: --force-context-aware cli flag

f120e6d

PR-URL: #29631 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Trott closed this Sep 25, 2019

codebytere deleted the warn-prevent-context-aware branch September 25, 2019 10:46

jasnell reviewed Sep 25, 2019

View reviewed changes

BridgeAR pushed a commit that referenced this pull request Oct 9, 2019

src: add buildflag to force context-aware addons

6bfe8f4

PR-URL: #29631 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

BridgeAR pushed a commit that referenced this pull request Oct 9, 2019

test: --force-context-aware cli flag

54ef0fd

PR-URL: #29631 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

BridgeAR mentioned this pull request Oct 10, 2019

v12.12.0 proposal #29919

Merged

dhritzkiv added a commit to stackgl/headless-gl that referenced this pull request Dec 31, 2019

Make gl context aware

d6cc49a

See nodejs/node#29631 electron/electron#18397

dhritzkiv added a commit to stackgl/headless-gl that referenced this pull request Jan 7, 2020

Make gl context aware

b2a9cee

See nodejs/node#29631 electron/electron#18397

dhritzkiv mentioned this pull request Jan 7, 2020

Make headless-gl context aware stackgl/headless-gl#182

Open

3 tasks

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update warn/force context-aware patch

371fddd

nodejs/node#29631

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update warn/force context-aware patch

2b841ad

nodejs/node#29631

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update warn/force context-aware patch

9d2414d

nodejs/node#29631

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update warn/force context-aware patch

df0d826

nodejs/node#29631

jacobthesumurai pushed a commit to noblesamurai/headless-gl that referenced this pull request Jul 9, 2021

Make gl context aware

dd2119a

See nodejs/node#29631 electron/electron#18397

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add buildflag to force context-aware addons #29631

src: add buildflag to force context-aware addons #29631

codebytere commented Sep 20, 2019 •

edited

Loading

addaleax left a comment

codebytere commented Sep 20, 2019

codebytere commented Sep 21, 2019

addaleax left a comment

nodejs-github-bot commented Sep 21, 2019

bnoordhuis left a comment

nodejs-github-bot commented Sep 24, 2019

nodejs-github-bot commented Sep 25, 2019

nodejs-github-bot commented Sep 25, 2019

nodejs-github-bot commented Sep 25, 2019

Trott commented Sep 25, 2019

jasnell Sep 25, 2019

jasnell Sep 25, 2019

src: add buildflag to force context-aware addons #29631

src: add buildflag to force context-aware addons #29631

Conversation

codebytere commented Sep 20, 2019 • edited Loading

Checklist

addaleax left a comment

Choose a reason for hiding this comment

codebytere commented Sep 20, 2019

codebytere commented Sep 21, 2019

addaleax left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 21, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 24, 2019

nodejs-github-bot commented Sep 25, 2019

nodejs-github-bot commented Sep 25, 2019

nodejs-github-bot commented Sep 25, 2019

Trott commented Sep 25, 2019

jasnell Sep 25, 2019

Choose a reason for hiding this comment

jasnell Sep 25, 2019

Choose a reason for hiding this comment

codebytere commented Sep 20, 2019 •

edited

Loading