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.

Sign up for GitHub

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

Jump to bottom

fix: abort ShipIt installation attempt at the final mile if the app is running #36130

Merged
merged 5 commits into from Nov 14, 2022
Merged

fix: abort ShipIt installation attempt at the final mile if the app is running #36130

merged 5 commits into from Nov 14, 2022

Conversation

MarshallOfSound
Copy link
Member

As in title, basically there is a race condition in Squirrel.Mac. The current flow looks like this

1. Download update
2. Quit app and launch ShipIt
3. ShipIt waits for app to terminate
4. ShipIt prepares update, unzips, validates, etc.
5. ShipIt installs update
6. ShipIt launches new app

If somehow the app manages to launch (the user clicks it, programatic relaunch, etc.) during step 4 then step 5 succeeds, step 6 bails as the app is running and everything appears to be fine. But it is not.... The running app no longer exists on disk and will therefore fail all uncached TCC / Security.h checks including microphone/camera permission checks and keychain access checks. (Amongst other awful things).

This fixes this race condition by adding a step 4.5 that checks if any app is running from the bundle path we're about to install an update to. We don't do step (3) again because waiting for the app to quit is unrealistic if the user manually launched it. The best thing to do is to get back into a known good state and bail the update flow with an error but without triggering an automatic launchd retry (hence the "Happy error" exit logic).

Notes: Fixed race condition during update on macOS that could result in TCC and Keychain errors

@MarshallOfSound MarshallOfSound requested a review from a team as a code owner October 25, 2022 20:17
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 25, 2022
@MarshallOfSound MarshallOfSound added semver/patch backwards-compatible bug fixes target/20-x-y target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. labels Oct 25, 2022
nornagon
nornagon approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

:shipit:

spec/yarn.lock Show resolved Hide resolved
patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch Show resolved Hide resolved
patches/squirrel.mac/fix_abort_installation_attempt_at_the_final_mile_if_the_app_is.patch Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 26, 2022
jkleinsc
jkleinsc previously requested changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Newly added test fails

@VerteDinde VerteDinde mentioned this pull request Oct 31, 2022
Closed
3 tasks
@MarshallOfSound MarshallOfSound added semver/patch backwards-compatible bug fixes and removed semver/patch backwards-compatible bug fixes labels Nov 14, 2022
@MarshallOfSound MarshallOfSound merged commit d8bb172 into main Nov 14, 2022
@MarshallOfSound MarshallOfSound deleted the fix-squirrel-mac-race branch November 14, 2022 18:12
@release-clerk
Copy link

release-clerk bot commented Nov 14, 2022

Release Notes Persisted

Fixed race condition during update on macOS that could result in TCC and Keychain errors

@trop
Copy link
Contributor

trop bot commented Nov 14, 2022

I was unable to backport this PR to "20-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/20-x-y label Nov 14, 2022
@trop
Copy link
Contributor

trop bot commented Nov 14, 2022

I was unable to backport this PR to "22-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Nov 14, 2022

I was unable to backport this PR to "21-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/22-x-y PR should also be added to the "22-x-y" branch. label Nov 14, 2022
@trop trop bot added needs-manual-bp/22-x-y needs-manual-bp/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. labels Nov 14, 2022
MarshallOfSound added a commit that referenced this pull request Nov 15, 2022
@MarshallOfSound @nornagon @patchup
fix: abort ShipIt installation attempt at the final mile if the app i…
176a116 
…s 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 <jeremya@chromium.org>

* chore: update patches

* spec: make the ShipIt process lister helper more resilient

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
@MarshallOfSound MarshallOfSound mentioned this pull request Nov 15, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Nov 15, 2022

@MarshallOfSound has manually backported this PR to "22-x-y", please check out #36362

@trop trop bot added in-flight/22-x-y merged/22-x-y PR was merged to the "22-x-y" branch. and removed needs-manual-bp/22-x-y in-flight/22-x-y labels Nov 15, 2022
codebytere pushed a commit that referenced this pull request Nov 16, 2022
@MarshallOfSound @nornagon @patchup
fix: abort ShipIt installation attempt at the final mile if the app i…
5d418c5 
…s 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 <jeremya@chromium.org>

* chore: update patches

* spec: make the ShipIt process lister helper more resilient

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@MarshallOfSound @nornagon
@patchup @khalwa
fix: abort ShipIt installation attempt at the final mile if the app i…
31ad244 
…s running (electron#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 <jeremya@chromium.org>

* chore: update patches

* spec: make the ShipIt process lister helper more resilient

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
@commit-lint commit-lint bot mentioned this pull request Feb 24, 2023
Open
VerteDinde added a commit that referenced this pull request Apr 2, 2023
@VerteDinde @nornagon @patchup
fix: abort ShipIt installation attempt at the final mile if the app i…
893db5b 
…s 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 <jeremya@chromium.org>

* chore: update patches

* spec: make the ShipIt process lister helper more resilient

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
@VerteDinde VerteDinde mentioned this pull request Apr 2, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Apr 2, 2023

@VerteDinde has manually backported this PR to "21-x-y", please check out #37794

codebytere pushed a commit that referenced this pull request Apr 3, 2023
@VerteDinde @nornagon @patchup
fix: abort ShipIt installation attempt at the final mile if the app i…
105e58f 
…s running (#37794)

* 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 <jeremya@chromium.org>

* chore: update patches

* spec: make the ShipIt process lister helper more resilient

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

* test: update spec & spec-main/yarn.lock

---------

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
@trop trop bot added the merged/21-x-y PR was merged to the "21-x-y" branch. label Apr 3, 2023
@beyondkmp beyondkmp mentioned this pull request Apr 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

@VerteDinde VerteDinde VerteDinde approved these changes

@jkleinsc jkleinsc jkleinsc left review comments

Assignees
No one assigned
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MarshallOfSound @nornagon @jkleinsc @zcbenz @VerteDinde @miniak