Desktop App: fix crash on Apple Silicon caused by 8.0.0. #78969

worldomonation · 2023-07-05T02:18:22Z

Proposed Changes

This PR adds fixes to the Entitlements list that caused the Apple Silicon build of the app to either crash or display a white screen on startup.

This PR also adds a fix for the failing Window build.

What went wrong?

The previous release of WordPress.com Desktop App 8.0.0 caused crashes on users on the Apple Silicon architecture.

After almost a week of investigations, debugging and trial I found the culprit is because we jumped from Electron 12.x.x to Electron 24.x.x.

More specifically, Electron has required (since 20.x.x) that additional entitlement be added in the form of com.apple.security.cs.allow-jit.

This issue was not caught in #78098, because:

CircleCI builds were all successful;
locally built Apple Silicon builds launched as expected, and it was assumed that CI-produced builds will be the same;

As it turns out, the CI-produced builds involve the Apple signing process, which is where the Entitlements come into play. Because the Entitlements didn't change from Electron 12.x.x, when the binary was signed additional privileges required to run Electron >= 20.x.x was not granted, hence the app crashed on all Apple Silicon users.

Testing Instructions

Ensure the following build configurations are passing:

CircleCI > wp-desktop-mac
CircleCI > wp-desktop-linux
CircleCI > wp-desktop-windows

Additionally, download the x64 and arm64 produced binaries from CircleCI and ensure it functions as expected.

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

github-actions · 2023-07-05T02:18:38Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-73590

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-73590&env=jetpack

matticbot · 2023-07-05T02:32:25Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@worldomonation please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

nsakaimbo · 2023-07-05T16:47:23Z

desktop/package.json

@@ -51,6 +50,7 @@
 		"webpack-cli": "^4.9.2"
 	},
 	"dependencies": {
+		"@electron/rebuild": "3.2.13",


electron-rebuild should be a dev dependency, and not a direct dependency of the app.

worldomonation · 2023-07-06T04:52:37Z

Status update, as of July 6, 2023:

Build

resolved the issue with entitlement file structure, the build can now complete.
E2E tests still can't run, likely related to the issue below.

Configuration:

electron 25.2.0
electron/rebuild 3.2.13
electron-builder 23.0.3

Issue
The app loads a white screen of death.

wp-desktop ci passing, closing review

.circleci/config.yml

worldomonation · 2023-07-06T05:54:19Z

desktop/bin/build-mac-ci.js

@@ -13,7 +13,8 @@ const circleTag = process.env.CIRCLE_TAG;
 const isReleaseBuild =
 	process.platform === 'darwin' && !! circleTag && circleTag.startsWith( 'desktop-v' );

-const arches = isReleaseBuild ? [ 'x64', 'arm64' ] : [ 'x64' ];
+// Always build both Intel and Apple Silicon versions.
+const arches = [ 'x64', 'arm64' ];


I've elected to remove the conditional and thus always build both the Intel and Apple Silicon binaries.

I am thinking that as more of the world moves to Apple Silicon, making sure the arm64 build doesn't break is going to be important.

I've elected to remove the conditional and thus always build both the Intel and Apple Silicon binaries.

I would suggest not doing this, and limiting the build to only one or the other (preferably the arm64 build, as we should be deprecating the Intel build soon anyway).

From the CI run (link), it's unclear which architecture the end-to-end tests were ran against (as far as I can tell it wasn't both). It's nice that the test script can find either the Apple Silicon or Intel binaries, but you still have to specify which of the two builds to use during a test run (or if you really want to build both, then amend the CI script to execute against both):

jest-haste-map: Haste module naming collision: WordPressDesktop The following files share their name; please adjust your hasteImpl: * <rootDir>/release/mac/WordPress.com.app/Contents/Resources/app/package.json * <rootDir>/release/mac-arm64/WordPress.com.app/Contents/Resources/app/package.json PASS test/e2e/specs/login.js User Can log in ✓ Log in (950 ms) Test Suites: 1 passed, 1 total Tests: 1 passed, 1 total Snapshots: 0 total Time: 3.645 s Ran all test suites.

if you really want to build both, then amend the CI script to execute against both

To clarify, the proposed choices are either trim it back to building and testing a single architecture (like the status quo), or build both and run tests for both binaries.

Edit: I saw this comment and it sounds like you're in favor of cutting it back to a single arm64 build for non-releases.

If so, I can look at getting this out, but with a high-visibility project today and tomorrow I am not sure how much time I have to work on it.

it sounds like you're in favor of cutting it back to a single arm64 build for non-releases.

Yes - I would lean towards a single architecture for non-release builds:

const arches = isReleaseBuild ? [ 'x64', 'arm64' ] : [ 'arm64' ];

I would consider the Intel build less important going forward (perhaps it's something we can eventually drop completely).

Sounds good, will make the change.

nsakaimbo · 2023-07-10T22:58:09Z

desktop/package.json

 		"copy-webpack-plugin": "^10.1.0",
-		"electron": "24.0.0",
+		"electron": "25.2.0",


Quick questions: Why is the bump to electron version necessary? If we maintain the same dependencies, the changes to the entitlement files should be sufficient for this fix, no?

In general, we should be a little more cautious when bumping major electron versions, as the process needs to be a little more comprehensive. For starters, you would need to evaluate any breaking API changes as described in Electron's breaking changes docs, and smoke-test the app on all platforms as there are things that definitely won't be caught by CI (for example menu items, keyboard shortcuts, the auto-updater). With the exception of the Apple Silicon build, I did verify these things when updating to version 8.0.0. Unless you're willing and able to verify all these items again (on all platforms), I would suggest maintaining the electron version and other dependencies. The less frequently we have to perform this sort of manual verification/smoke-testing, the better IMO.

I updated the packages initially as part of our private conversation about using the up-to-date version of electron-related packages.

Though I didn't check all of the breaking changes doc, I did get my teammates to check macOS builds and I checked Windows myself. Is there more in-depth checks required if bumping electron versions?

Reverted in 3e0a8ece862d8eb2f51604ff715eea2a7515c195.

Is there more in-depth checks required if bumping electron versions?

I tried to enumerate them in this preliminary PR from a while ago. Aside from smoke-testing, the auto-updater is a critical component. Included in manual smoke-testing is ensuring keyboard shortcuts and menu items continue to work as expected.

desktop/package.json

desktop/test/e2e/specs/login.js

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@worldomonation please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

worldomonation · 2023-07-11T15:05:17Z

@nsakaimbo One of the reverted changes ended up causing the build to break. I'll have to investigate, but can we settle on an acceptable solution to get the fix out for users complaining about the crashes?

Edit: investigated arm64 build on Apple Silicon, we're back to a white screen of death.

Edit Edit: fixed the issue, the core fix for the crash/white screen got dropped when I rebased.

wp-desktop ci passing, closing review

nsakaimbo

Nice work, and way to go in identifying that entitlements fix. 👍🏽 For a quick sanity check I tested the x64 artifact as generated by the M1 image and confirmed it worked correctly. 🎉

I would suggest limiting builds to a single architecture for non-release builds, but ultimately that's up to you and LGTM otherwise.

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@worldomonation please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

wp-desktop ci passing, closing review

worldomonation · 2023-07-12T08:55:20Z

@nsakaimbo Thanks for the review! This PR is ready to go, with all builds building correctly and passing.

One other thing I had to fix was the artifact path for yet another step: Persist Mac Executable.

After fixing the path, I noticed the artifact list isn't as descriptive as it used to be when building two arches:

When building two arches:

From comparing the artifact list to other trunk builds, it seems like normally the "less descriptive" artifacts are captured, but I want to make sure this looks alright before merging the fix.

Links
Two arches: https://app.circleci.com/pipelines/github/Automattic/wp-calypso/184425/workflows/b35a8043-c225-4df8-963a-0cc8eb09d75e/jobs/1123016/artifacts
One arch: https://app.circleci.com/pipelines/github/Automattic/wp-calypso/184577/workflows/420582f2-7535-4b74-85a8-b8d8c20edb19/jobs/1123102/artifacts

nsakaimbo · 2023-07-12T16:41:24Z

I noticed the artifact list isn't as descriptive as it used to be when building two arches:

Good catch, but yes this is by design. If you look at the existing artifacts being persisted for business-as-usual jobs in trunk right now, they would match the smaller artifact set (i.e. the app zip and contents of the results/ folder). The full artifact set should be persisted only at release time. My guess is there are probably a few more places in the build scripts that would need to accommodate two architectures being more cleanly persisted for non-release jobs. In addition, it seems the SHA of the "two arches" CI job you shared doesn't seem to be available any longer - it's possible that version of the branch had some changes in desktop/package.json or yarn.lock, either of which would trigger a full artifact build. However the important thing is the artifact set looks correct when isolated to a single architecture. Hope that makes sense.

As a sanity check, if you like you can temporarily set ELECTRON_BUILDER_ARGS to an empty string and isReleaseBuild to true in the mac release script to compare what artifacts will be persisted at release time (in this case, the artifact set should look more like the "full" set you noted). The electron builder args are set here, and a full build is invoked when dependencies change or the version number is bumped. (Therefore most jobs in trunk will have the smaller artifact set, except when dependencies change, which should be much less frequent.)

Thanks for being so thorough!

worldomonation · 2023-07-13T09:29:36Z

Confirmed that if I set ELECTRON_BUILDER_ARGS = '' and isReleaseBuild = true in the script, the longer artifact list is produced:

With that, I think it's good to go. I will merge this after dropping the experimental changes.

- remove electron-rebuild

- fix test path to handle both Apple Silicon and Intel by checking the arch. bin/build-mac-ci.js - build both x64 and arm64.

- add new key to permit jit

- remove commented out code. - add comment. test/e2e/specs/login.js - remove debugging code. - add comments.

- fix command for windows (unrelated to crash fix for Apple Silicon)

- only build arm64 by default

- add wildcard for the Persist Mac Excutable step to be able to locate the default arm64 build.

)" This reverts commit a38cdf5.

worldomonation self-assigned this Jul 5, 2023

wp-desktop previously requested changes Jul 5, 2023

View reviewed changes

worldomonation force-pushed the desktop/fix-electron-crash branch from b36f665 to a4ce80c Compare July 5, 2023 06:50

nsakaimbo reviewed Jul 5, 2023

View reviewed changes

worldomonation force-pushed the desktop/fix-electron-crash branch from 88c61cf to 3ca024d Compare July 6, 2023 04:51

worldomonation force-pushed the desktop/fix-electron-crash branch from 014b364 to 5abe782 Compare July 6, 2023 05:39

worldomonation commented Jul 6, 2023

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

worldomonation commented Jul 6, 2023

View reviewed changes

worldomonation marked this pull request as ready for review July 6, 2023 05:54

worldomonation changed the title ~~Desktop/fix electron crash~~ Desktop App: fix crash on Apple Silicon caused by 8.0.0. Jul 6, 2023

worldomonation added the [Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. label Jul 6, 2023

nsakaimbo reviewed Jul 10, 2023

View reviewed changes

desktop/package.json Outdated Show resolved Hide resolved

nsakaimbo reviewed Jul 10, 2023

View reviewed changes

desktop/test/e2e/specs/login.js Show resolved Hide resolved

worldomonation force-pushed the desktop/fix-electron-crash branch from 3e0a8ec to 98cd59f Compare July 11, 2023 09:33

wp-desktop previously requested changes Jul 11, 2023

View reviewed changes

worldomonation force-pushed the desktop/fix-electron-crash branch from 98cd59f to 9a1226b Compare July 11, 2023 09:46

nsakaimbo self-requested a review July 11, 2023 18:53

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 11, 2023

nsakaimbo approved these changes Jul 11, 2023

View reviewed changes

worldomonation force-pushed the desktop/fix-electron-crash branch from d1cfe7a to 28581c2 Compare July 12, 2023 08:06

wp-desktop previously requested changes Jul 12, 2023

View reviewed changes

worldomonation added 8 commits July 13, 2023 18:29

desktop/package.json

0383e77

- remove electron-rebuild

test/e2e/specs/login.js

e6bb2ad

- fix test path to handle both Apple Silicon and Intel by checking the arch. bin/build-mac-ci.js - build both x64 and arm64.

resource/macos/entitlements.mac.plist

a295006

- add new key to permit jit

bin/build-mac-ci.js

b9cb22f

- remove commented out code. - add comment. test/e2e/specs/login.js - remove debugging code. - add comments.

.circleci/config.yml

1380c32

- fix command for windows (unrelated to crash fix for Apple Silicon)

desktop/bin/build-mac-ci.js

73824d4

- only build arm64 by default

remove changes of yarn.lock

861e0ed

.circleci/config.yml

95ea157

- add wildcard for the Persist Mac Excutable step to be able to locate the default arm64 build.

worldomonation force-pushed the desktop/fix-electron-crash branch from a23a2bc to 95ea157 Compare July 13, 2023 09:30

worldomonation merged commit a38cdf5 into trunk Jul 14, 2023
22 checks passed

worldomonation deleted the desktop/fix-electron-crash branch July 14, 2023 02:24

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 14, 2023

worldomonation added a commit that referenced this pull request Jul 14, 2023

Revert "Desktop App: fix crash on Apple Silicon caused by 8.0.0. (#78969

ed40953

)" This reverts commit a38cdf5.

This was referenced Jul 14, 2023

Revert "Desktop App: fix crash on Apple Silicon caused by 8.0.0." #79397

Closed

Desktop App: bump version to 8.0.1 #79409

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop App: fix crash on Apple Silicon caused by 8.0.0. #78969

Desktop App: fix crash on Apple Silicon caused by 8.0.0. #78969

worldomonation commented Jul 5, 2023 •

edited

github-actions bot commented Jul 5, 2023 •

edited by matticbot

matticbot commented Jul 5, 2023

wp-desktop left a comment

nsakaimbo Jul 5, 2023 •

edited

worldomonation commented Jul 6, 2023

worldomonation Jul 6, 2023

nsakaimbo Jul 10, 2023

worldomonation Jul 11, 2023 •

edited

nsakaimbo Jul 11, 2023

worldomonation Jul 12, 2023

nsakaimbo Jul 10, 2023

worldomonation Jul 11, 2023 •

edited

worldomonation Jul 11, 2023

nsakaimbo Jul 11, 2023

wp-desktop left a comment

worldomonation commented Jul 11, 2023 •

edited

nsakaimbo left a comment

wp-desktop left a comment

worldomonation commented Jul 12, 2023 •

edited

nsakaimbo commented Jul 12, 2023 •

edited

worldomonation commented Jul 13, 2023

Desktop App: fix crash on Apple Silicon caused by 8.0.0. #78969

Desktop App: fix crash on Apple Silicon caused by 8.0.0. #78969

Conversation

worldomonation commented Jul 5, 2023 • edited

Proposed Changes

What went wrong?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Jul 5, 2023 • edited by matticbot

matticbot commented Jul 5, 2023

wp-desktop left a comment

Choose a reason for hiding this comment

nsakaimbo Jul 5, 2023 • edited

Choose a reason for hiding this comment

worldomonation commented Jul 6, 2023

worldomonation Jul 6, 2023

Choose a reason for hiding this comment

nsakaimbo Jul 10, 2023

Choose a reason for hiding this comment

worldomonation Jul 11, 2023 • edited

Choose a reason for hiding this comment

nsakaimbo Jul 11, 2023

Choose a reason for hiding this comment

worldomonation Jul 12, 2023

Choose a reason for hiding this comment

nsakaimbo Jul 10, 2023

Choose a reason for hiding this comment

worldomonation Jul 11, 2023 • edited

Choose a reason for hiding this comment

worldomonation Jul 11, 2023

Choose a reason for hiding this comment

nsakaimbo Jul 11, 2023

Choose a reason for hiding this comment

wp-desktop left a comment

Choose a reason for hiding this comment

worldomonation commented Jul 11, 2023 • edited

nsakaimbo left a comment

Choose a reason for hiding this comment

wp-desktop left a comment

Choose a reason for hiding this comment

worldomonation commented Jul 12, 2023 • edited

nsakaimbo commented Jul 12, 2023 • edited

worldomonation commented Jul 13, 2023

worldomonation commented Jul 5, 2023 •

edited

github-actions bot commented Jul 5, 2023 •

edited by matticbot

nsakaimbo Jul 5, 2023 •

edited

worldomonation Jul 11, 2023 •

edited

worldomonation Jul 11, 2023 •

edited

worldomonation commented Jul 11, 2023 •

edited

worldomonation commented Jul 12, 2023 •

edited

nsakaimbo commented Jul 12, 2023 •

edited