Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: limit the number of globals defined due to the v8 snapshot #25051

Merged
merged 7 commits into from Dec 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions .circleci/workflows.yml
Expand Up @@ -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
Expand All @@ -37,15 +37,15 @@ 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 >>
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 >>
Expand All @@ -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 >>
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts
Expand Up @@ -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'
Expand Down
24 changes: 10 additions & 14 deletions packages/v8-snapshot-require/src/snapshot-require.ts
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions scripts/binary/smoke.js
Expand Up @@ -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) {
Expand Down
@@ -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
})
})
167 changes: 88 additions & 79 deletions tooling/v8-snapshot/src/generator/blueprint.ts
Expand Up @@ -109,101 +109,110 @@ export function scriptFromBlueprint (config: BlueprintConfig): {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is more easily reviewed by ignoring whitespace changes.

image

const wrapperOpen = Buffer.from(
`
const PATH_SEP = '${pathSep}'
var snapshotAuxiliaryData = ${auxiliaryData}

${integrityCheckSource || ''}

function generateSnapshot() {
//
// <process>
//
function cannotAccess(proto, prop) {
return function () {
throw 'Cannot access ' + proto + '.' + prop + ' during snapshot creation'
(function () {
const PATH_SEP = '${pathSep}'
${integrityCheckSource || ''}

function generateSnapshot() {
//
// <process>
//
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
}
//
// </process>
//
function get_process() {
return process
}
//
// </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]
Expand Down
11 changes: 1 addition & 10 deletions tooling/v8-snapshot/test/utils/bundle.ts
Expand Up @@ -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 }
}