From 7c6fc71d895dafaee921460cb4ea87dffc7fda06 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 25 Feb 2023 00:57:40 +0100 Subject: [PATCH 1/3] lib: make sure globals can be loaded after globalThis is freezed Since we made some of the globals lazily loaded to reduce startup overhead, and tried to update then as data properties once loaded, the lazily-loaded globals can no longer be loaded once globalThis is freezed. Fix this by not attempting to overwrite the property on loading these globals (so the property descriptors won't be spec-compliant in this case, but at least it's a good compromise between breakages and startup overhead). Note that certain globals that come from undici are still broken because undici attempts to define a global dispatcher symbol on globalThis. This is reflected in the test. --- lib/internal/util.js | 6 +++++- test/parallel/test-frozen-global.js | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-frozen-global.js diff --git a/lib/internal/util.js b/lib/internal/util.js index 5665923a2cd297..ae1bda66d1022f 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -15,6 +15,7 @@ const { ObjectGetOwnPropertyDescriptors, ObjectGetPrototypeOf, ObjectFreeze, + ObjectIsFrozen, ObjectPrototypeHasOwnProperty, ObjectSetPrototypeOf, ObjectValues, @@ -532,7 +533,10 @@ function defineLazyProperties(target, id, keys, enumerable = true) { mod ??= require(id); if (lazyLoadedValue === undefined) { lazyLoadedValue = mod[key]; - set(lazyLoadedValue); + if (!ObjectIsFrozen(this)) { + // Replace it with a data property. + set(lazyLoadedValue); + } } return lazyLoadedValue; } diff --git a/test/parallel/test-frozen-global.js b/test/parallel/test-frozen-global.js new file mode 100644 index 00000000000000..2f21b7e2aeb00f --- /dev/null +++ b/test/parallel/test-frozen-global.js @@ -0,0 +1,24 @@ +'use strict'; + +// This tests that the globals can still be loaded after globalThis is freezed. + +require('../common'); +const assert = require('assert'); + +Object.freeze(globalThis); +const keys = Reflect.ownKeys(globalThis).filter((k) => typeof k === 'string'); + +// These failures come from undici. We can remove them once they are fixed. +const knownFailures = new Set(['FormData', 'Headers', 'Request', 'Response']); +const failures = new Map(); +const success = new Map(); +for (const key of keys) { + try { + success.set(key, globalThis[key]); + } catch (e) { + failures.set(key, e); + } +} + +console.log(failures); +assert.deepStrictEqual(new Set(failures.keys()), knownFailures); From 8392842fd22d7151e3e9647cccbf3ab88e6047ec Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 28 Feb 2023 00:23:00 +0100 Subject: [PATCH 2/3] fixup! lib: make sure globals can be loaded after globalThis is freezed --- lib/internal/util.js | 6 +++--- test/parallel/test-frozen-global.js | 1 - test/parallel/test-sealed-global.js | 23 +++++++++++++++++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-sealed-global.js diff --git a/lib/internal/util.js b/lib/internal/util.js index ae1bda66d1022f..2b8b2117d52efb 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -15,7 +15,7 @@ const { ObjectGetOwnPropertyDescriptors, ObjectGetPrototypeOf, ObjectFreeze, - ObjectIsFrozen, + ObjectIsSealed, ObjectPrototypeHasOwnProperty, ObjectSetPrototypeOf, ObjectValues, @@ -530,10 +530,10 @@ function defineLazyProperties(target, id, keys, enumerable = true) { value: `set ${key}`, }); function get() { - mod ??= require(id); if (lazyLoadedValue === undefined) { + mod ??= require(id); lazyLoadedValue = mod[key]; - if (!ObjectIsFrozen(this)) { + if (!ObjectIsSealed(this)) { // Replace it with a data property. set(lazyLoadedValue); } diff --git a/test/parallel/test-frozen-global.js b/test/parallel/test-frozen-global.js index 2f21b7e2aeb00f..512afc7d1a8ec2 100644 --- a/test/parallel/test-frozen-global.js +++ b/test/parallel/test-frozen-global.js @@ -20,5 +20,4 @@ for (const key of keys) { } } -console.log(failures); assert.deepStrictEqual(new Set(failures.keys()), knownFailures); diff --git a/test/parallel/test-sealed-global.js b/test/parallel/test-sealed-global.js new file mode 100644 index 00000000000000..27e84b5ea44933 --- /dev/null +++ b/test/parallel/test-sealed-global.js @@ -0,0 +1,23 @@ +'use strict'; + +// This tests that the globals can still be loaded after globalThis is freezed. + +require('../common'); +const assert = require('assert'); + +Object.seal(globalThis); +const keys = Reflect.ownKeys(globalThis).filter((k) => typeof k === 'string'); + +// These failures come from undici. We can remove them once they are fixed. +const knownFailures = new Set(['FormData', 'Headers', 'Request', 'Response']); +const failures = new Map(); +const success = new Map(); +for (const key of keys) { + try { + success.set(key, globalThis[key]); + } catch (e) { + failures.set(key, e); + } +} + +assert.deepStrictEqual(new Set(failures.keys()), knownFailures); From fffbcbdd617820b36f0f1986e2b7267450b62f94 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 28 Feb 2023 15:54:11 +0100 Subject: [PATCH 3/3] fixup! lib: make sure globals can be loaded after globalThis is sealed #46831 Co-authored-by: Antoine du Hamel --- test/parallel/test-frozen-global.js | 2 +- test/parallel/test-sealed-global.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-frozen-global.js b/test/parallel/test-frozen-global.js index 512afc7d1a8ec2..8284a60c54cb18 100644 --- a/test/parallel/test-frozen-global.js +++ b/test/parallel/test-frozen-global.js @@ -1,6 +1,6 @@ 'use strict'; -// This tests that the globals can still be loaded after globalThis is freezed. +// This tests that the globals can still be loaded after globalThis is frozen. require('../common'); const assert = require('assert'); diff --git a/test/parallel/test-sealed-global.js b/test/parallel/test-sealed-global.js index 27e84b5ea44933..9eb96ccdf03edb 100644 --- a/test/parallel/test-sealed-global.js +++ b/test/parallel/test-sealed-global.js @@ -1,6 +1,6 @@ 'use strict'; -// This tests that the globals can still be loaded after globalThis is freezed. +// This tests that the globals can still be loaded after globalThis is sealed. require('../common'); const assert = require('assert');