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: crash reporting on Windows on Arm #19391

Merged
merged 8 commits into from Aug 27, 2019

Conversation

@richard-townsend-arm
Copy link
Contributor

commented Jul 23, 2019

Description of Change

This PR contains two back-ports from Chromium 77:

Together, these allow electron to generate crash reports in all circumstances. Previously, if native C++ code (in, say, atom or a node module) was called from Javascript and a fatal exception occurred (e.g. a null pointer dereference), Windows would not be able to unwind the stack correctly and find an exception handler. It would then kill the electron process without leaving behind any indication of what caused the crash. This V8 change, alongside the crashpad capture context changes, mean that field crash reports can be collected for Windows on Arm applications.

CC: @jkleinsc

Checklist

Release Notes

Notes: no-notes

@richard-townsend-arm richard-townsend-arm requested a review from electron/wg-upgrades as a code owner Jul 23, 2019

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

linux-arm64-testing seems to have failed due to a corrupt sscache stream (should be unrelated to the change).

@electron-cation electron-cation bot removed the new-pr 🌱 label Jul 24, 2019

@MarshallOfSound MarshallOfSound force-pushed the richard-townsend-arm:woa-crashreporting branch from d0a57a4 to 0b62702 Aug 1, 2019

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

I've reconfigured this PR to only apply these two patches when targetting arm64. @jkleinsc can you update our CI to set the apply_win_arm64_patches variable for arm builds?

@jkleinsc

This comment has been minimized.

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@richard-townsend-arm richard-townsend-arm changed the title fix: crash reporting on Windows on Arm [WIP] [DO NOT MERGE] fix: crash reporting on Windows on Arm Aug 8, 2019

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

CI flaked with this (unrelated) test

not ok 747 session module ses.protocol handles requests from partition
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-session-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24
ok 748 session module ses.setProxy(options) allows configuring proxy settings
ok 749 session module ses.setProxy(options) allows configuring proxy settings (callback)

@jkleinsc: could you kick off a build for Windows on Arm?

@jkleinsc

This comment has been minimized.

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

NB @jkleinsc: there should be two additional patches applied after the rest of them. Is --custom-var=apply_win_arm64_patches=True or similar being passed via %GCLIENT_EXTRA_ARGS%?

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

The TARGET_ARCH variable also seems to be unspecified on that previous build.

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

The electron/script/zip_manifests/dist_zip.win.arm64.manifest file should look quite similar to the x64 version, but a few extra DLLs are required (see this file generated from GN's runtime deps). I'll try to get another patch out tomorrow that adds those.

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

So, if everything's building right, together with #19366 and the optional #19383, we should get a test run like the following (i.e. about 2-4 unit test failures).

electron-6-0-x-integration-output.log

One other thing that occurred to me is that because dugite doesn't know how to install git prebuilts on Windows on Arm, you have to install the x86 version of Git onto the test machine and set LOCAL_GIT_DIRECTORY=C:\Program Files (x86)\Git\ in the environment. desktop/dugite#376 should fix this in the long run.

@jkleinsc jkleinsc added the 6-0-x label Aug 15, 2019

@jkleinsc jkleinsc referenced this pull request Aug 15, 2019
3 of 3 tasks complete
@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@richard-townsend-arm can you rebase this with the latest from 6-0-x? I just merged in the changes so that we can properly build WOA.

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Rebasing now...

richard-townsend-arm and others added 8 commits Jul 23, 2019
feat: cherry-pick V8 unwinding support
This commit backports [1] (originally written by @ThomsonTan) into V8
7.6. With this patch in place, calls made from JS into the atom core
which crash electron.exe will now generate crash dumps on Windows on
Arm rather than silently dying.

[1] https://chromium-review.googlesource.com/c/v8/v8/+/1701133/11
feat: backport crashpad cpu capture context
Backport of [1] (originally written by @kaadam) to Chromium 76's
crashpad. This lets you see the register values within the crash dump.

[1]
https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1632749

@richard-townsend-arm richard-townsend-arm force-pushed the richard-townsend-arm:woa-crashreporting branch from 3605ce3 to 3a1b8e7 Aug 26, 2019

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Cool, so I've confirmed that it builds for WoA, unsure whether it works yet because all my WoA devices are offline, I'm OoO and unable to go kick them.

@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

win-ia32-testing-pr has flaked due to an unrelated issue:

not ok 750 session module ses.protocol handles requests from partition
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-session-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24
ok 751 session module ses.setProxy(options) allows configuring proxy settings
ok 752 session module ses.setProxy(options) allows configuring proxy settings (callback)
@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@jkleinsc
Copy link
Contributor

left a comment

LGTM

@jkleinsc jkleinsc merged commit bb63189 into electron:6-0-x Aug 27, 2019

14 of 16 checks passed

Valid Backport Invalid Backport
Details
electron-woa-testing Build #20190826.1 failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190826.23 succeeded
Details
electron-arm64-testing Build #20190826.24 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Aug 27, 2019

Release Notes Persisted

no-notes

@richard-townsend-arm richard-townsend-arm deleted the richard-townsend-arm:woa-crashreporting branch Aug 27, 2019

@sofianguy sofianguy added this to Fixed in 6.0.5 in 6.0.x Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.