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

[Bug]: Desktop capturer segmentation Fault #36660

Closed
3 tasks done
mgonzalezg9 opened this issue Dec 14, 2022 · 26 comments
Closed
3 tasks done

[Bug]: Desktop capturer segmentation Fault #36660

mgonzalezg9 opened this issue Dec 14, 2022 · 26 comments

Comments

@mgonzalezg9
Copy link

mgonzalezg9 commented Dec 14, 2022

Preflight Checklist

Electron Version

19.0.17

What operating system are you using?

Ubuntu

Operating System Version

Weston wayland

What arch are you using?

arm64 (including Apple Silicon)

Last Known Working Electron version

No response

Expected Behavior

In the file background.ts (main process) I am calling desktopCapturer to retrieve the screen id:

const a = await desktopCapturer.getSources({ types: ['screen'] });
console.dir(a);

Actual Behavior

However when this line is invoked it prints Segmentation fault and the program crashes. I believe the problem may be related to the types array because when I set it empty it works without problems.

Testcase Gist URL

No response

Additional Information

No response

@nornagon
Copy link
Member

Please attach a crash dump. You can collect them by adding the following snippet to your main process code, before app.whenReady:

const { app, crashReporter } = require('electron')

console.log(app.getPath('crashDumps'))
crashReporter.start({ submitURL: '', uploadToServer: false })

Then reproduce the crash, zip up the crash dumps directory and attach it here.

@nornagon nornagon added the blocked/need-info ❌ Cannot proceed without more information label Dec 20, 2022
@mgonzalezg9
Copy link
Author

@SpacingBat3
Copy link

Operating System Version

Weston wayland

@mgonzalezg9 does weston actually implement portals backend needed for WebRTCPipeWireCapturer?

That being said, for me Electron didn't segfaulted on weston (both within my own app and REPL) if XWayland was present (on Electron 20 and 22), it just seemed to pick X11 server resources even with --enable-features=WebRTCPipeWireCapturer command-line flag…

Also, increasing the log verbosity in Electron seemed to confirm that for me it segfaulted because API was looking for X11 windows / desktops. Also the weston I've tested Electron with was 11.0.0.

@mgonzalezg9
Copy link
Author

Operating System Version

Weston wayland

@mgonzalezg9 does weston actually implement portals backend needed for WebRTCPipeWireCapturer?

That being said, for me Electron didn't segfaulted on weston (both within my own app and REPL) if XWayland was present (on Electron 20 and 22), it just seemed to pick X11 server resources even with --enable-features=WebRTCPipeWireCapturer command-line flag…

Also, increasing the log verbosity in Electron seemed to confirm that for me it segfaulted because API was looking for X11 windows / desktops. Also the weston I've tested Electron with was 11.0.0.

I think it must, because screen sharing is working perfectly in Chromium and it is the same underlying technology

@SpacingBat3
Copy link

@mgonzalezg9 could you share how you run weston session (weston.ini, command line flags)? For me it won't really work with the default configuration (empty config, no flags). I've both tried Chromium and Electron, same results (only X window/screen capture + browser cards).

I also couldn't really find any information about PipeWire support and how that should be enabled (I'm pretty sure it won't work out-of-the-box).

@mgonzalezg9
Copy link
Author

mgonzalezg9 commented Jan 11, 2023

@mgonzalezg9 could you share how you run weston session (weston.ini, command line flags)? For me it won't really work with the default configuration (empty config, no flags). I've both tried Chromium and Electron, same results (only X window/screen capture + browser cards).

I also couldn't really find any information about PipeWire support and how that should be enabled (I'm pretty sure it won't work out-of-the-box).

These are the contents of my weston.ini file:

[core]
# i.MX: Disable idle timeout
idle-time=0
gbm-format=argb8888
use-g2d=0

[libinput]
touchscreen_calibrator=true

[shell]
#panel-location="" 
#panel-position=none** 
background-image=/home/ventilator/fondoversatile1366766.png

[launcher]
icon=/home/ventilator/ib.png
path=DISPLAY=wayland-0 /usr/bin/inbentus-sav-21-frontend --no-sandbox --enable-features=UseOzonePlatform --ozone-platform=wayland

The launcher icon is for the app that is causing the segmentation fault.

Sorry for replying so late, I've been off for a week

@SpacingBat3
Copy link

@mgonzalezg9 I've actually tested how it works on GNOME and I can confirm the mentioned issue is reproducible on 22.x.y regardless of the portals presence, but I couldn't reproduce it in older Electron versions (I believe even on 19.x.y it wasn't reproducible)… And as you know, I managed to workaround it a bit with the hard-coded source, although a more appropriate fix would be needed given I have no clue what really triggers the PipeWireCapturer correctly (do we really need the exact screen source if there's a picker that will likely change it anyway?).

@mgonzalezg9
Copy link
Author

@SpacingBat3 well the underlying problem is that my app needs to record the whole screen without prompting the user, so that's why I thought in that solution using desktopCapturer. However I thought electron would use PipeWire internally

@mgonzalezg9
Copy link
Author

Is there another alternative to PipeWire that can make it work?

@SpacingBat3
Copy link

Is there another alternative to PipeWire that can make it work?

@mgonzalezg9 I'm not sure there's anything standardized as portals and PipeWire across different DE's.

(...) well the underlying problem is that my app needs to record the whole screen without prompting the user (...)

There are good reasons why this should not be possible on Wayland (mostly privacy concerns and overuses for malicious purposes), at least for regular user applications. Some people even mentioned that X11 was vulnerable because of how many permissions regular applications had within entire desktop.

These are the contents of my weston.ini file:

There's nothing special here, I assume most of your issues are due to the lack of PipeWire. As of the Chromium however, I believe it could work normally when I've tested it even without PipeWire and XWayland being present, but you would be unable to record anything related to desktop or other windows (only tabs recording were available) or with XWayland and without PipeWire support, you would be limited only to X11 windows.

@kris7t
Copy link

kris7t commented Feb 15, 2023

Since upgrading to Electron 23, my usual workaround of disabling WebRTCPipeWireCapturer and forcing the X11 ozone backend failed to work. In fact, Electron 23 crashes when trying to screenshare even if there is no org.freedesktop.portal.Desktop on the session D-Bus at all, so quite likely the crash happens even before enumerating the outputs to capture.

Downgrading to Electron 22.2.1 allows the workaround (only sharing XWayland windows via X11) to keep working.

@mgonzalezg9
Copy link
Author

Hi @kris7t, I tried with Electron 22.2.1 but I am still facing the same problem. I've been searching the web but cannot find a flag to run the app with xwayland, do you know any ways of testing that?

@kris7t
Copy link

kris7t commented Feb 16, 2023

@mgonzalezg9 I tried --ozone-backend=x11 (or just not passing any --ozone-backend, I believe x11 is still a default). That is not enough, though, you also need to make sure that the WebRTCPipeWireCapturer feature is not enabled, because Chromium can now also use pipewire on X11. Normally, WebRTCPipeWireCapturer is disabled unless you pass --enable-features=WebRTCPipeWireCapturer, but you may try passing --disable-features=WebRTCPipeWireCapturer.

Note that the application you're running might get started with any of these flags by default, and it may enable or disable feature flags at runtime. Check the launcher script or desktop entry, if any, and the source code of the application.

To see whether the application is using XWayland, refer to your compositor (e.g., sway lists XWayland clients differently in swaymsg -r -t get_tree), or you can also try something like xeyes to see whether other XWayland clients can listen for mouse events from the application.

@apocalyp0sys
Copy link

Hi. I was able to run electron on xwayland by removing environment variable.

if (process.platform === "linux") {
    delete process.env.WAYLAND_DISPLAY; // force to run under xwayland (or crash)
}

Unfortunately, desktopCapturer is not able to capture screen properly under xwayland, shared stream is a black rectangle with cursor. The app does not crash, at least.

@kris7t
Copy link

kris7t commented Feb 17, 2023

@apocalyp0sys you can still share individual applications, provided they run under XWayland, too (unless your compositor does something to isolate individual XWayland client by running multiple XWayland servers, but that's rare, I think).

@SpacingBat3
Copy link

FYI I've found a workaround myself for the crash, take a look at this comment:

SpacingBat3/WebCord#328 (comment)

Even through keeping things hard-coded doesn't seem to be the best idea, it seem to at least work to most people which set up PipeWire for screen sharing correctly. While @mgonzalezg9 suggested that it might not work for every environment, the number he got seemed to be associated to XWayland rather than PipeWire capture API.

Hopefully that will help you to deal with it for now and Electron team will react (at least update the labels – there's much more information about the issue now and my tests shown that versions older than 22-x-y are fine).

@mgonzalezg9
Copy link
Author

mgonzalezg9 commented Feb 20, 2023

Hi @SpacingBat3, I tried to figure out the id so that the workaround works but wasn't able :/
All that I have found is that process.env.DISPLAY has the string 'wayland-0', but setting that screen source gave me an error.

Any ideas of how to find it?

@SpacingBat3
Copy link

SpacingBat3 commented Feb 20, 2023

Hi @SpacingBat3, I tried to figure out the id so that the workaround works but wasn't able :/ All that I have found is that process.env.DISPLAY has the string 'wayland-0', but setting that screen source gave me an error.

Any ideas of how to find it?

I used Electron 20/21 where desktopCapturer actually works to get information about which ID is commonly used. Also I was on GNOME, which supports PipeWireCapturer, I believe on Weston this won't work as it doesn't have any kind of implementation for the PipeWire WebRTC (so flag --enable-features=WebRTCPipeWireCapturer on Weston probably won't change anything or will be a cause of crashes).

You might as well take a look at my code and fetch it from there, I won't mind doing that as I think that hard-coded object will have similar or exactly the same structure to be compliant with Electron types.

Take a note that you still might want to use desktopCapturer if process.versions.electron is older than 22.0.0, as it works and might be just more reliable (even through I had no reports that this workaround is broken or doesn't work with the specific enviroment).

@jrose-signal
Copy link

I dug into this a bit on Electron main and got past the crash with this change to WebRTC's desktop_media_list_base.cc:

diff --git a/chrome/browser/media/webrtc/desktop_media_list_base.cc b/chrome/browser/media/webrtc/desktop_media_list_base.cc
index 7809273017..a956f3c2dc 100644
--- a/chrome/browser/media/webrtc/desktop_media_list_base.cc
+++ b/chrome/browser/media/webrtc/desktop_media_list_base.cc
@@ -72,6 +72,11 @@ void DesktopMediaListBase::StartUpdating(DesktopMediaListObserver* observer) {
 void DesktopMediaListBase::Update(UpdateCallback callback, bool refresh_thumbnails) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(sources_.empty());
+
+  // If there is a delegated source list, it may not have been started yet.
+  if (IsSourceListDelegated())
+    StartDelegatedCapturer();
+
   DCHECK(!refresh_callback_);
   refresh_callback_ = std::move(callback);
   Refresh(refresh_thumbnails);

The general problem is that Electron's DesktopCapturer does not seem to "start" the "delegated capturer" (Wayland's native capture window) before calling this Update method. "Delegated" capturers that show their own UI follow slightly different code paths than the old style of capturers, and of course Chromium does something different from DesktopCapturer.

With this change I can get a toy example to show the Wayland picker and let me share the screen on Ubuntu 22.04.2 (though the picker comes up twice, possibly once each for the main and renderer processes, I didn't check). I don't think it's quite as simple as that, because sometimes when I quit I see the system still showing the "you are sharing your screen" status icon in the top right. I'm not a strong enough Pipewire, WebRTC, or Electron engineer to say whether this is the correct fix, but maybe an Electron person can now make progress here.

@jrose-signal
Copy link

Thinking about this further, it seems like desktopCapturer.getSources() should be able to immediately return the "placeholder" sources without the capturer actually hitting Pipewire, avoiding the double-dialog problem. This would be similar to the solution hardcoding the source ID as described above, but with the exact name to "hardcode" still generated within Chromium. (Hardcoding a source ID doesn't actually keep my toy project from crashing when the screenshare actually starts, though.)

@indutny-signal
Copy link
Contributor

cc @nornagon because you are the proud owner of a patch for webrtc 40e76dc 😂 Feel free to redirect it if needed! Thanks!

@hadogenes
Copy link

the #37511 may fix this

@jrose-signal
Copy link

That might fix the crash (unsure), but we still eventually do need the Wayland capturer, or we'll only be able to capture a subset of the local windows.

@mgonzalezg9
Copy link
Author

mgonzalezg9 commented Apr 20, 2023

Fixed the issue using the app with xwayland, desktopCapturer.getSources is not crashing this way.
This quick demo works.
Closing :)

@gza
Copy link

gza commented Jun 8, 2023

As @jrose-signal said, you can only share x11 application with this so-called "fix"

@mgonzalezg9 As the use case is clearly the screen sharing of a wayland environment, I'm not sure one can consider the problem as fixed because one can share the few old apps not supporting wayland (purpose of xwayland).

What about the patch in #36660 (comment) ? Is it a realistic fix or a workaround ?

@SpacingBat3
Copy link

What about the patch in #36660 (comment) ? Is it a realistic fix or a workaround ?

@gza As of workaround that doesn't require patching Electron, this is what I've been doing myself for quite a long time when my app detects Wayland environment and PipeWire capturer flag used:

https://github.com/SpacingBat3/WebCord/blob/f26e95bf17281d82a25bee82a4805136bd3f042b/sources/code/main/windows/main.ts#L503-L510

          // Workaround #328: Segfault on `desktopCapturer.getSources()` since Electron 22
          }) : Promise.resolve([{
            id: "screen:1:0",
            appIcon: nativeImage.createEmpty(),
            display_id: "",
            name: "Entire Screen",
            thumbnail: nativeImage.createEmpty()
          } satisfies Electron.DesktopCapturerSource]);

Somehow, hard-coding a source this way to avoid using desktopCapturer.getSources() looks like as a viable workaround – from my tests PipeWire capturer is triggered correctly when using this source.

I've also didn't test if desktopCapturer.getSources() still breaks on newer versions, so I don't confirm this issue is still relevant.

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

No branches or pull requests