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

chore: cherry-pick dd4c3ddadbb9 from chromium #32603

Closed

Conversation

primeos
Copy link

@primeos primeos commented Jan 24, 2022

Description of Change

Those patches are required to fix Ozone/Wayland on newer Wayland
compositors. The previous behaviour is incorrect but worked as long as
Wayland compositors didn't implement newer protocol versions that aren't
supported yet by Ozone/Wayland (e.g. wl_output version 4).

Fix #32487.
Upstream Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1279574

Co-authored-by: Jeremy Rose nornagon@nornagon.net

Merge to M98: [linux/wayland] Fixed terminate caused by binding to...

...wrong version.

The Ozone/Wayland implementation had a few places where the Wayland
objects were bound without proper checking for their versions. That was
part of the technical debt not addressed before, and ended up causing
the issue explained in the linked crbug: the compositor terminates the
client that binds to the protocol that it does not actually support.

This patch fixes the issue by adding the necessary checks in all places
where they were missing. Also a convenience macro for validating the
version is proposed.

(cherry picked from commit dd4c3ddadbb9869f59cee201a38e9ca3b9154f4d)

Bug: 1279574
Change-Id: I74efa97f64b480bed47372d8d559593ae84eeb18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3337037
Reviewed-by: Maksim Sisov msisov@igalia.com
Commit-Queue: Alexander Dunaev adunaev@igalia.com
Cr-Original-Commit-Position: refs/heads/main@{#951428}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3402123
Cr-Commit-Position: refs/branch-heads/4758@{#772}
Cr-Branched-From: 4a2cf4baf90326df19c3ee70ff987960d59a386e-refs/heads/main@{#950365}

Notes: Backported fix for 1279574.

@primeos primeos requested a review from a team as a code owner January 24, 2022 18:34
@welcome
Copy link

welcome bot commented Jan 24, 2022

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 24, 2022
Those patches are required to fix Ozone/Wayland on newer Wayland
compositors. The previous behaviour is incorrect but worked as long as
Wayland compositors didn't implement newer protocol versions that aren't
supported yet by Ozone/Wayland (e.g. wl_output version 4).

Fix electron#32487.
Upstream Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1279574

Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
@primeos primeos force-pushed the herry-pick/16-x-y/chromium/31376b855469 branch from cbbcf18 to c3d730f Compare January 24, 2022 19:23
@primeos
Copy link
Author

primeos commented Jan 24, 2022

Ok, so at least the Linux builds are passing now (the rest of the builds is still running).

As for testing: With Electron version 16.0.7 (https://github.com/electron/electron/releases/download/v16.0.7/electron-v16.0.7-linux-x64.zip) it crashes as expected:

$ electron --enable-features=UseOzonePlatform --ozone-platform=wayland https://github.com/electron/electron/releases/tag/v16.0.7
interface 'wl_output' has no event 4
^C

$ echo $?
130

And with the build artifact for this PR (https://app.circleci.com/pipelines/github/electron/electron/48705/workflows/e2e1e0d2-d04a-49df-9a44-173baef7f4ad/jobs/1107659/artifacts -> https://1107659-9384267-gh.circle-artifacts.com/0/dist.zip) I get past that but it still fails:

$ result/bin/electron --enable-features=UseOzonePlatform --ozone-platform=wayland https://github.com/electron/electron/releases/tag/v16.0.7
[6798:0124/214016.725787:ERROR:power_monitor_device_source_stub.cc(11)] Not implemented reached in virtual bool base::PowerMonitorDeviceSource::IsOnBatteryPower()
[6798:0124/214016.966095:ERROR:wayland_connection.cc(611)] Not implemented reached in static void ui::WaylandConnection::Global(void *, wl_registry *, uint32_t, const char *, uint32_t)
[6798:0124/214017.359549:ERROR:cursor_loader.cc(114)] Failed to load a platform cursor of type kNull
[6798:0124/214017.360234:ERROR:wayland_window.cc(394)] Not implemented reached in virtual void ui::WaylandWindow::SetWindowIcons(const gfx::ImageSkia &, const gfx::ImageSkia &)
[6832:0124/214017.441688:ERROR:egl_util.cc(74)] Failed to load GLES library: libGLESv2.so.2: libGLESv2.so.2: cannot open shared object file: No such file or directory
[6832:0124/214017.449296:ERROR:viz_main_impl.cc(161)] Exiting GPU process due to errors during initialization
[6863:0124/214017.578882:ERROR:egl_util.cc(74)] Failed to load GLES library: libGLESv2.so.2: libGLESv2.so.2: cannot open shared object file: No such file or directory
[6863:0124/214017.590378:ERROR:viz_main_impl.cc(161)] Exiting GPU process due to errors during initialization
[6893:0124/214017.829907:ERROR:sandbox_linux.cc(376)] InitializeSandbox() called with multiple threads in process gpu-process.
[6893:0124/214017.843207:ERROR:wayland_canvas_surface.cc(244)] Not implemented reached in virtual std::unique_ptr<gfx::VSyncProvider> ui::WaylandCanvasSurface::CreateVSyncProvider()
[6798:0124/214042.334603:ERROR:object_proxy.cc(642)] Failed to call method: org.freedesktop.DBus.Properties.Get: object_path= /org/freedesktop/portal/desktop: org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.
[6798:0124/214042.334646:ERROR:select_file_dialog_impl_portal.cc(243)] Failed to read portal version property
^C[6798:0124/214116.236338:ERROR:wayland_surface.cc(257)] Not implemented reached in zwp_linux_surface_synchronization_v1 *ui::WaylandSurface::GetSurfaceSync()
[6798:0124/214116.303097:ERROR:process_posix.cc(330)] Unable to terminate process 6839: No such process (3)

The Failed to load GLES library: libGLESv2.so.2 should be due to my setup and I expected that but I haven't seen those Not implemented reached errors before (I assume this depends on the build configuration and they might normally be hidden? - Ozone/Wayland is experimental so some things might simply not be implemented yet). Some but not all of those errors happen in Wayland codepaths so that's a bit concerning (could be regressions). Can someone else with a Wayland setup help with testing this (i.e. if this PR fixes the issues with affected Wayland compositors and if there are no new errors compared to 16.0.7)?

@smasher164
Copy link

Can someone else with a Wayland setup help with testing this (i.e. if this PR fixes the issues with affected Wayland compositors and if there are no new errors compared to 16.0.7)?

@primeos I applied your patch into my environment to test, and I also see the Not implemented reached in virtual std::unique_ptr<gfx::VSyncProvider> error. However, I also see a segfault, which might be unrelated since it mentions egl vs ANGLE. I'm running Sway on NixOS, btw.

I then addressed the GLES error by adding libglvnd into my environment, but it still segfaults. This is sitting on my fork of nixpkgs here: smasher164/nixpkgs@ac7d1eb. Here is the output after applying this patch and adding libglvnd:

$ ./result/bin/electron --enable-features=UseOzonePlatform --ozone-platform=wayland https://github.com/electron/electron/releases/tag/v16.0.7
[41005:0127/015450.626382:ERROR:power_monitor_device_source_stub.cc(11)] Not implemented reached in virtual bool base::PowerMonitorDeviceSource::IsOnBatteryPower()
[41005:0127/015450.751684:ERROR:wayland_connection.cc(611)] Not implemented reached in static void ui::WaylandConnection::Global(void *, wl_registry *, uint32_t, const char *, uint32_t)

(electron:41005): Gdk-WARNING **: 01:54:50.754: Failed to read portal settings: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.portal.Settings” on object at path /org/freedesktop/portal/desktop
[41005:0127/015450.800697:ERROR:object_proxy.cc(642)] Failed to call method: org.freedesktop.DBus.Properties.Get: object_path= /org/freedesktop/portal/desktop: org.freedesktop.DBus.Error.InvalidArgs: No such interface “org.freedesktop.portal.FileChooser”
[41005:0127/015450.800711:ERROR:select_file_dialog_impl_portal.cc(243)] Failed to read portal version property
[41005:0127/015450.811864:ERROR:cursor_loader.cc(114)] Failed to load a platform cursor of type kNull
[41005:0127/015450.812327:ERROR:wayland_window.cc(394)] Not implemented reached in virtual void ui::WaylandWindow::SetWindowIcons(const gfx::ImageSkia &, const gfx::ImageSkia &)
Received signal 11 SEGV_MAPERR 000000000000
#0 0x55e3e571eba9 base::debug::CollectStackTrace()
#1 0x55e3e564edb3 base::debug::StackTrace::StackTrace()
#2 0x55e3e571e631 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7fc8bb6e6ee0 (/nix/store/saw6nkqqqfx5xm1h5cpk7gxnxmw9wk47-glibc-2.33-62/lib/libpthread-2.33.so+0x12edf)
#4 0x55e3e1874e20 base::ObserverList<>::AddObserver()
#5 0x55e3e1879e64 electron::WindowStateWatcher::WindowStateWatcher()
#6 0x55e3e1869116 electron::NativeWindowViews::NativeWindowViews()
#7 0x55e3e186c94b electron::NativeWindow::Create()
#8 0x55e3e16c7210 electron::api::BaseWindow::BaseWindow()
#9 0x55e3e16da635 electron::api::BrowserWindow::BrowserWindow()
#10 0x55e3e16dcf3d electron::api::BrowserWindow::New()
#11 0x55e3e16dd475 gin_helper::internal::InvokeNew<>()
#12 0x55e3e16ce0bf gin_helper::Dispatcher<>::DispatchToCallback()
#13 0x55e3e2ebad08 v8::internal::FunctionCallbackArguments::Call()
#14 0x55e3e2eb7f0a v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#15 0x55e3e2eb6746 v8::internal::Builtin_Impl_HandleApiCall()
#16 0x55e3e2eb627d v8::internal::Builtin_HandleApiCall()
#17 0x2a8000093838 <unknown>
  r8: ffffffff00000000  r9: 00000001ffffffff r10: 00001fa801001310 r11: 000055e3eaadcb88
 r12: 00001fa800145e80 r13: 00007fff6eb9a560 r14: 00001fa800641300 r15: 00001fa800145ed8
  di: 00001fa800641300  si: 00001fa8010ca940  bp: 00007fff6eb99fa0  bx: 00001fa8010ca940
  dx: 00007fc8b6455160  ax: 0000000000000000  cx: 00001fa8007f8976  sp: 00007fff6eb99790
  ip: 000055e3e1874e20 efl: 0000000000010293 cgf: 002b000000000033 erf: 0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
[41047:0100/000000.834953:ERROR:gpu_init.cc(457)] Passthrough is not supported, GL is egl, ANGLE is 
Segmentation fault (core dumped)
[41047:0100/000000.408323:ERROR:sandbox_linux.cc(376)] InitializeSandbox() called with multiple threads in process gpu-process.

@Molytho
Copy link

Molytho commented Jan 28, 2022

I'm working on a backport for electron 13.
I think the issue is that chromium tries to map the keymap fd (from wl_keyboard:keymap) read-write but since version 7 this fd is readonly (at least that's what it does in version 13). This might be a bug of wlroots because I don't think that we actually use version 7 but mapping it readonly should also be possible on our side.

OK, maybe mapping it readonly is not enough. It also expects that we map it with MAP_PRIVATE which needs some change in the code base.

@trop
Copy link
Contributor

trop bot commented Jan 29, 2022

@Molytho has manually backported this PR to "13-x-y", please check out #32652

@Molytho
Copy link

Molytho commented Jan 31, 2022

The segmentation fault we get confuses me:

  1. Starting electron without an application doesn't segfault on my system (up-to-date arch)
  2. Starting electron with geogebra application (from arch repository) does segfault
  3. Starting geogebra with electron from arch repository (they also apply the patch) doesn't segfault.

I guess it has something to do with the build arguments :(

@primeos
Copy link
Author

primeos commented Jan 31, 2022

Thanks for the testing, suggestions, etc. Unfortunately I don't have much time and motivation for this so if someone wants to take over that'd be welcome (feel free to open a new PR based on this one).

@smasher164 thanks for testing this PR. I coincidentally also tested it with Sway on NixOS with pretty much the same approach (I didn't override headersFetcher but I also tried it with libglvnd, pciutils (Chrome started requiring it), and IIRC at least a third package). Unfortunately there's another Electron 16 regression (NixOS/nixpkgs@e06082e) so that I cannot properly test this PR as Ozone/Wayland is already broken, even on Wayland compositors that don't need this additional fix.

@Molytho IIRC I once saw that Chromium issue regarding wl_keyboard:keymap but I cannot find it anymore (edit: maybe https://bugs.chromium.org/p/chromium/issues/detail?id=1246834?). I can't quite follow your last comment (what's the difference between 2 and 3?). Can you use Electron 16.0.6+ via Ozone/Wayland (I can't due to #32436 or something similar)?

@Molytho
Copy link

Molytho commented Jan 31, 2022

@Molytho IIRC I once saw that Chromium issue regarding wl_keyboard:keymap but I cannot find it anymore (edit: maybe https://bugs.chromium.org/p/chromium/issues/detail?id=1246834?). I can't quite follow your last comment (what's the difference between 2 and 3?). Can you use Electron 16.0.6+ via Ozone/Wayland (I can't due to #32436 or something similar)?

Yes it's this issue. Looks like it is fix with later versions of chromium.

The difference between 2 and 3 is:
2 uses the electron version compiled by the CI run of this PR. This segfaults on startup.
3 uses https://archlinux.org/packages/community/x86_64/electron from the arch repository. This works fine.

Looks like Arch also suffered from this segfaults and fix it by building without -fstack-clash-protection
https://bugs.archlinux.org/task/73518

@miniak
Copy link
Contributor

miniak commented Mar 30, 2022

@primeos is this PR still relevant?

@prusnak
Copy link

prusnak commented Apr 1, 2022

@primeos is this PR still relevant?

I think it is, see #33355 (comment)

Btw, hi! :D

@primeos
Copy link
Author

primeos commented Apr 1, 2022

Yes, if it's a goal to fix Ozone/Wayland for Electron 16 then this PR is still required (it fixes #32487).
If not we can close this issue ;)

#33355 is the missing part to fix the rest of the issues/regressions.

I've managed to stop using Electron since the Ozone/Wayland support regressed so I'm not really motivated to backport the other PR as well. Does anyone have time and motivation to take this over?

@miniak
Copy link
Contributor

miniak commented Apr 4, 2022

Both Electron 15 and 16 are going to be discontinued when Electron 19 is released in May. This PR needs to be fixed and merged before that happens, otherwise it will just be abandoned as obsolete.

@VerteDinde
Copy link
Member

Hey all! We're going to be releasing our final 15-x-y and 16-x-y Electron builds this morning - since it looks like tests are still failing here, and it hasn't been touched in some time, I'm so sorry, but I'm going to close this PR. Happy to accept fixes and changes to later Electron versions if the issue persists!

@VerteDinde VerteDinde closed this May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
16-x-y backport-check-skip Skip trop's backport validity checking semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants