sakithb / media-controls

A media indicator for the Gnome shell.
MIT License
236 stars 36 forks source link

Multiple-delete causes GNOME 45 Shell segmentation fault #102

Closed Thesola10 closed 10 months ago

Thesola10 commented 11 months ago

I'm using GNOME Shell 45.0 with Mutter 45 API13, from the Arch Linux repository.

When unlocking the session and adding/removing a MPRIS player, the entire session crashes, leaving behind these logs:

gs-log.txt gnome-shell-coredump.split.zip gnome-shell-coredump.split.z01 gnome-shell-coredump.split.z02

ChrisLauinger77 commented 11 months ago

I see one warning in log from media-controls.

But a lot of those:

== Stack trace for context 0x5568fbb51c30 ==

0 5568fbc148d8 i file:///home/karim/.local/share/gnome-shell/extensions/quick-settings-tweaks@qwreey/libs/volumeMixerHandler.js:114 (3e3db13960b0 @ 71)

I do not use quick-settings-tweaks. And I do not have any issue. and the mutter version should be 45. at least in debian which I use the gnome-shell and mutter versions are 45

Thesola10 commented 11 months ago

I may have misread the log output. I disabled quick-settings-tweaks and will keep you updated on whether the crashes come back

Also Mutter has an API version as well, hence 13 (/lib/mutter-13)

ChrisLauinger77 commented 11 months ago

https://archlinux.org/packages/extra/x86_64/mutter/ The libmutter is 13 but libmutter and mutter are not the same thing

Thesola10 commented 11 months ago

The moment I decided to close this ticket, the crash happened again, this time wihout quick-settings-tweaks.

Thesola10 commented 11 months ago

Sorry, I realize I sent an unrelated excerpt due to the dense nature of journalctl logs. Here are the actual logs from media-controls, as evidenced by the following "Starting GNOME Wayland" line: gs-log.txt

The QSTweaks warnings are benign and never lead to a crash.

ChrisLauinger77 commented 11 months ago

oct. 27 22:10:33 SolaXBook gnome-shell[2099]: JS ERROR: TypeError: this._extension.settings is undefined

this I already fixed in main yesterday over this commit https://github.com/sakithb/media-controls/commit/397129a66ea0f1f5b621c6ed47e009e9697f8e48 can you please take the player.js and widget.js from main branch and see if this helps ?

Thesola10 commented 11 months ago

Seems to be working right now, will keep you posted if the crash happens again.

Stress-testing it by hovering over YouTube videos quickly (cf autoplay) doesn't seem to trigger the crash anymore.

Thesola10 commented 11 months ago

Crash happened again, same log readout. Seems to mostly happen after resuming from suspend.

danilo-alm commented 11 months ago

Gnome 45 just crashed for the second time on me and I think it's this extension as well. Here's the journal from when the crash happened and you can see the stack trace of the dumped core right after some errors mentioning the extension. If it helps, the crashes happened when I was pausing/resuming a video on brave-browser, specifically using my mouse side-buttons, mapped to KEY_PLAYPAUSE. My distro is Fedora 39 (beta).

ChrisLauinger77 commented 11 months ago

I see some crashes reported upstream - do you guys use a touchscreen / tablet ? I do not have any touch input device - I cannot reproduce those crashes. maybe report them directly to gnome-shell on their upstream gitlab ? https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/7144

danilo-alm commented 11 months ago

I didn't use my wacom tablet on that Fedora install, so no. After disabling the extension, I still had gnome crashing on me when returning from suspend, so I installed Fedora 38 and learned my lesson not to use beta software. That said, I will not report it since I will not be able to help them debug it

ChrisLauinger77 commented 11 months ago

@danilo-alm gnome 45 and the ESM modules are brand new - I expect there are some bugs to be solved upstream. But it does not happen to everybody - for me it runs OK.

Thesola10 commented 11 months ago

Crash now occurs reliably with these steps:

  1. Lock session
  2. Unlock session
  3. Add or remove MPRIS player, by closing or opening a player app that wasn't playing before
  4. Crash.
Thesola10 commented 11 months ago

PS: maybe test interaction with Dash to Panel?

ChrisLauinger77 commented 11 months ago

I use Dash to Dock - not a fan of Dash to Panel. Maybe a problem of Dash to Panel rather then a problem of media-controls ?

Thesola10 commented 11 months ago

I wouldn't dismiss Media Controls' involvement entirely -- no other extension, including Tray Icons Reloaded, has this kind of issue.

ChrisLauinger77 commented 11 months ago

I would argue that when the standard panel is used it does not crash. Maybe you can confirm it by disabling Dash to panel ? I have no time to analyze the code from dash to panel.

ChrisLauinger77 commented 11 months ago

So when we know it also works 4 U (without dash to panel) I would update the known issues section. Then maybe file a issue on Dash to panel

Thesola10 commented 11 months ago

Just confirmed, crash occurs without Dash to Panel as well

Maluscat commented 11 months ago

Crash now occurs reliably with these steps:

  1. Lock session
  2. Unlock session
  3. Add or remove MPRIS player, by closing or opening a player app that wasn't playing before
  4. Crash.

Crashes for me as well with the provided steps. No other extensions enabled. Here's my log if it is of any help :)

ChrisLauinger77 commented 11 months ago

Am Dienstag, dem 31.10.2023 um 17:54 -0700 schrieb Hallo89:

Crash now occurs reliably with these steps:    1. Lock session    2. Unlock session    3. Add or remove MPRIS player, by closing or opening a player app that wasn't playing before    4. Crash. Crashes for me as well with the provided steps. No other extensions enabled. Here's my log if it is of any help :) — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

I can reproduce it. I had 2 players playing (which i usually never have) and locked the session. Then I unlocked it. Closed 1st player - no crash. Closed 2nd player - crash.

ChrisLauinger77 commented 11 months ago

This needs to be reported upstream. Or any other suggestion @sakithb ? I try to reproduce this with an older gnome (before ESM modules are introduced) and see if this is also there (which I doubt)

ChrisLauinger77 commented 11 months ago

I tested it with mediacontrols 29 on gnome 43. No crash there - so its new on gnome 45 which changed a lot by introducing ESM modules. They need to have a look. @Thesola10 / @Hallo89 please report this issue upstream @ gnome-shell https://gitlab.gnome.org/GNOME/gnome-shell/-/issues

Thesola10 commented 11 months ago

I don't think a native Clutter crash necessarily means it's an upstream issue. The way GJS works, it's basically wired up to C Clutter calls directly, so double-deletes aren't unheard of. You shouldn't think of it as safe, managed JS, but more like a GObject-aware C FFI, much like Vala.

Thesola10 commented 11 months ago

Okay, narrowed it down. Locking the session calls disable() which destroys the Widget and its Players, but then on session resume, we're somehow still using the same Widget, which obviously crashes everything -- maybe because the Widget is trying to destroy() itself, taking down the Clutter instance but not the GJS object?

Removing all destroy() calls leads to a different issue: delete Main.panel.statusArea["media_controls_extension"] doesn't remove the widget from the panel, resulting in a duplicate widget.

ChrisLauinger77 commented 11 months ago

I don't think a native Clutter crash necessarily means it's an upstream issue. The way GJS works, it's basically wired up to C Clutter calls directly, so double-deletes aren't unheard of. You shouldn't think of it as safe, managed JS, but more like a GObject-aware C FFI, much like Vala.

We do the same things on earlier gnome versions and no crash happens. So is has something to do with the upstream changes for 45

ChrisLauinger77 commented 11 months ago

https://9to5linux.com/gnome-shell-and-mutter-45-1-released-with-xwayland-and-wayland-improvements

benjamin051000 commented 10 months ago

Just chiming in, I have experienced the same crash, although my device was unlocked for a while. I'm not exactly sure what I did to cause the crash. Looks like you guys have had plenty of discussion already, so let me know if you need any additional info from me.

Okay, just confirmed it crashes after I lock/unlock, just like the other reports here.

sakithb commented 10 months ago

Okay, narrowed it down. Locking the session calls disable() which destroys the Widget and its Players, but then on session resume, we're somehow still using the same Widget, which obviously crashes everything -- maybe because the Widget is trying to destroy() itself, taking down the Clutter instance but not the GJS object?

Removing all destroy() calls leads to a different issue: delete Main.panel.statusArea["media_controls_extension"] doesn't remove the widget from the panel, resulting in a duplicate widget.

I assume unlocking the session should call enable() which should create a new instance of the widget. If that is so, how can it be a problem?

Thesola10 commented 10 months ago

Because enable() is a method of the Clutter subclass that got destroyed?

sakithb commented 10 months ago

Because enable() is a method of the Clutter subclass that got destroyed?

That was just a thought. I'm not sure what is really happening, I'm not a clutter expert 😄 but shouldn't a new instance of the clutter subclass (MediaControls) be created when the enable method of the Extension instance (extension.js) is called?

ChrisLauinger77 commented 10 months ago

I just got gnome-shell and mutter updated to 45.1 in debian. and I cannot reproduce it any more. No change in the extension ...

benjamin051000 commented 10 months ago

Thanks for the timely fix! Cheers! 😁

Spurlos commented 10 months ago

The PR mentioned in discussion has fixed the issue for me. Looking forward for the proper release on the Gnome extensions repo