From d78a717bc44aa01a40e7fad3777eb1ea7ad2047b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 10 Aug 2022 18:39:12 +0800 Subject: [PATCH 1/4] 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(). --- lib/internal/errors.js | 6 +++++ lib/internal/main/mksnapshot.js | 31 +++++++++++++++++++++-- lib/internal/v8/startup_snapshot.js | 2 -- test/parallel/test-snapshot-typescript.js | 7 +---- test/parallel/test-snapshot-warning.js | 7 +---- 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index df079d2952b47e..1f5c278168357f 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 c4ea3a06cfc378..5f00d8a0eca821 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -3,7 +3,10 @@ const { Error, SafeSet, - SafeArrayIterator + SafeArrayIterator, + ObjectPrototypeHasOwnProperty, + ObjectGetOwnPropertyDescriptor, + ObjectDefineProperty } = 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,16 @@ function main() { } else { serializeMainFunction(requireForUserSnapshot, filename, dirname); } + + addSerializeCallback(() => { + if (ObjectPrototypeHasOwnProperty(Error, 'stackTraceLimit')) { + stackTraceLimitDesc = + ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit'); + 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-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'); From 6df1fae71dcd59b8114fdc567614c445d7a7c598 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 11 Aug 2022 12:29:52 +0800 Subject: [PATCH 2/4] fixup! bootstrap: fixup Error.stackTraceLimit for user-land snapshot Co-authored-by: Antoine du Hamel --- lib/internal/main/mksnapshot.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index 5f00d8a0eca821..f4cfb9eba42ca5 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -153,9 +153,10 @@ function main() { } addSerializeCallback(() => { - if (ObjectPrototypeHasOwnProperty(Error, 'stackTraceLimit')) { - stackTraceLimitDesc = - ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit'); + stackTraceLimitDesc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit'); + + if (stackTraceLimitDesc !== undefined) { + ObjectSetPrototypeOf(stackTraceLimitDesc, null); process._rawDebug('Deleting Error.stackTraceLimit from the snapshot. ' + 'It will be re-installed after deserialization'); delete Error.stackTraceLimit; From 86dfc54361f25f8a1e3b3bcf6042626f114297f8 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 11 Aug 2022 15:09:21 +0800 Subject: [PATCH 3/4] fixup! fixup! bootstrap: fixup Error.stackTraceLimit for user-land snapshot --- lib/internal/main/mksnapshot.js | 4 ++-- test/parallel/test-snapshot-api.js | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index f4cfb9eba42ca5..1c0f2d8de74880 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -4,9 +4,9 @@ const { Error, SafeSet, SafeArrayIterator, - ObjectPrototypeHasOwnProperty, ObjectGetOwnPropertyDescriptor, - ObjectDefineProperty + ObjectDefineProperty, + ObjectSetPrototypeOf } = primordials; const binding = internalBinding('mksnapshot'); 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'); From 082f6656b958a834939124cc98590952a57d36a0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 17 Aug 2022 23:46:16 +0800 Subject: [PATCH 4/4] fixup! bootstrap: fixup Error.stackTraceLimit for user-land snapshot Co-authored-by: Antoine du Hamel --- lib/internal/main/mksnapshot.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index 1c0f2d8de74880..b91ef29ef14da4 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -2,11 +2,11 @@ const { Error, - SafeSet, - SafeArrayIterator, - ObjectGetOwnPropertyDescriptor, ObjectDefineProperty, - ObjectSetPrototypeOf + ObjectGetOwnPropertyDescriptor, + ObjectSetPrototypeOf, + SafeArrayIterator, + SafeSet, } = primordials; const binding = internalBinding('mksnapshot'); @@ -156,6 +156,8 @@ function main() { 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');