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

lib: save primordials during bootstrap and use it in builtins #25816

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

This patches changes the safe_globals internal module into a
script that gets run during bootstrap and saves JavaScript builtins
(primordials) into an object that is available for all other builtin
modules to access lexically later.

Refs: #18795

cc @hashseed @devsnek @bmeck

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

This patches changes the `safe_globals` internal module into a
script that gets run during bootstrap and saves JavaScript builtins
(primordials) into an object that is available for all other builtin
modules to access lexically later.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 30, 2019
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

can you add a test demonstrating that modifying Object.defineProperty or something doesn't break internal code?

@joyeecheung
Copy link
Member Author

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

Adding them to the bootstrap seems fine, but we still cannot use methods via . if we are wanting to have the original function.

lib/internal/bootstrap/loaders.js Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 30, 2019

can you add a test demonstrating that modifying Object.defineProperty or something doesn't break internal code?

@devsnek Do you have a good API to test this in mind? This test would depend on the implementation details of the API and if the API being tested is changed we would have to look for another one that uses these globals after user code execution starts. Or is there already any tests for the previous safe_globals? That would probably be enough to demonstrate this.

lib/internal/error-serdes.js Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2019
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Landed in 39d9221

@joyeecheung joyeecheung closed this Feb 1, 2019
pull bot pushed a commit to zys-contrib/node that referenced this pull request Feb 1, 2019
This patches changes the `safe_globals` internal module into a
script that gets run during bootstrap and saves JavaScript builtins
(primordials) into an object that is available for all other builtin
modules to access lexically later.

PR-URL: nodejs#25816
Refs: nodejs#18795
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@addaleax
Copy link
Member

addaleax commented Feb 1, 2019

@joyeecheung This also needs to be backported to v11.x manually

targos pushed a commit that referenced this pull request Feb 10, 2019
This patches changes the `safe_globals` internal module into a
script that gets run during bootstrap and saves JavaScript builtins
(primordials) into an object that is available for all other builtin
modules to access lexically later.

PR-URL: #25816
Refs: #18795
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
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. 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.

6 participants