From f9541aff10efa62ffdd15f34775c2761ba78adf3 Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Thu, 8 Dec 2022 09:35:58 -0600 Subject: [PATCH] fix: limit the number of globals defined due to the v8 snapshot (#25051) --- .circleci/workflows.yml | 10 +- .../cypress/e2e/e2ePluginSetup.ts | 6 +- .../src/snapshot-require.ts | 24 ++- scripts/binary/smoke.js | 15 ++ .../e2e/cypress/e2e/simple_v8_snapshot.cy.js | 2 +- .../v8-snapshot/src/generator/blueprint.ts | 167 +++++++++--------- tooling/v8-snapshot/test/utils/bundle.ts | 11 +- 7 files changed, 123 insertions(+), 112 deletions(-) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index c6fe1302d633..5f6185bcea4d 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -28,7 +28,7 @@ mainBuildFilters: &mainBuildFilters only: - develop - /^release\/\d+\.\d+\.\d+$/ - - 'ryanm/fix/cy-in-cy-and-v8-snapshots' + - 'ryanm/fix/issue-with-integrity-check' # usually we don't build Mac app - it takes a long time # but sometimes we want to really confirm we are doing the right thing @@ -37,7 +37,7 @@ macWorkflowFilters: &darwin-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'ryanm/fix/cy-in-cy-and-v8-snapshots', << pipeline.git.branch >> ] + - equal: [ 'ryanm/fix/issue-with-integrity-check', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -45,7 +45,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'ryanm/fix/cy-in-cy-and-v8-snapshots', << pipeline.git.branch >> ] + - equal: [ 'ryanm/fix/issue-with-integrity-check', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -63,7 +63,7 @@ windowsWorkflowFilters: &windows-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'ryanm/fix/cy-in-cy-and-v8-snapshots', << pipeline.git.branch >> ] + - equal: [ 'ryanm/fix/issue-with-integrity-check', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -129,7 +129,7 @@ commands: - run: name: Check current branch to persist artifacts command: | - if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "ryanm/fix/cy-in-cy-and-v8-snapshots" ]]; then + if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "ryanm/fix/issue-with-integrity-check" ]]; then echo "Not uploading artifacts or posting install comment for this branch." circleci-agent step halt fi diff --git a/packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts b/packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts index 09cd4ff25773..4af90b590380 100644 --- a/packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts +++ b/packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts @@ -47,9 +47,9 @@ chai.use(chaiSubset) chai.use(sinonChai) export async function e2ePluginSetup (on: Cypress.PluginEvents, config: Cypress.PluginConfigOptions) { - // @ts-ignore snapshotAuxiliaryData is injected by the snapshot script - if (typeof global.snapshotAuxiliaryData === 'undefined') { - throw new Error('snapshotAuxiliaryData is undefined. v8 snapshots are not being used in Cypress in Cypress') + // @ts-ignore getSnapshotResult is injected by the snapshot script + if (typeof global.getSnapshotResult === 'undefined') { + throw new Error('getSnapshotResult is undefined. v8 snapshots are not being used in Cypress in Cypress') } process.env.CYPRESS_INTERNAL_E2E_TESTING_SELF = 'true' diff --git a/packages/v8-snapshot-require/src/snapshot-require.ts b/packages/v8-snapshot-require/src/snapshot-require.ts index 83006733c1c6..5adf7df66f88 100644 --- a/packages/v8-snapshot-require/src/snapshot-require.ts +++ b/packages/v8-snapshot-require/src/snapshot-require.ts @@ -207,20 +207,16 @@ export function snapshotRequire ( let moduleNeedsReload: ModuleNeedsReload | undefined // @ts-ignore global snapshotAuxiliaryData - if (typeof snapshotAuxiliaryData !== 'undefined') { - // @ts-ignore global snapshotAuxiliaryData - resolverMap = snapshotAuxiliaryData.resolverMap - const dependencyMapArray: DependencyMapArray = - // @ts-ignore global snapshotAuxiliaryData - snapshotAuxiliaryData.dependencyMapArray - - // 5. Setup the module needs reload predicate with the dependency map - if (dependencyMapArray != null) { - moduleNeedsReload = createModuleNeedsReload( - dependencyMapArray, - projectBaseDir, - ) - } + resolverMap = sr.snapshotAuxiliaryData.resolverMap + // @ts-ignore global snapshotAuxiliaryData + const dependencyMapArray: DependencyMapArray = sr.snapshotAuxiliaryData.dependencyMapArray + + // 5. Setup the module needs reload predicate with the dependency map + if (dependencyMapArray != null) { + moduleNeedsReload = createModuleNeedsReload( + dependencyMapArray, + projectBaseDir, + ) } // 6. Setup the module key resolver with the resolver map diff --git a/scripts/binary/smoke.js b/scripts/binary/smoke.js index cbe38d75674a..063ad600ea24 100644 --- a/scripts/binary/smoke.js +++ b/scripts/binary/smoke.js @@ -287,6 +287,21 @@ const runIntegrityTest = async function (buildAppExecutable, buildAppDir, e2e) { await testAlteringEntryPoint('console.log("simple alteration")', 'Integrity check failed with expected stack length 9 but got 10') await testAlteringEntryPoint('console.log("accessing " + global.getSnapshotResult())', 'getSnapshotResult can only be called once') + + function compareGlobals () { + const childProcess = require('child_process') + const nodeGlobalKeys = JSON.parse(childProcess.spawnSync('node -p "const x = Object.getOwnPropertyNames(global);JSON.stringify(x)"', { shell: true, encoding: 'utf8' }).stdout) + + const extraKeys = Object.getOwnPropertyNames(global).filter((key) => { + return !nodeGlobalKeys.includes(key) + }) + + console.error(`extra keys in electron process: ${extraKeys}`) + } + + const allowList = ['regeneratorRuntime', '__core-js_shared__', 'getSnapshotResult', 'supportTypeScript'] + + await testAlteringEntryPoint(`(${compareGlobals.toString()})()`, `extra keys in electron process: ${allowList}\nIntegrity check failed with expected stack length 9 but got 10`) } const test = async function (buildAppExecutable, buildAppDir) { diff --git a/system-tests/projects/e2e/cypress/e2e/simple_v8_snapshot.cy.js b/system-tests/projects/e2e/cypress/e2e/simple_v8_snapshot.cy.js index affa47bc99b8..8e22b50fc85f 100644 --- a/system-tests/projects/e2e/cypress/e2e/simple_v8_snapshot.cy.js +++ b/system-tests/projects/e2e/cypress/e2e/simple_v8_snapshot.cy.js @@ -1,6 +1,6 @@ describe('simple v8 snapshot spec', () => { it('passes', () => { // Snapshot result needs to be undefined in the renderer process - expect(window.snapshotResult).to.be.undefined + expect(window.getSnapshotResult).to.be.undefined }) }) diff --git a/tooling/v8-snapshot/src/generator/blueprint.ts b/tooling/v8-snapshot/src/generator/blueprint.ts index 89cc9eb347a5..6d82f5bfc954 100644 --- a/tooling/v8-snapshot/src/generator/blueprint.ts +++ b/tooling/v8-snapshot/src/generator/blueprint.ts @@ -109,101 +109,110 @@ export function scriptFromBlueprint (config: BlueprintConfig): { const wrapperOpen = Buffer.from( ` -const PATH_SEP = '${pathSep}' -var snapshotAuxiliaryData = ${auxiliaryData} - -${integrityCheckSource || ''} - -function generateSnapshot() { - // - // - // - function cannotAccess(proto, prop) { - return function () { - throw 'Cannot access ' + proto + '.' + prop + ' during snapshot creation' +(function () { + const PATH_SEP = '${pathSep}' + ${integrityCheckSource || ''} + + function generateSnapshot() { + // + // + // + function cannotAccess(proto, prop) { + return function () { + throw 'Cannot access ' + proto + '.' + prop + ' during snapshot creation' + } } - } - function getPrevent(proto, prop) { - return { - get: cannotAccess(proto, prop) + function getPrevent(proto, prop) { + return { + get: cannotAccess(proto, prop) + } } - } - let process = {} - Object.defineProperties(process, { - platform: { - value: '${processPlatform}', - enumerable: false, - }, - argv: { - value: [], - enumerable: false, - }, - env: { - value: { - NODE_ENV: '${nodeEnv}', + let process = {} + Object.defineProperties(process, { + platform: { + value: '${processPlatform}', + enumerable: false, }, - enumerable: false, - }, - version: { - value: '${processNodeVersion}', - enumerable: false, - }, - versions: { - value: { node: '${processNodeVersion}' }, - enumerable: false, - }, - nextTick: getPrevent('process', 'nextTick') - }) + argv: { + value: [], + enumerable: false, + }, + env: { + value: { + NODE_ENV: '${nodeEnv}', + }, + enumerable: false, + }, + version: { + value: '${processNodeVersion}', + enumerable: false, + }, + versions: { + value: { node: '${processNodeVersion}' }, + enumerable: false, + }, + nextTick: getPrevent('process', 'nextTick') + }) - function get_process() { - return process - } - // - // - // + function get_process() { + return process + } + // + // + // - ${globals} - ${includeStrictVerifiers ? strictGlobals : ''} + ${globals} + ${includeStrictVerifiers ? strictGlobals : ''} `, 'utf8', ) const wrapperClose = Buffer.from( - ` - ${customRequire} - ${includeStrictVerifiers ? 'require.isStrict = true' : ''} + ` + ${customRequire} + ${includeStrictVerifiers ? 'require.isStrict = true' : ''} + + customRequire(${normalizedMainModuleRequirePath}, ${normalizedMainModuleRequirePath}) + const result = {} + Object.defineProperties(result, { + customRequire: { + writable: false, + value: customRequire + }, + setGlobals: { + writable: false, + value: ${setGlobals} + }, + snapshotAuxiliaryData: { + writable: false, + value: ${auxiliaryData}, + }, + }) + return result + } - customRequire(${normalizedMainModuleRequirePath}, ${normalizedMainModuleRequirePath}) - const result = {} - Object.defineProperties(result, { - customRequire: { + let numberOfGetSnapshotResultCalls = 0 + const snapshotResult = generateSnapshot.call({}) + Object.defineProperties(this, { + getSnapshotResult: { writable: false, - value: customRequire + value: function () { + if (numberOfGetSnapshotResultCalls > 0) { + throw new Error('getSnapshotResult can only be called once') + } + numberOfGetSnapshotResultCalls++ + return snapshotResult + }, }, - setGlobals: { + supportTypeScript: { writable: false, - value: ${setGlobals} - } + value: ${supportTypeScript}, + }, }) - return result -} - -let numberOfGetSnapshotResultCalls = 0 -const snapshotResult = generateSnapshot.call({}) -Object.defineProperty(this, 'getSnapshotResult', { - writable: false, - value: function () { - if (numberOfGetSnapshotResultCalls > 0) { - throw new Error('getSnapshotResult can only be called once') - } - numberOfGetSnapshotResultCalls++ - return snapshotResult - }, -}) -var supportTypeScript = ${supportTypeScript} -generateSnapshot = null + generateSnapshot = null +}).call(this) `, - 'utf8', + 'utf8', ) const buffers = [wrapperOpen, customRequireDefinitions, wrapperClose] diff --git a/tooling/v8-snapshot/test/utils/bundle.ts b/tooling/v8-snapshot/test/utils/bundle.ts index 642dff6d7106..9e14335df2de 100644 --- a/tooling/v8-snapshot/test/utils/bundle.ts +++ b/tooling/v8-snapshot/test/utils/bundle.ts @@ -14,14 +14,5 @@ export function readSnapshotResult (cacheDir: string) { const metaFile = path.join(cacheDir, 'snapshot-meta.json') const meta = fs.readJSONSync(metaFile) - const snapshotFile = path.join(cacheDir, 'snapshot.js') - - const snapshotFileContent = fs.readFileSync(snapshotFile, 'utf8') - const sourcemapComment = snapshotFileContent.split('\n').pop() - - const { snapshotResult, snapshotAuxiliaryData } = eval( - `(function () {\n${snapshotFileContent};\n return { snapshotResult, snapshotAuxiliaryData };}).bind({})()`, - ) - - return { meta, snapshotResult, snapshotAuxiliaryData, sourcemapComment } + return { meta } }