From 0bdc7f0b76df17898467270b095f3b4b70952f86 Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Wed, 7 Dec 2022 13:51:13 -0600 Subject: [PATCH 1/5] fix: limit the number of globals defined due to the v8 snapshot --- .../src/snapshot-require.ts | 24 ++- scripts/binary/smoke.js | 19 ++ .../v8-snapshot/src/generator/blueprint.ts | 166 +++++++++--------- 3 files changed, 116 insertions(+), 93 deletions(-) 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 6438ca54e50d..d30027c7ef90 100644 --- a/scripts/binary/smoke.js +++ b/scripts/binary/smoke.js @@ -288,6 +288,25 @@ 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).forEach((key) => { + if (!nodeGlobalKeys.includes(key)) { + extraKeys.push(key) + } + }) + + console.error(`extra keys in electron process: ${extraKeys}`) + } + + const allowList = ['regeneratorRuntime', '__core-js_shared__', 'getSnapshotResult'] + + 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/tooling/v8-snapshot/src/generator/blueprint.ts b/tooling/v8-snapshot/src/generator/blueprint.ts index 89cc9eb347a5..4f02cb70a908 100644 --- a/tooling/v8-snapshot/src/generator/blueprint.ts +++ b/tooling/v8-snapshot/src/generator/blueprint.ts @@ -109,101 +109,109 @@ 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 +}).call(this) `, - 'utf8', + 'utf8', ) const buffers = [wrapperOpen, customRequireDefinitions, wrapperClose] From 8ec5f3eaedc92fc1941e3518e23452b91da3d04f Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Wed, 7 Dec 2022 19:38:44 -0600 Subject: [PATCH 2/5] fix tests --- scripts/binary/smoke.js | 2 +- .../projects/e2e/cypress/e2e/simple_v8_snapshot.cy.js | 2 +- tooling/v8-snapshot/src/generator/blueprint.ts | 1 + tooling/v8-snapshot/test/utils/bundle.ts | 11 +---------- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/scripts/binary/smoke.js b/scripts/binary/smoke.js index d30027c7ef90..a132e6329497 100644 --- a/scripts/binary/smoke.js +++ b/scripts/binary/smoke.js @@ -304,7 +304,7 @@ const runIntegrityTest = async function (buildAppExecutable, buildAppDir, e2e) { console.error(`extra keys in electron process: ${extraKeys}`) } - const allowList = ['regeneratorRuntime', '__core-js_shared__', 'getSnapshotResult'] + 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`) } 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 4f02cb70a908..6d82f5bfc954 100644 --- a/tooling/v8-snapshot/src/generator/blueprint.ts +++ b/tooling/v8-snapshot/src/generator/blueprint.ts @@ -209,6 +209,7 @@ export function scriptFromBlueprint (config: BlueprintConfig): { value: ${supportTypeScript}, }, }) + generateSnapshot = null }).call(this) `, 'utf8', 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 } } From db44f638e77d0c8d356054e3ab2ea4cab3f4f00b Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Wed, 7 Dec 2022 22:26:08 -0600 Subject: [PATCH 3/5] slight refactoring --- scripts/binary/smoke.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/scripts/binary/smoke.js b/scripts/binary/smoke.js index a132e6329497..3ab72696bb05 100644 --- a/scripts/binary/smoke.js +++ b/scripts/binary/smoke.js @@ -293,12 +293,8 @@ const runIntegrityTest = async function (buildAppExecutable, buildAppDir, e2e) { 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).forEach((key) => { - if (!nodeGlobalKeys.includes(key)) { - extraKeys.push(key) - } + const extraKeys = Object.getOwnPropertyNames(global).filter((key) => { + return !nodeGlobalKeys.includes(key) }) console.error(`extra keys in electron process: ${extraKeys}`) From f7bf83762277fd1b98b40a1cd842cefe383e3fa2 Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Wed, 7 Dec 2022 23:10:27 -0600 Subject: [PATCH 4/5] add in all platforms for build --- .circleci/workflows.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index 6154fa8a414b..e83ed010ca83 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -28,7 +28,7 @@ mainBuildFilters: &mainBuildFilters only: - develop - /^release\/\d+\.\d+\.\d+$/ - - 'ryanm/fix/v8-improvements' + - 'ryanm/fix/issue-with-integrity-check' - 'mschile/windows_session' # usually we don't build Mac app - it takes a long time @@ -38,7 +38,7 @@ macWorkflowFilters: &darwin-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'ryanm/fix/v8-improvements', << pipeline.git.branch >> ] + - equal: [ 'ryanm/fix/issue-with-integrity-check', << pipeline.git.branch >> ] - equal: [ 'mschile/windows_session', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ @@ -47,7 +47,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'ryanm/fix/v8-improvements', << pipeline.git.branch >> ] + - equal: [ 'ryanm/fix/issue-with-integrity-check', << pipeline.git.branch >> ] - equal: [ 'mschile/windows_session', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ @@ -66,7 +66,7 @@ windowsWorkflowFilters: &windows-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'ryanm/fix/v8-improvements', << pipeline.git.branch >> ] + - equal: [ 'ryanm/fix/issue-with-integrity-check', << pipeline.git.branch >> ] - equal: [ 'mschile/windows_session', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ @@ -133,7 +133,7 @@ commands: - run: name: Check current branch to persist artifacts command: | - if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "ryanm/fix/v8-improvements" ]]; 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 From 1b9b2f5e0bfca772c5e8c6ff48f55e6b345a3c7d Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Thu, 8 Dec 2022 07:59:14 -0600 Subject: [PATCH 5/5] fix tests --- packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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'