From 87d7845b4f676ba0674e1c745b44351e5cd64aaa Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 18 Aug 2022 21:05:31 +0800 Subject: [PATCH] bootstrap: fixup Error.stackTraceLimit for user-land snapshot It's difficult for V8 to handle Error.stackTraceLimit in the snapshot, so delete it from the Error constructor if it's present before snapshot serialization, and re-install it after deserialization. In addition try not to touch it from our internals during snapshot building in the first place by updating isErrorStackTraceLimitWritable(). PR-URL: https://github.com/nodejs/node/pull/44203 Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel --- lib/internal/errors.js | 6 ++++ lib/internal/main/mksnapshot.js | 34 +++++++++++++++++++++-- lib/internal/v8/startup_snapshot.js | 2 -- test/parallel/test-snapshot-api.js | 6 ++++ test/parallel/test-snapshot-typescript.js | 7 +---- test/parallel/test-snapshot-warning.js | 7 +---- 6 files changed, 46 insertions(+), 16 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index bb981290168c8c..de861b813a4fe9 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -191,6 +191,12 @@ function lazyBuffer() { } function isErrorStackTraceLimitWritable() { + // Do no touch Error.stackTraceLimit as V8 would attempt to install + // it again during deserialization. + if (require('v8').startupSnapshot.isBuildingSnapshot()) { + return false; + } + const desc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit'); if (desc === undefined) { return ObjectIsExtensible(Error); diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index fdf0e021204858..0b373c6dd8cea2 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -2,8 +2,11 @@ const { Error, + ObjectDefineProperty, + ObjectGetOwnPropertyDescriptor, + ObjectSetPrototypeOf, + SafeArrayIterator, SafeSet, - SafeArrayIterator } = primordials; const binding = internalBinding('mksnapshot'); @@ -125,7 +128,21 @@ function main() { const source = readFileSync(file, 'utf-8'); const serializeMainFunction = compileSerializeMain(filename, source); - require('internal/v8/startup_snapshot').initializeCallbacks(); + const { + initializeCallbacks, + namespace: { + addSerializeCallback, + addDeserializeCallback, + }, + } = require('internal/v8/startup_snapshot'); + initializeCallbacks(); + + let stackTraceLimitDesc; + addDeserializeCallback(() => { + if (stackTraceLimitDesc !== undefined) { + ObjectDefineProperty(Error, 'stackTraceLimit', stackTraceLimitDesc); + } + }); if (getOptionValue('--inspect-brk')) { internalBinding('inspector').callAndPauseOnStart( @@ -134,6 +151,19 @@ function main() { } else { serializeMainFunction(requireForUserSnapshot, filename, dirname); } + + addSerializeCallback(() => { + stackTraceLimitDesc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit'); + + if (stackTraceLimitDesc !== undefined) { + // We want to use null-prototype objects to not rely on globally mutable + // %Object.prototype%. + ObjectSetPrototypeOf(stackTraceLimitDesc, null); + process._rawDebug('Deleting Error.stackTraceLimit from the snapshot. ' + + 'It will be re-installed after deserialization'); + delete Error.stackTraceLimit; + } + }); } main(); diff --git a/lib/internal/v8/startup_snapshot.js b/lib/internal/v8/startup_snapshot.js index 86bee8749566d7..655cc1903e85ee 100644 --- a/lib/internal/v8/startup_snapshot.js +++ b/lib/internal/v8/startup_snapshot.js @@ -54,8 +54,6 @@ function runSerializeCallbacks() { const { 0: callback, 1: data } = serializeCallbacks.shift(); callback(data); } - // Remove the hooks from the snapshot. - require('v8').startupSnapshot = undefined; } function addSerializeCallback(callback, data) { diff --git a/test/parallel/test-snapshot-api.js b/test/parallel/test-snapshot-api.js index d6599ceab4199c..49f764d2cb43bd 100644 --- a/test/parallel/test-snapshot-api.js +++ b/test/parallel/test-snapshot-api.js @@ -10,6 +10,12 @@ const fixtures = require('../common/fixtures'); const path = require('path'); const fs = require('fs'); +const v8 = require('v8'); + +// By default it should be false. We'll test that it's true in snapshot +// building mode in the fixture. +assert(!v8.startupSnapshot.isBuildingSnapshot()); + tmpdir.refresh(); const blobPath = path.join(tmpdir.path, 'snapshot.blob'); const entry = fixtures.path('snapshot', 'v8-startup-snapshot-api.js'); diff --git a/test/parallel/test-snapshot-typescript.js b/test/parallel/test-snapshot-typescript.js index bf9baf25153151..0d0832648dfe2b 100644 --- a/test/parallel/test-snapshot-typescript.js +++ b/test/parallel/test-snapshot-typescript.js @@ -2,12 +2,7 @@ // This tests the TypeScript compiler in the snapshot. -const common = require('../common'); - -if (process.features.debug) { - common.skip('V8 snapshot does not work with mutated globals yet: ' + - 'https://bugs.chromium.org/p/v8/issues/detail?id=12772'); -} +require('../common'); const assert = require('assert'); const { spawnSync } = require('child_process'); diff --git a/test/parallel/test-snapshot-warning.js b/test/parallel/test-snapshot-warning.js index f7defe1d5b3bf4..357749ad8e6875 100644 --- a/test/parallel/test-snapshot-warning.js +++ b/test/parallel/test-snapshot-warning.js @@ -4,12 +4,7 @@ // during snapshot serialization and installed again during // deserialization. -const common = require('../common'); - -if (process.features.debug) { - common.skip('V8 snapshot does not work with mutated globals yet: ' + - 'https://bugs.chromium.org/p/v8/issues/detail?id=12772'); -} +require('../common'); const assert = require('assert'); const { spawnSync } = require('child_process');