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.

Sign up for GitHub

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

Jump to bottom

lib: make the builtInObjects set deterministic during bootstrap #33049

Closed
wants to merge 2 commits into from
Closed

lib: make the builtInObjects set deterministic during bootstrap #33049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bedf4d
lib: make the builtInObjects set deterministic during bootstrap
joyeecheung Apr 24, 2020
5f01a4f
fixup! lib: make the builtInObjects set deterministic during bootstrap
joyeecheung Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/internal/bootstrap/pre_execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const {
Map,
ObjectDefineProperty,
ObjectGetOwnPropertyNames,
SafeWeakMap,
} = primordials;

Expand Down Expand Up @@ -178,6 +179,13 @@ function setupDebugEnv() {
if (getOptionValue('--expose-internals')) {
require('internal/bootstrap/loaders').NativeModule.exposeInternals();
}

const { builtInObjects } = require('internal/util/inspect');
// The content of this list depends on e.g. V8 flags, as well as
// what gets added by Node.js to the global object.
for (const key of ObjectGetOwnPropertyNames(globalThis)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This now contains these added by Node.js

  'global',
  'process',
  'Buffer',
  'URL',
  'URLSearchParams',
  'TextEncoder',
  'TextDecoder',
  'clearInterval',
  'clearTimeout',
  'setInterval',
  'setTimeout',
  'queueMicrotask',
  'clearImmediate',
  'setImmediate'

We still need this loop if we want to pick up what get's added by V8 through harmony flags (like WeakRefs). I wonder which of these do we want to skip? @BridgeAR

Copy link
Member

Choose a reason for hiding this comment

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

We mostly do not need those. The only ones that we should keep are global classes that have a prototype. The current RegExp is not matching exactly what we actually need. I just checked and this seems to ideally model what is needed for inspect:

function isConstructor(name) {
  try {
    Reflect.construct(String, [], globalThis[name]);
  } catch {
    return false;
  }
  return globalThis[name].prototype !== undefined && name[0].toUpperCase() === name[0];
}

builtInObjects.add(key);
}
}

// This has to be called after initializeReport() is called
Expand Down
58 changes: 54 additions & 4 deletions lib/internal/util/inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,58 @@ const typedArraySizeGetter = uncurryThis(

let hexSlice;

const builtInObjects = new Set(
ObjectGetOwnPropertyNames(global).filter((e) => /^[A-Z][a-zA-Z0-9]+$/.test(e))
);
// Make this list explicit instead of filtering over globalThis so that
// this list is deterministic during the bootstrap regardless of:
// a) Whether this file is required when the context is going to be
// serialized, or what the V8 flags are. V8 does not add
// SharedArrayBuffer, Atomics, WebAssembly, and other flaggable
// globals e.g. harmony ones to a context that will be serialized.
// b) Whether this file is required before or after bootstrap
// (Node.js adds things like Buffer, URL, etc.)
const builtInObjects = new Set([
'Object',
'Function',
'Array',
'Number',
'Infinity',
'NaN',
Comment on lines +146 to +147
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Infinity',
'NaN',

'Boolean',
'String',
'Symbol',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Symbol',

'Date',
'Promise',
'RegExp',
'Error',
'EvalError',
'RangeError',
'ReferenceError',
'SyntaxError',
'TypeError',
'URIError',
'JSON',
'Math',
'Intl',
Comment on lines +161 to +163
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'JSON',
'Math',
'Intl',

'ArrayBuffer',
'Uint8Array',
'Int8Array',
'Uint16Array',
'Int16Array',
'Uint32Array',
'Int32Array',
'Float32Array',
'Float64Array',
'Uint8ClampedArray',
'BigUint64Array',
'BigInt64Array',
'DataView',
'Map',
'BigInt',
'Set',
'WeakMap',
'WeakSet',
'Proxy',
'Reflect'
Comment on lines +182 to +183
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Proxy',
'Reflect'
'SharedArrayBuffer'

]);

// https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot
const isUndetectableObject = (v) => typeof v === 'undefined' && v !== undefined;
Expand Down Expand Up @@ -2033,5 +2082,6 @@ module.exports = {
formatWithOptions,
getStringWidth,
inspectDefaultOptions,
stripVTControlCharacters
stripVTControlCharacters,
builtInObjects,
};