From 5d418c5dab732b8d92fbf81fd127695977d807c3 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 16 Nov 2022 01:48:56 -0800 Subject: [PATCH] fix: abort ShipIt installation attempt at the final mile if the app is running (#36362) fix: abort ShipIt installation attempt at the final mile if the app is running (#36130) * fix: abort ShipIt installation attempt at the final mile if the app is running * chore: remove only * Update patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch Co-authored-by: Jeremy Rose * chore: update patches * spec: make the ShipIt process lister helper more resilient Co-authored-by: Jeremy Rose Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Co-authored-by: Jeremy Rose Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> --- patches/squirrel.mac/.patches | 1 + ...empt_at_the_final_mile_if_the_app_is.patch | 75 ++++++++++++++++++ spec/api-autoupdater-darwin-spec.ts | 76 +++++++++++++++++++ spec/fixtures/auto-update/update/index.js | 52 +++++++------ spec/package.json | 1 + spec/yarn.lock | 14 ++-- 6 files changed, 191 insertions(+), 28 deletions(-) create mode 100644 patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch diff --git a/patches/squirrel.mac/.patches b/patches/squirrel.mac/.patches index 53f27f11a9e25..1a2a44222c4a8 100644 --- a/patches/squirrel.mac/.patches +++ b/patches/squirrel.mac/.patches @@ -3,3 +3,4 @@ fix_ensure_that_self_is_retained_until_the_racsignal_is_complete.patch fix_use_kseccschecknestedcode_kseccsstrictvalidate_in_the_sec.patch feat_add_new_squirrel_mac_bundle_installation_method_behind_flag.patch refactor_use_posix_spawn_instead_of_nstask_so_we_can_disclaim_the.patch +fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch diff --git a/patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch b/patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch new file mode 100644 index 0000000000000..febb0650dba1b --- /dev/null +++ b/patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch @@ -0,0 +1,75 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Samuel Attard +Date: Tue, 25 Oct 2022 13:09:55 -0700 +Subject: fix: abort installation attempt at the final mile if the app is + running + +There is a race condition between ShipIt launching and it performing an atomic rename to "install" the app bundle. This fixes the race by checking the list of running apps immediately before performing the rename. + +diff --git a/Squirrel/SQRLInstaller.h b/Squirrel/SQRLInstaller.h +index 2de1c384aae200f41de429cc35313e4c2ba9d0de..35a0c99129478f09526667d73c9544c3bfe14706 100644 +--- a/Squirrel/SQRLInstaller.h ++++ b/Squirrel/SQRLInstaller.h +@@ -37,6 +37,9 @@ extern const NSInteger SQRLInstallerErrorMovingAcrossVolumes; + // There was an error changing the file permissions of the update. + extern const NSInteger SQRLInstallerErrorChangingPermissions; + ++// There was a running instance of the app just prior to the update attempt ++extern const NSInteger SQRLInstallerErrorAppStillRunning; ++ + @class RACCommand; + + // Performs the installation of an update, saving its intermediate state to user +diff --git a/Squirrel/SQRLInstaller.m b/Squirrel/SQRLInstaller.m +index c1f328fa8c3689218ef260347cb8f9d30b789efe..f502df2f88424ea902a061adfeb30358daf212e4 100644 +--- a/Squirrel/SQRLInstaller.m ++++ b/Squirrel/SQRLInstaller.m +@@ -36,6 +36,7 @@ + const NSInteger SQRLInstallerErrorInvalidState = -6; + const NSInteger SQRLInstallerErrorMovingAcrossVolumes = -7; + const NSInteger SQRLInstallerErrorChangingPermissions = -8; ++const NSInteger SQRLInstallerErrorAppStillRunning = -9; + + NSString * const SQRLShipItInstallationAttemptsKey = @"SQRLShipItInstallationAttempts"; + NSString * const SQRLInstallerOwnedBundleKey = @"SQRLInstallerOwnedBundle"; +@@ -286,6 +287,19 @@ - (RACSignal *)installRequest:(SQRLShipItRequest *)request { + return [[[[self + renameIfNeeded:request updateBundleURL:updateBundleURL] + flattenMap:^(SQRLShipItRequest *request) { ++ // Final validation that the application is not running again; ++ NSArray *apps = [[NSRunningApplication runningApplicationsWithBundleIdentifier:request.bundleIdentifier] filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(NSRunningApplication *app, NSDictionary *bindings) { ++ return [[[app bundleURL] URLByStandardizingPath] isEqual:request.targetBundleURL]; ++ }]]; ++ if ([apps count] != 0) { ++ NSLog(@"Aborting update attempt because there are %lu running instances of the target app", [apps count]); ++ NSDictionary *errorInfo = @{ ++ NSLocalizedDescriptionKey: NSLocalizedString(@"App Still Running Error", nil), ++ NSLocalizedRecoverySuggestionErrorKey: NSLocalizedString(@"All instances of the target application should be quit during the update process", nil), ++ }; ++ return [RACSignal error:[NSError errorWithDomain:SQRLInstallerErrorDomain code:SQRLInstallerErrorAppStillRunning userInfo:errorInfo]]; ++ } ++ + return [[self acquireTargetBundleURLForRequest:request] concat:[RACSignal return:request]]; + }] + flattenMap:^(SQRLShipItRequest *request) { +diff --git a/Squirrel/ShipIt-main.m b/Squirrel/ShipIt-main.m +index 2c515ffdd67052a08ee8155c0e46b57e9721a0e5..5f3e29642012d04fc506b730a4e87fba861df250 100644 +--- a/Squirrel/ShipIt-main.m ++++ b/Squirrel/ShipIt-main.m +@@ -201,8 +201,14 @@ static void installRequest(RACSignal *readRequestSignal, NSString *applicationId + return action; + }] + subscribeError:^(NSError *error) { +- NSLog(@"Installation error: %@", error); +- exit(EXIT_FAILURE); ++ if ([[error domain] isEqual:SQRLInstallerErrorDomain] && [error code] == SQRLInstallerErrorAppStillRunning) { ++ NSLog(@"Installation cancelled: %@", error); ++ clearInstallationAttempts(applicationIdentifier); ++ exit(EXIT_SUCCESS); ++ } else { ++ NSLog(@"Installation error: %@", error); ++ exit(EXIT_FAILURE); ++ } + } completed:^{ + exit(EXIT_SUCCESS); + }]; diff --git a/spec/api-autoupdater-darwin-spec.ts b/spec/api-autoupdater-darwin-spec.ts index 18c075f0c7695..0ea8674f3dc48 100644 --- a/spec/api-autoupdater-darwin-spec.ts +++ b/spec/api-autoupdater-darwin-spec.ts @@ -5,6 +5,7 @@ import * as express from 'express'; import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; +import * as psList from 'ps-list'; import { AddressInfo } from 'net'; import { ifdescribe, ifit } from './spec-helpers'; import * as uuid from 'uuid'; @@ -95,6 +96,16 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch === return spawn(path.resolve(appPath, 'Contents/MacOS/Electron'), args); }; + const spawnAppWithHandle = (appPath: string, args: string[] = []) => { + return cp.spawn(path.resolve(appPath, 'Contents/MacOS/Electron'), args); + }; + + const getRunningShipIts = async (appPath: string) => { + const processes = await psList(); + const activeShipIts = processes.filter(p => p.cmd?.includes('Squirrel.framework/Resources/ShipIt com.github.Electron.ShipIt') && p.cmd!.startsWith(appPath)); + return activeShipIts; + }; + const withTempDirectory = async (fn: (dir: string) => Promise, autoCleanUp = true) => { const dir = await fs.mkdtemp(path.resolve(os.tmpdir(), 'electron-update-spec-')); try { @@ -323,6 +334,71 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch === }); }); + it('should abort the update if the application is still running when ShipIt kicks off', async () => { + await withUpdatableApp({ + nextVersion: '2.0.0', + startFixture: 'update', + endFixture: 'update' + }, async (appPath, updateZipPath) => { + server.get('/update-file', (req, res) => { + res.download(updateZipPath); + }); + server.get('/update-check', (req, res) => { + res.json({ + url: `http://localhost:${port}/update-file`, + name: 'My Release Name', + notes: 'Theses are some release notes innit', + pub_date: (new Date()).toString() + }); + }); + + enum FlipFlop { + INITIAL, + FLIPPED, + FLOPPED, + } + + const shipItFlipFlopPromise = new Promise((resolve) => { + let state = FlipFlop.INITIAL; + const checker = setInterval(async () => { + const running = await getRunningShipIts(appPath); + switch (state) { + case FlipFlop.INITIAL: { + if (running.length) state = FlipFlop.FLIPPED; + break; + } + case FlipFlop.FLIPPED: { + if (!running.length) state = FlipFlop.FLOPPED; + break; + } + } + if (state === FlipFlop.FLOPPED) { + clearInterval(checker); + resolve(); + } + }, 500); + }); + + const launchResult = await launchApp(appPath, [`http://localhost:${port}/update-check`]); + const retainerHandle = spawnAppWithHandle(appPath, ['remain-open']); + logOnError(launchResult, () => { + expect(launchResult).to.have.property('code', 0); + expect(launchResult.out).to.include('Update Downloaded'); + expect(requests).to.have.lengthOf(2); + expect(requests[0]).to.have.property('url', '/update-check'); + expect(requests[1]).to.have.property('url', '/update-file'); + expect(requests[0].header('user-agent')).to.include('Electron/'); + expect(requests[1].header('user-agent')).to.include('Electron/'); + }); + + await shipItFlipFlopPromise; + expect(requests).to.have.lengthOf(2, 'should not have relaunched the updated app'); + expect(JSON.parse(await fs.readFile(path.resolve(appPath, 'Contents/Resources/app/package.json'), 'utf8')).version).to.equal('1.0.0', 'should still be the old version on disk'); + + retainerHandle.kill('SIGINT'); + }); + }); + describe('with SquirrelMacEnableDirectContentsWrite enabled', () => { let previousValue: any; diff --git a/spec/fixtures/auto-update/update/index.js b/spec/fixtures/auto-update/update/index.js index e391362f12f04..b15dc7dc72e91 100644 --- a/spec/fixtures/auto-update/update/index.js +++ b/spec/fixtures/auto-update/update/index.js @@ -15,28 +15,34 @@ autoUpdater.on('error', (err) => { const urlPath = path.resolve(__dirname, '../../../../url.txt'); let feedUrl = process.argv[1]; -if (!feedUrl || !feedUrl.startsWith('http')) { - feedUrl = `${fs.readFileSync(urlPath, 'utf8')}/${app.getVersion()}`; + +if (feedUrl === 'remain-open') { + // Hold the event loop + setInterval(() => {}); } else { - fs.writeFileSync(urlPath, `${feedUrl}/updated`); + if (!feedUrl || !feedUrl.startsWith('http')) { + feedUrl = `${fs.readFileSync(urlPath, 'utf8')}/${app.getVersion()}`; + } else { + fs.writeFileSync(urlPath, `${feedUrl}/updated`); + } + + autoUpdater.setFeedURL({ + url: feedUrl + }); + + autoUpdater.checkForUpdates(); + + autoUpdater.on('update-available', () => { + console.log('Update Available'); + }); + + autoUpdater.on('update-downloaded', () => { + console.log('Update Downloaded'); + autoUpdater.quitAndInstall(); + }); + + autoUpdater.on('update-not-available', () => { + console.error('No update available'); + process.exit(1); + }); } - -autoUpdater.setFeedURL({ - url: feedUrl -}); - -autoUpdater.checkForUpdates(); - -autoUpdater.on('update-available', () => { - console.log('Update Available'); -}); - -autoUpdater.on('update-downloaded', () => { - console.log('Update Downloaded'); - autoUpdater.quitAndInstall(); -}); - -autoUpdater.on('update-not-available', () => { - console.error('No update available'); - process.exit(1); -}); diff --git a/spec/package.json b/spec/package.json index fd7ea280d5173..8884b2f591a7d 100644 --- a/spec/package.json +++ b/spec/package.json @@ -24,6 +24,7 @@ "mocha-junit-reporter": "^1.18.0", "mocha-multi-reporters": "^1.1.7", "pdfjs-dist": "^2.2.228", + "ps-list": "^7.0.0", "q": "^1.5.1", "send": "^0.16.2", "sinon": "^9.0.1", diff --git a/spec/yarn.lock b/spec/yarn.lock index ac0c5d34704c9..2f2caedda938e 100644 --- a/spec/yarn.lock +++ b/spec/yarn.lock @@ -73,10 +73,9 @@ dependencies: "@types/node" "*" -abstract-socket@^2.0.0: - version "2.1.1" - resolved "https://registry.yarnpkg.com/abstract-socket/-/abstract-socket-2.1.1.tgz#243a7e6e6ff65bb9eab16a22fa90699b91e528f7" - integrity sha512-YZJizsvS1aBua5Gd01woe4zuyYBGgSMeqDOB6/ChwdTI904KP6QGtJswXl4hcqWxbz86hQBe++HWV0hF1aGUtA== +"abstract-socket@github:saghul/node-abstractsocket#35b1b1491fabc04899bde5be3428abf5cf9cd528": + version "2.1.0" + resolved "https://codeload.github.com/saghul/node-abstractsocket/tar.gz/35b1b1491fabc04899bde5be3428abf5cf9cd528" dependencies: bindings "^1.2.1" nan "^2.12.1" @@ -385,7 +384,7 @@ data-uri-to-buffer@0.0.3: safe-buffer "^5.1.1" xml2js "^0.4.17" optionalDependencies: - abstract-socket "^2.0.0" + abstract-socket "github:saghul/node-abstractsocket#35b1b1491fabc04899bde5be3428abf5cf9cd528" debug@2.6.9, debug@^2.2.0: version "2.6.9" @@ -1283,6 +1282,11 @@ pngjs@^3.3.3: resolved "https://registry.yarnpkg.com/pngjs/-/pngjs-3.4.0.tgz#99ca7d725965fb655814eaf65f38f12bbdbf555f" integrity sha512-NCrCHhWmnQklfH4MtJMRjZ2a8c80qXeMlQMv2uVp9ISJMTt562SbGd6n2oq0PaPgKm7Z6pL9E2UlLIhC+SHL3w== +ps-list@^7.0.0: + version "7.2.0" + resolved "https://registry.yarnpkg.com/ps-list/-/ps-list-7.2.0.tgz#3d110e1de8249a4b178c9b1cf2a215d1e4e42fc0" + integrity sha512-v4Bl6I3f2kJfr5o80ShABNHAokIgY+wFDTQfE+X3zWYgSGQOCBeYptLZUpoOALBqO5EawmDN/tjTldJesd0ujQ== + psl@^1.1.24: version "1.9.0" resolved "https://registry.yarnpkg.com/psl/-/psl-1.9.0.tgz#d0df2a137f00794565fcaf3b2c00cd09f8d5a5a7"