Fix/segfault #32

[weisJ]

Working branch for #32

[txomon]

The client crashes before a notification can be sent

[weisJ]

I have added some checks and used a local settings instance for the signal connection (as it seems to be working for getTheme).

[txomon]

So I did something bad :), I used the C api instead of the C++ one and it works (but the C++ one still crashes), diff:

diff --git a/linux/gnome/src/main/cpp/DarkModeGnome.cpp b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
index e8ee7ec..8853148 100644
--- a/linux/gnome/src/main/cpp/DarkModeGnome.cpp
+++ b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
@@ -29,6 +29,7 @@
 #include <iostream>
 #include <glibmm-2.4/glibmm.h>
 #include <giomm-2.4/giomm.h>
+#include <gio/gio.h>

 constexpr auto SETTINGS_SCHEMA_NAME = "org.gnome.desktop.interface";
 constexpr auto THEME_NAME_KEY = "gtk-theme";
@@ -36,8 +37,16 @@ Glib::RefPtr<Gio::Settings> settings;

 JNIEXPORT jstring JNICALL
 Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_getCurrentTheme(JNIEnv *env, jclass) {
+    std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" << std::endl;
     auto settingsRef = Gio::Settings::create("org.gnome.desktop.interface");
     if (!settingsRef) std::cout << "Settings are null!" << std::endl;
+    std::cout << "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB" << std::endl;
+
+    GSettings *obj = settingsRef->gobj();
+    const gchar *val;
+    val = g_settings_get_string(obj, "gtk-theme");
+    std::cout << val << std::endl;
+    std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC" << std::endl;

     auto themeStr = settingsRef->get_string("gtk-theme");
     if (themeStr.empty()) std::cout << "Theme string is null!" << std::endl;

The output:

AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA            
BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB
Adwaita-dark                                                               
CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC
#                                                                          
# A fatal error has been detected by the Java Runtime Environment:
#                                                                             
#  SIGSEGV (0xb) at pc=0x00007feb9c658d42, pid=461109, tid=461144
#                                                                                                                                                                                                                                                                                                                             
# JRE version: OpenJDK Runtime Environment (11.0.10+9) (build 11.0.10+9)
# Java VM: OpenJDK 64-Bit Server VM (11.0.10+9, mixed mode, tiered, compressed oops, concurrent mark sweep gc, linux-amd64)
# Problematic frame:       
# C  [libgiomm-2.4.so.1+0x12ed42]  Gio::Settings::signal_changed(Glib::ustring const&)+0x2
#                           
# Core dump will be written. Default location: Core dumps may be processed with "/usr/lib/systemd/systemd-coredump %P %u %g %s %t %c %h" (or dumping to /home/javier/projects/spinoffs/auto-dark-mode/core.461109)
#                                                                                                                                                                                                                                                                                                                             
# An error report file with more information is saved as:
# /home/javier/java_error_in_webstorm_461109.log     
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp 
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#              

[weisJ]

Are you sure the segfault happens in Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_getCurrentTheme? There error says it happened when calling Gio::Settings::signal_changed(Glib::ustring const&) which is called in the EventHandler constructor.

[txomon]

You are right

The diff:

--- a/linux/gnome/src/main/cpp/DarkModeGnome.cpp
+++ b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
@@ -29,6 +29,7 @@
 #include <iostream>
 #include <glibmm-2.4/glibmm.h>
 #include <giomm-2.4/giomm.h>
+#include <gio/gio.h>

 constexpr auto SETTINGS_SCHEMA_NAME = "org.gnome.desktop.interface";
 constexpr auto THEME_NAME_KEY = "gtk-theme";
@@ -36,8 +37,16 @@ Glib::RefPtr<Gio::Settings> settings;

 JNIEXPORT jstring JNICALL
 Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_getCurrentTheme(JNIEnv *env, jclass) {
+    std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" << std::endl;
     auto settingsRef = Gio::Settings::create("org.gnome.desktop.interface");
     if (!settingsRef) std::cout << "Settings are null!" << std::endl;
+    std::cout << "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB" << std::endl;
+
+    GSettings *obj = settingsRef->gobj();
+    const gchar *val;
+    val = g_settings_get_string(obj, "gtk-theme");
+    std::cout << val << std::endl;
+    std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC" << std::endl;

     auto themeStr = settingsRef->get_string("gtk-theme");
     if (themeStr.empty()) std::cout << "Theme string is null!" << std::endl;
@@ -83,6 +92,8 @@ struct EventHandler {
     EventHandler(JavaVM *jvm_, jobject callback_) {
         jvm = jvm_;
         callback = callback_;
+        if (!settings) std::cout << "EventHandler => Settings are null!" << std::endl;
+        std::cout << "DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD" << std::endl;
         settingsChangedSignalConnection = settings->signal_changed(THEME_NAME_KEY).connect(
                 sigc::mem_fun(this, &EventHandler::settingChanged));
     }

The output:

AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB
Adwaita-dark
CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC
EventHandler => Settings are null!
DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f6c2095fd42, pid=464134, tid=464168
#
# JRE version: OpenJDK Runtime Environment (11.0.10+9) (build 11.0.10+9)
# Java VM: OpenJDK 64-Bit Server VM (11.0.10+9, mixed mode, tiered, compressed oops, concurrent mark sweep gc, linux-amd64)
# Problematic frame:
# C  [libgiomm-2.4.so.1+0x12ed42]  Gio::Settings::signal_changed(Glib::ustring const&)+0x2
#
# Core dump will be written. Default location: Core dumps may be processed with "/usr/lib/systemd/systemd-coredump %P %u %g %s %t %c %h" (or dumping to /home/javier/projects/spinoffs/auto-dark-mode/core.464134)
#
# An error report file with more information is saved as:
# /home/javier/java_error_in_webstorm_464134.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

[weisJ]

The current state of this branch adds some more logging and replaces the global settings in favour of a local ref in the EventHandler. Could you try whether this prevents the segfault?

[txomon]

Yeah, this fixes it! :)

However although the plugin takes care to adapt the system theme on startup, the event subscription doesn't seem to be working.

[weisJ]

Yes that is a known issue (#27) which I sadly wasn't able to debug as the theme would change when running the runIde task but not for a normal installation.

Does the theme also not follow the system using runIde for you? Also is there any console output stating that the created settings are null?

[txomon]

Does the theme also not follow the system using runIde for you?

Yes, it also doesn't work in runIde

Also is there any console output stating that the created settings are null?

No, there is no output

Additionally I have added:

diff --git a/linux/gnome/src/main/cpp/DarkModeGnome.cpp b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
index a336fb2..f0fd166 100644
--- a/linux/gnome/src/main/cpp/DarkModeGnome.cpp
+++ b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
@@ -52,6 +52,8 @@ struct EventHandler {
     sigc::connection settingsChangedSignalConnection;

     void settingChanged(const Glib::ustring &name) {
+        std::cout << "Theme change detected" << std::endl;
+        std::cout << name << std::endl;
         runCallBack();
     }

And it doesn't output anything, which makes me thing the event subscription is not getting properly set up

[txomon]

Found https://developer.gnome.org/glibmm/stable/classGio_1_1Settings.html#a36ef647876b1a813a4eb67d9be75c6e6

Note that settings only emits this signal if you have read key at least once while a signal handler was already connected for key.

Maybe this is why?

[txomon]

So the code in fd140d8 wasn't compiling, tried to fix it with:

diff --git a/linux/gnome/src/main/cpp/DarkModeGnome.cpp b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
index 1cf7b66..23a343c 100644
--- a/linux/gnome/src/main/cpp/DarkModeGnome.cpp
+++ b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
@@ -32,11 +32,11 @@

 #define SETTINGS_SCHEMA_NAME "org.gnome.desktop.interface"
 #define THEME_NAME_KEY "gtk-theme"
-Gio::Settings settings;
+Gio::Settings *settings;

 JNIEXPORT jstring JNICALL
 Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_getCurrentTheme(JNIEnv *env, jclass) {
-    auto themeStr = settings.get_string("gtk-theme");
+    auto themeStr = settings->get_string("gtk-theme");
     if (themeStr.empty()) std::cout << "Theme string is null!" << std::endl;

     return env->NewStringUTF(themeStr.c_str());
@@ -82,7 +82,7 @@ struct EventHandler {
         callback = callback_;

         if (!settings) std::cout << "Settings are null" << std::endl;
-        settingsChangedSignalConnection = settings.signal_changed(THEME_NAME_KEY).connect(
+        settingsChangedSignalConnection = settings->signal_changed(THEME_NAME_KEY).connect(
                 sigc::mem_fun(this, &EventHandler::settingChanged));
     }
 };
@@ -114,5 +114,5 @@ Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_init(JNIEnv *env
     ensure_gio_init();
     auto settingsRef = Gio::Settings::create(SETTINGS_SCHEMA_NAME).get();
     if (!settingsRef) std::cout << "Created settings are null!" << std::endl;
-    settings = *settingsRef
+    settings = settingsRef;
 }

But went back to the segfault:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007fcb8155f689, pid=475830, tid=475865
#
# JRE version: OpenJDK Runtime Environment (11.0.10+9) (build 11.0.10+9)
# Java VM: OpenJDK 64-Bit Server VM (11.0.10+9, mixed mode, tiered, compressed oops, concurrent mark sweep gc, linux-amd64)
# Problematic frame:
# C  [libgiomm-2.4.so.1+0x12c689]  Gio::Settings::get_string(Glib::ustring const&) const+0x19
#
# Core dump will be written. Default location: Core dumps may be processed with "/usr/lib/systemd/systemd-coredump %P %u %g %s %t %c %h" (or dumping to /home/javier/projects/spinoffs/auto-dark-mode/core.475830)
#
# An error report file with more information is saved as:
# /home/javier/java_error_in_webstorm_475830.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

Edit Additionally, I believe Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_init is not getting called

[weisJ]

You are right that Java_com_github_weisj_darkmode_platform_linux_gnome_GnomeNative_init didn't actually get called. It's a rather niche issue though. It gets called in GnomeThemeMonitorService#install which gets called when the actual ThemeMonitor is created i.e. here https://github.com/weisJ/auto-dark-mode/blob/883f96b3885e329042c6a33b9c538bfbe2449770/base/src/main/java/com/github/weisj/darkmode/platform/ThemeMonitorImpl.kt#L47

The tricky part is that GnomeThemeMonitorService acts as a delegate for LinuxThemeMonitorService and the ThemeMonitorService#install method is annotated using @JvmDefault. But for some peculiar reasons Jvm-default interface methods won't call delegates. E.g. in this scenario the output is Default bar() implementation

interface Foo {
    @JvmDefault
    fun bar() {
        println("Default bar() implementation")
    }
}

class FooImpl : Foo {
    override fun bar() {
        println("Explicit bar() implementation")
    }
}

class FooDelegated : Foo by FooImpl()

fun main() {
    FooDelegated().bar()
}

Found https://developer.gnome.org/glibmm/stable/classGio_1_1Settings.html#a36ef647876b1a813a4eb67d9be75c6e6

Note that settings only emits this signal if you have read key at least once while a signal handler was already connected for key.

Maybe this is why?

That is interesting. Lets try to add an explicit read request and see what happens.

[txomon]

I'm afraid the problem #27 is still there. thankfully the plugin no longer crashes. These are the logs on the terminal, which seems to suggest that there is no printing from the different cout lines.

I tried to add a manual trace on the settingChanged listener but it didn't seem to get triggered..

diff --git a/linux/gnome/src/main/cpp/DarkModeGnome.cpp b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
index edb530a..004b4d1 100644
--- a/linux/gnome/src/main/cpp/DarkModeGnome.cpp
+++ b/linux/gnome/src/main/cpp/DarkModeGnome.cpp
@@ -49,6 +49,7 @@ struct EventHandler {
     sigc::connection settingsChangedSignalConnection;

     void settingChanged(const Glib::ustring &name) {
+        std::cout << "Setting changed! " << name << std::endl;
         runCallBack();
     }

@@ -86,6 +87,7 @@ struct EventHandler {
                 sigc::mem_fun(this, &EventHandler::settingChanged));
         // Request key to ensure updates are send to the signal handler.
         auto currentTheme = settings->get_string(THEME_NAME_KEY);
+        std::cout << "Initial theme when setting listener is " << currentTheme << std::endl;
     }
 };

Output:

...
Initial theme when setting listener is Adwaita
...

[weisJ]

I think I have found a solution for this problem. The events couldn't be propagated to the signal handler due to a missing event loop. Now I wonder why the code even worked in the first place.

[txomon]

From conversations in GNOME IRC, it seems like we also need to make it settings->get_string("changed::"THEME_NAME_KEY); following documentation, however after making locally the change it doesn't seem to be working.

Oh event loops, nice... haha

[txomon]

Heh... the tests... actually run on the system and update my theme...

[txomon]

It works!!! Great work diving into this, I have no idea how you did it but thanks!

[weisJ]

From conversations in GNOME IRC, it seems like we also need to make it settings->get_string("changed::"THEME_NAME_KEY); following documentation, however after making locally the change it doesn't seem to be working.

Oh event loops, nice... haha

This seems to be related to the c-api and isn't necessary with the c++ bindings.

I think there might be still some room for improvement. From what I read the MainLoop is simply a glorified "hot" loop. However there seems to be a way to run one iteration of the loop manually in a blocking fashion where the thread is idle if no event is sent.

[txomon]

I think there might be still some room for improvement. From what I read the MainLoop is simply a glorified "hot" loop. However there seems to be a way to run one iteration of the loop manually in a blocking fashion where the thread is idle if no event is sent.

Because it's on a thread and an event loop, won't it just be sleep until there is activity coming from the signals?

From my side everything is working already. I would only like to mention that I had to patch manually the includes and linking steps because the outputs of pkg-config are slightly different to the ones committed. Would it make sense to use the original pkg-config calls instead of the hardcoded links?

diff --git a/linux/gnome/build.gradle.kts b/linux/gnome/build.gradle.kts
index 914a940..2fa222e 100644
--- a/linux/gnome/build.gradle.kts
+++ b/linux/gnome/build.gradle.kts
@@ -26,34 +26,33 @@ library {
                         is Gcc, is Clang -> listOf(
                             "--std=c++11",
                             "-pthread",
-                            "-I/usr/include/giomm-2.4",
-
 "-I/usr/lib/x86_64-linux-gnu/giomm-2.4/include",
-                            "-I/usr/include/glibmm-2.4",
-
 "-I/usr/lib/x86_64-linux-gnu/glibmm-2.4/include",
-                            "-I/usr/include/sigc++-2.0",
-
 "-I/usr/lib/x86_64-linux-gnu/sigc++-2.0/include",
-                            "-I/usr/include/gtk-3.0",
-                            "-I/usr/include/at-spi2-atk/2.0",
-                            "-I/usr/include/at-spi-2.0",
-                            "-I/usr/include/dbus-1.0",
-                            "-I/usr/lib/x86_64-linux-gnu/dbus-1.0/include",
-                            "-I/usr/include/gtk-3.0",
-                            "-I/usr/include/gio-unix-2.0",
-                            "-I/usr/include/cairo",
-                            "-I/usr/include/pango-1.0",
-                            "-I/usr/include/fribidi",
-                            "-I/usr/include/harfbuzz",
                             "-I/usr/include/atk-1.0",
+                            "-I/usr/include/at-spi-2.0",
+                            "-I/usr/include/at-spi2-atk/2.0",
+                            "-I/usr/include/blkid",
                             "-I/usr/include/cairo",
-                            "-I/usr/include/pixman-1",
-                            "-I/usr/include/uuid",
+                            "-I/usr/include/cloudproviders",
+                            "-I/usr/include/dbus-1.0",
                             "-I/usr/include/freetype2",
-                            "-I/usr/include/libpng16",
+                            "-I/usr/include/fribidi",
                             "-I/usr/include/gdk-pixbuf-2.0",
-                            "-I/usr/include/libmount",
-                            "-I/usr/include/blkid",
+                            "-I/usr/include/giomm-2.4",
+                            "-I/usr/include/gio-unix-2.0",
                             "-I/usr/include/glib-2.0",
-                            "-I/usr/lib/x86_64-linux-gnu/glib-2.0/include"
+                            "-I/usr/include/glibmm-2.4",
+                            "-I/usr/include/gtk-3.0",
+                            "-I/usr/include/harfbuzz",
+                            "-I/usr/include/libmount",
+                            "-I/usr/include/libpng16",
+                            "-I/usr/include/lzo",
+                            "-I/usr/include/pango-1.0",
+                            "-I/usr/include/pixman-1",
+                            "-I/usr/include/sigc++-2.0",
+                            "-I/usr/lib/dbus-1.0/include",
+                            "-I/usr/lib/giomm-2.4/include",
+                            "-I/usr/lib/glib-2.0/include",
+                            "-I/usr/lib/glibmm-2.4/include",
+                            "-I/usr/lib/sigc++-2.0/include"
                         )
                         else -> emptyList()
                     }
@@ -62,7 +61,7 @@ library {
                 // Build type not modeled yet, assuming release
                 compilerArgs.addAll(toolChain.map {
                     when (it) {
-                        is Gcc, is Clang -> listOf("-O2")
+                        is Gcc, is Clang -> emptyList()
                         else -> emptyList()
                     }
                 })
@@ -77,6 +76,7 @@ library {
                             "-lsigc-2.0",
                             "-lgtk-3",
                             "-lgdk-3",
+                            "-lz",
                             "-lpangocairo-1.0",
                             "-lpango-1.0",
                             "-lharfbuzz",

[weisJ]

I think there might be still some room for improvement. From what I read the MainLoop is simply a glorified "hot" loop. However there seems to be a way to run one iteration of the loop manually in a blocking fashion where the thread is idle if no event is sent.

Because it's on a thread and an event loop, won't it just be sleep until there is activity coming from the signals?

Apparently the default behaviour of the MainLoop is to not block. I have implemented the alternative approach. Now it would be good to know whether there is any difference in CPU consumption between the two versions. I think the easiest way to test this would be to update GnomeNativeTest to wait a long time for a theme change and see what the CPU consumption of the gradle process is

diff --git a/plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt b/plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt
--- a/plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt  (revision a76f7984883fb74c94affadf480363da50b47e2c)
+++ b/plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt  (date 1617925348523)
@@ -90,10 +90,10 @@
             countDownLatch.countDown()
         }!!

-        "gsettings set $settingsPath $settingsKey Adwaita".runCommand()
-        assertEquals("Adwaita", service.currentGtkTheme)
+        // "gsettings set $settingsPath $settingsKey Adwaita".runCommand()
+        // assertEquals("Adwaita", service.currentGtkTheme)

-        countDownLatch.await(10, TimeUnit.SECONDS)
+        countDownLatch.await(10, TimeUnit.MINUTES)
         assertTrue(countDownLatch.count == 0L)

         assertEquals(currentGtkTheme(), service.currentGtkTheme)

From my side everything is working already. I would only like to mention that I had to patch manually the includes and linking steps because the outputs of pkg-config are slightly different to the ones committed. Would it make sense to use the original pkg-config calls instead of the hardcoded links?

Yes surely. The flags are taken one-to-one from the output of pkg-config. Instead of taking your new values I simply made use of the pkg-config output directly and passed it to the compiler/linker.

[txomon]

I'm doing the tests with env JAVA_HOME=/usr/lib/jvm/java-15-openjdk/ time ./gradlew clean build -PisPublished=false not sure if this is good enough.

3.80user 0.55system 10:28.96elapsed 0%CPU (0avgtext+0avgdata 109868maxresident)k
70536inputs+0outputs (283major+35097minor)pagefaults 0swaps

[txomon]

Also as a side note, I have realised that the builds of the plugin are very version constrained. I don't understand how exactly, but do you think it would be doable to have as much cross-version compatibility as possible? I have tried several times (and failed) to set up intellij.updateSinceUntilBuild = false so that it works across all intellij editors and versions but I always end up with a plugin release that breaks with today's upgrade.

[weisJ]

The versions are constraint by virtue of how they are distributed. Jetbrains doesn't allow plugins which don't have a upper bound of supported versions. Instead one needs to deliberately upload a new version supporting the newer IDEA builds. Otherwise there is no guarantee of binary compatibility. The appropriate versions to change can be found in https://github.com/weisJ/auto-dark-mode/blob/master/gradle.properties

[txomon]

So for the other test, I reverted 9b26cc7, run again, and the result is:

3.16user 0.54system 10:22.81elapsed 0%CPU (0avgtext+0avgdata 110580maxresident)k
62192inputs+0outputs (198major+34533minor)pagefaults 0swaps

I would personally advocate for the reverted solution given that there is not a really big difference and the code looks much easier to maintain.

[weisJ]

I managed to setup a VM to run some test and could confirm that there is no noticeable difference. For each run I waited 15 seconds before changing the theme and measured how long the thread spend non idle.

MainContext::iteration blocking: ~1.5ms
MainContext::iteration non-blocking: ~50ms
MainLoop::run: ~1.5ms

I'll revert back to the version using a Glib::MainLoop.