Skip to content

Comments

vesktop: revert to electron_33#380991

Closed
waltmck wants to merge 1 commit intoNixOS:masterfrom
waltmck:fix-vesktop
Closed

vesktop: revert to electron_33#380991
waltmck wants to merge 1 commit intoNixOS:masterfrom
waltmck:fix-vesktop

Conversation

@waltmck
Copy link

@waltmck waltmck commented Feb 10, 2025

There is a serious bug in Chromium 132 (and therefore Electron 34) that causes the renderer to crash on all systems using 16K pages---see this bug for Obsidian which was fixed by reverting Electron.

This affects Vesktop as well as any other app which is built using Electron 34. As I mentioned in the Obsidian issue, probably the principled thing to do here is to either backport the Chromium patch fixing this issue, or do a system-wide revert of the default Electron version to Electron 33. However, as a new contributor I am not sure the best way to handle that. In the meantime, I plan to submit PRs to at least revert Electron for apps I use daily.

Things done

This PR pins the Electron version for Vesktop to Electron 33 (as was done for Obsidian). I have tested it on my aarch64-linux machine (a Macbook Air M2 running Asahi).

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 10, 2025
Copy link
Member

@w-lfchen w-lfchen left a comment

Choose a reason for hiding this comment

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

i'm not sure whether this could lead to issues, but vesktop seems to use electron 34, judging by their package.json. just wanted to point that out

@emilylange
Copy link
Member

This affects Vesktop as well as any other app which is built using Electron 34.

Essentially all the packages listed in #376770 (comment) then.

As I mentioned in the Obsidian issue, probably the principled thing to do here is to either backport the Chromium patch fixing this issue, or do a system-wide revert of the default Electron version to Electron 33.

We have the infrastructure in nixpkgs to build electron from source. FWIW electron_32 and electron_33 are built from source.
For electron_34 we decided against source builds for now, given the current electron maintainer situation and electron being security critical and thus requiring very timely updates. See #376770 (review) for some more context on this.

In chromium, we already carry a patch for the page size issues on aarch64-linux for chromium M132+ since #365364.

++ lib.optionals (chromiumVersionAtLeast "131" && stdenv.hostPlatform.isAarch64) [
# Reverts decommit pooled pages which causes random crashes of tabs on systems
# with page sizes different than 4k. It 'supports' runtime page sizes, but has
# a hardcode for aarch64 systems.
# https://issues.chromium.org/issues/378017037
(fetchpatch {
name = "reverted-v8-decommit-pooled-paged-by-default.patch";
# https://chromium-review.googlesource.com/c/v8/v8/+/5864909
url = "https://chromium.googlesource.com/v8/v8/+/1ab1a14ad97394d384d8dc6de51bb229625e66d6^!?format=TEXT";
decode = "base64 -d";
stripLen = 1;
extraPrefix = "v8/";
revert = true;
hash = "sha256-PuinMLhJ2W4KPXI5K0ujw85ENTB1wG7Hv785SZ55xnY=";
})
];

A source build of electron_34 would inherit that automatically.

So this is all a bit sad.

@waltmck can you check whether passing --js-flags=--no-decommit-pooled-pages12 to vesktop or any other affected package fixes this for you? In which case we could add that to our electron binary wrapper and unblock all of the packages instead.

Thank you for your time :)

Footnotes

  1. https://github.com/NixOS/nixpkgs/pull/365364#issuecomment-2564288160

  2. https://github.com/tpwrules/nixos-apple-silicon/pull/259

@waltmck
Copy link
Author

waltmck commented Feb 11, 2025

@emilylange Passing --js-flags=--no-decommit-pooled-pages to Vesktop does fix the issue for me. This issue will also be fixed by a pending Electron PR using the same workaround, so the wrapper would only need to be a stopgap until that propagates to the next Electron release.

@ImUrX
Copy link
Contributor

ImUrX commented Feb 16, 2025

This still should be merged for now as #380429 is still an issue with binary builds for electron, and probably should be changed to specifically use -source version with a comment on it explaining why

@ImUrX
Copy link
Contributor

ImUrX commented Feb 16, 2025

Oh I don't know tho if its preferred to change the used package inside the package.nix instead of just doing it via all-packages.nix

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

I'm fine with reverting to 33 for the reasons mentioned above. I don't believe upstream really has a hard requirement on any given Electron version (they usually just stay at the latest), and we haven't tried to match it with the default in Nixpkgs before either

I would prefer this in the package.nix file itself though so that this workaround is more apparent and we can be reminded to remove it -- preferably with a comment as to why, or a simple link to this PR ;)

@ImUrX
Copy link
Contributor

ImUrX commented Feb 16, 2025

They do in this PR Vencord/Vesktop#1058, there might be some other case, but personally electron hardware accel never worked for me in Vesktop, so might not matter.

@emilylange
Copy link
Member

Opened #382497 as a proper global workaround in order to get it fixed for all consumers of electron_34.

@ImUrX
Copy link
Contributor

ImUrX commented Feb 16, 2025

#382497 wouldnt fix #380429 tho

@ImUrX
Copy link
Contributor

ImUrX commented Feb 16, 2025

apparently there is a degradation on electron-bin (all versions) that makes screenshare unusable (not arm64 only), at least with Vesktop, using a -source build of any version fixes the issue

@emilylange
Copy link
Member

@ImUrX can you please test the following diff. It fixes screenshare on Wayland/Sway in element-desktop (when forced to use -bin) for me.

diff --git a/pkgs/development/tools/electron/binary/generic.nix b/pkgs/development/tools/electron/binary/generic.nix
index 6864efed6579..234fa7971fc5 100644
--- a/pkgs/development/tools/electron/binary/generic.nix
+++ b/pkgs/development/tools/electron/binary/generic.nix
@@ -27,6 +27,11 @@
   pango,
   systemd,
   pciutils,
+  libnotify,
+  pipewire,
+  libsecret,
+  libpulseaudio,
+  speechd-minimal,
 }:
 
 version: hashes:
@@ -117,6 +122,11 @@ let
       pciutils
       stdenv.cc.cc
       systemd
+      libnotify
+      pipewire
+      libsecret
+      libpulseaudio
+      speechd-minimal
     ]
     ++ lib.optionals (lib.versionAtLeast version "9.0.0") [
       libdrm

@ImUrX
Copy link
Contributor

ImUrX commented Feb 16, 2025

@emilylange yeah that seems to fix it, sadly hardware acceleration still does not work. If i use chromium directly with the parameters for enabling it, it does work.

@ImUrX
Copy link
Contributor

ImUrX commented Feb 17, 2025

I see some weird behavior with notifications when clicking on them, the app kinda breaks and requires you to press escape while using the app to fix it with electron_34-bin. Do you have any idea what could it be?

@emilylange
Copy link
Member

Closing, as the underlying reason(s) for this revert where fixed by #382497 and #383706.

I see some weird behavior with notifications when clicking on them, the app kinda breaks and requires you to press escape while using the app to fix it with electron_34-bin. Do you have any idea what could it be?

No idea, no, sorry. Feel free to open a new issue, but don't put your hopes up too high. Electron is an inherently fragile ecosystem, and it's often extremely difficult to reproduce issues.

In any case, thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants