Bug 275366 - gtk-chtheme crashes when changing from oxygen-gtk to another style
Summary: gtk-chtheme crashes when changing from oxygen-gtk to another style
Status: CLOSED FIXED
Alias: None
Product: Oxygen
Classification: Plasma
Component: gtk2-engine (show other bugs)
Version: unspecified
Platform: Unlisted Binaries Linux
: NOR crash
Target Milestone: ---
Assignee: Hugo Pereira Da Costa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-11 00:27 UTC by Ruslan Kabatsayev
Modified: 2012-05-15 04:10 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
attempted patch (835 bytes, patch)
2011-06-12 22:08 UTC, Hugo Pereira Da Costa
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ruslan Kabatsayev 2011-06-11 00:27:08 UTC
Version:           unspecified
OS:                Linux



Reproducible: Always

Steps to Reproduce:
1. Make sure oxygen-gtk is selected as default gtk theme
2. Start gtk-chtheme
3. Change selection from oxygen-gtk to another style, e.g. Null
4. See crash



It's a regression introduced by this commit:

aa454297b5a365123ff971640d6e670f0d5117d7 is the first bad commit
commit aa454297b5a365123ff971640d6e670f0d5117d7
Author: Hugo Pereira Da Costa <hugo@oxygen-icons.org>
Date:   Wed Jun 1 11:57:06 2011 +0200

    use root window of default screen to create ref surface.
    CCBUG: 274623

:040000 040000 0b2f5f5bdc87656426bdce895c04d9abef400c14 85d14d5b1ae9a2f3be843b302e05ba5091a527ce M      src
Comment 1 Ruslan Kabatsayev 2011-06-11 01:05:45 UTC
In fact, in this case liboxygen-gtk.so gets unloaded before most (or all) of destructors are called. dlclose is called by gtk_theme_engine_unload(), which in turn calls theme->exit(), which points to our theme_exit() in oxygentheme.cpp. But theme_exit() is empty. I think all the destruction should be done there.
Comment 2 Hugo Pereira Da Costa 2011-06-12 22:08:25 UTC
Created attachment 60938 [details]
attempted patch

Hello Ruslan,
I had similar issues in the past, but somehow don't have them anymore.
Anyway, your analysis about theme_exit() sounds convincing. 
I left it empty in the past, because it is actually not called (ever) for 90% of the apps. So I got lazy.

The attached patch deletes oxygen::Style::Intance() in theme_exit, and contains some safety belt in case the deletion should not have happened (namely, the singleton gets reset to 0, so that everything gets "re-initialized" if the instance is attempted to be re-used after theme_exit is called.

Deleting Style::instance() should be enough to have everything cleaned-up (that's why I always tried to keep all objects as children of this guy in the past ;)). Tell me if 
- it fixes the issue for you,  
- you don't see any suspicious behavior with other apps (I have not as far as I've tested)
- and feel free to commit + close the bugs, if the above two are fulfilled.

Cheers,

Hugo
Comment 3 Ruslan Kabatsayev 2011-06-12 22:33:53 UTC
No, your patch doesn't fix this bug.
Also, now i've found what part of code gets called when the library is already unloaded. It's InnerShadowData::targetExposeEvent(). But i currently don't understand why it doesn't get disconnected in time...
Comment 4 Ruslan Kabatsayev 2011-06-12 22:37:10 UTC
On the other hand, disabling inner shadow hack doesn't remove this bug. It instead leads to even less informative backtrace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb6fa5800 (LWP 26524)]
0xb763401b in g_main_context_prepare (context=0x8071c00, priority=0xbffff39c) at gmain.c:2757
2757              prepare = source->source_funcs->prepare;
(gdb) bt
#0  0xb763401b in g_main_context_prepare (context=0x8071c00, priority=0xbffff39c) at gmain.c:2757
#1  0xb7634d07 in g_main_context_iterate (context=0x8071c00, block=1, dispatch=1, self=0x80534d0) at gmain.c:3071
#2  0xb763568a in g_main_loop_run (loop=0x8654010) at gmain.c:3299
#3  0xb7cbd7a5 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#4  0x0804d4ba in main (argc=1, argv=0xbffff524) at main.c:231

I suspect this in fact might be GTK bug... still have to understand how to reduce the case.
Comment 5 Ruslan Kabatsayev 2011-10-28 22:15:01 UTC
New strange facts: commenting out the block of code from gdk_screen_get_default() to "} else" which leaves only image surface creation still leads to crash.
At the same time, creating a GtkWindow (and even just g_object_newv(GTK_TYPE_WINDOW,0,NULL)) somewhere in this code prevents crash.
Comment 6 Hugo Pereira Da Costa 2012-03-07 17:11:08 UTC
Git commit 8ddab0bfb6a7377b28206cbc1463b8aa8c08a552 by Hugo Pereira Da Costa.
Committed on 07/03/2012 at 15:07.
Pushed by hpereiradacosta into branch 'master'.

Make sure signals and hooks are all properly disconnected at deletion of Oxygen::Style::Instance
Delete TimeLine::Server in theme_exit.

Addresses (partially) https://bugs.mageia.org/show_bug.cgi?id=2679
To my understanding, there is still a crash due to DBus (still working on it),
and issues with gtk_window_set_composite (reverting to old value does not seem to work. Might be a GTk bug).

M  +11   -0    src/animations/oxygenanimations.cpp
M  +1    -1    src/animations/oxygencomboboxdata.h
M  +1    -1    src/animations/oxygencomboboxentrydata.cpp
M  +1    -1    src/animations/oxygencomboboxentrydata.h
M  +21   -1    src/animations/oxygenhook.cpp
M  +2    -2    src/animations/oxygenhoverdata.cpp
M  +1    -1    src/animations/oxygenhoverdata.h
M  +3    -3    src/animations/oxygeninnershadowdata.cpp
M  +5    -2    src/animations/oxygeninnershadowdata.h
M  +1    -1    src/animations/oxygenmainwindowdata.h
M  +1    -1    src/animations/oxygenmenubarstatedata.cpp
M  +1    -1    src/animations/oxygenmenubarstatedata.h
M  +1    -1    src/animations/oxygenmenustatedata.h
M  +4    -1    src/animations/oxygenpaneddata.h
M  +1    -1    src/animations/oxygenscrollbardata.cpp
M  +1    -1    src/animations/oxygenscrollbardata.h
M  +1    -1    src/animations/oxygenscrolledwindowdata.cpp
M  +1    -1    src/animations/oxygenscrolledwindowdata.h
M  +20   -1    src/animations/oxygensignal.cpp
M  +1    -1    src/animations/oxygentabwidgetdata.h
M  +13   -1    src/animations/oxygentimelineserver.cpp
M  +1    -1    src/animations/oxygentoolbarstatedata.h
M  +1    -1    src/animations/oxygentreeviewdata.cpp
M  +1    -1    src/animations/oxygentreeviewdata.h
M  +9    -1    src/oxygenargbhelper.cpp
M  +17   -0    src/oxygendbus.cpp
M  +2    -4    src/oxygendbus.h
M  +2    -4    src/oxygenshadowhelper.cpp
M  +13   -1    src/oxygenstylehelper.cpp
M  +1    -2    src/oxygenstylehelper.h
M  +3    -0    src/oxygentheme.cpp
M  +8    -0    src/oxygenwindowmanager.cpp

http://commits.kde.org/oxygen-gtk/8ddab0bfb6a7377b28206cbc1463b8aa8c08a552
Comment 7 Ruslan Kabatsayev 2012-03-07 18:59:56 UTC
In fact, as I've now tested, DBus doesn't affect this bug. It's enough to disable inner shadows hack to have this working, so inner shadows seem to be the only problem (at least for gtk-chtheme).
Comment 8 Ruslan Kabatsayev 2012-03-07 19:19:02 UTC
Oops, I was wrong. In fact, the following is true:
1. If I disable only DBus, there's no crash, but treeview is no longer drawn
2. Disabling both DBus and inner shadows hack makes everything work
Comment 9 Ruslan Kabatsayev 2012-03-07 20:48:48 UTC
Now after commit fe399f97bd478a33241a5a4213498072292ce841, inner shadows problem is solved, and now only DBus usage bug remains.
Comment 10 Hugo Pereira Da Costa 2012-03-08 12:29:51 UTC
mmm Thinking again, the commit 8ddab0bfb6a7377b28206cbc1463b8aa8c08a552
Might have introduced a number of regressions (basically, signal being disconnected while inserted in the map). I need to double-check.

For your observation about the interplay with dbus and inner-shadow-hack I can reproduce the same observations as you. The dbus issue might be a bug in the glib-dbus library.
I've read that dbus support is now built-in glib starting from version 2.26, so I guess I'll try this instead, and disable dbus for too old versions of glib.

And, thanks for the fix for the inner-shadow-hack !
(hard to spot typo)

PS: I'd like to have this fixed and backported before releasing new versions of 1.2 (and gtk3), due to the mageia bug. At least if fix becomes available within ~1 week.
Comment 11 Hugo Pereira Da Costa 2012-03-08 12:41:07 UTC
ok. After double checking, my changes are safe.
I'll merge, and focus on dbus.
Comment 12 Hugo Pereira Da Costa 2012-03-08 13:34:44 UTC
Git commit cf3628cfdbb5eed04aba5d7ff9894c2c5544fe5c by Hugo Pereira Da Costa.
Committed on 08/03/2012 at 14:30.
Pushed by hpereiradacosta into branch '1.2'.

Make sure signals and hooks are all properly disconnected at deletion of Oxygen::Style::Instance
Delete TimeLine::Server in theme_exit.

Addresses (partially) https://bugs.mageia.org/show_bug.cgi?id=2679
To my understanding, there is still a crash due to DBus (still working on it),
and issues with gtk_window_set_composite (reverting to old value does not seem to work. Might be a GTk bug).

Conflicts:

	src/animations/oxygeninnershadowdata.cpp

M  +11   -0    src/animations/oxygenanimations.cpp
M  +1    -1    src/animations/oxygencomboboxdata.h
M  +1    -1    src/animations/oxygencomboboxentrydata.cpp
M  +1    -1    src/animations/oxygencomboboxentrydata.h
M  +21   -1    src/animations/oxygenhook.cpp
M  +2    -2    src/animations/oxygenhoverdata.cpp
M  +1    -1    src/animations/oxygenhoverdata.h
M  +2    -2    src/animations/oxygeninnershadowdata.cpp
M  +5    -2    src/animations/oxygeninnershadowdata.h
M  +1    -1    src/animations/oxygenmainwindowdata.h
M  +1    -1    src/animations/oxygenmenubarstatedata.cpp
M  +1    -1    src/animations/oxygenmenubarstatedata.h
M  +1    -1    src/animations/oxygenmenustatedata.h
M  +4    -1    src/animations/oxygenpaneddata.h
M  +1    -1    src/animations/oxygenscrollbardata.cpp
M  +1    -1    src/animations/oxygenscrollbardata.h
M  +1    -1    src/animations/oxygenscrolledwindowdata.cpp
M  +1    -1    src/animations/oxygenscrolledwindowdata.h
M  +20   -1    src/animations/oxygensignal.cpp
M  +1    -1    src/animations/oxygentabwidgetdata.h
M  +13   -1    src/animations/oxygentimelineserver.cpp
M  +1    -1    src/animations/oxygentoolbarstatedata.h
M  +1    -1    src/animations/oxygentreeviewdata.cpp
M  +1    -1    src/animations/oxygentreeviewdata.h
M  +9    -1    src/oxygenargbhelper.cpp
M  +17   -0    src/oxygendbus.cpp
M  +2    -4    src/oxygendbus.h
M  +2    -4    src/oxygenshadowhelper.cpp
M  +13   -1    src/oxygenstylehelper.cpp
M  +1    -2    src/oxygenstylehelper.h
M  +3    -0    src/oxygentheme.cpp
M  +8    -0    src/oxygenwindowmanager.cpp

http://commits.kde.org/oxygen-gtk/cf3628cfdbb5eed04aba5d7ff9894c2c5544fe5c
Comment 13 Hugo Pereira Da Costa 2012-03-08 17:07:59 UTC
Git commit f34a6758417ef5bc4fb51d20ce6f6ff8c8032bd8 by Hugo Pereira Da Costa.
Committed on 08/03/2012 at 15:49.
Pushed by hpereiradacosta into branch 'master'.

implement use of glib's build-in dbus support if glib version is new enough (2.26).
Fallback to old glib-dbus implementation, if fails.

M  +21   -9    CMakeLists.txt
M  +1    -0    config.h.cmake
M  +119  -44   src/oxygendbus.cpp
M  +38   -22   src/oxygendbus.h
M  +1    -1    src/oxygenstylewrapper.cpp
M  +1    -0    src/oxygentheme.cpp

http://commits.kde.org/oxygen-gtk/f34a6758417ef5bc4fb51d20ce6f6ff8c8032bd8
Comment 14 Hugo Pereira Da Costa 2012-03-08 17:09:48 UTC
Latest commit, if glib is recent enough, fixes the crash for me.
(gtk-chtheme still crashes every now and then when swithcing theme +a lot+, but I am not sure it is our fault anymore).

@Ruslan, please confirm, and double check.

I also checked (by artificially increasing the required glib version number in CMakeLists.txt), that the old implementation still works, and still crashes.
Comment 15 Hugo Pereira Da Costa 2012-03-09 11:30:38 UTC
Git commit ac6c14cfedafdb08ffe8f0d00febb56e62a65e82 by Hugo Pereira Da Costa.
Committed on 09/03/2012 at 11:56.
Pushed by hpereiradacosta into branch 'gtk3'.

moved disconnect call outside of destructor of Oxygen::DBus
call disconnect explicitely in theme_exit()
comment out disconnect call, to prevent oxygen-gtk-chtheme crash.

M  +1    -4    src/oxygendbus.cpp
M  +3    -0    src/oxygentheme.cpp

http://commits.kde.org/oxygen-gtk/ac6c14cfedafdb08ffe8f0d00febb56e62a65e82
Comment 16 Hugo Pereira Da Costa 2012-03-09 13:07:19 UTC
@Ruslan,
1/ I was able to reproduce crashes even after commits from #13, especially when running with gdb.
Seems like there is some serious memory corruption when disconnecting the dbus interface.

2/ would you double-check that after commit from #15 the crash is gone ? 
Here it is gone (both running with and without gdb), although sadly enough there are other issues, namely:
if you switch away from oxygen and back to oxygen, then any update on the settings will crash, because of the unclear dangling connection ...

Still. Keep me posted.
PS: I have also merged with gtk3.
If you confirm that crash is gone, I'll merge with 1.2 and gtk3-1.0 since the current situation is already some kind of improvement with respect to before ...
Comment 17 Ruslan Kabatsayev 2012-03-09 15:30:24 UTC
(In reply to comment #16)
> 2/ would you double-check that after commit from #15 the crash is gone ? 
Yes, the basic crash is gone, i.e. when you start gtk-chtheme and immediately change theme
> if you switch away from oxygen and back to oxygen, then any update on the
> settings will crash, because of the unclear dangling connection ...
Strictly this way doesn't crash for me, but when I change some settings, and *then* change from oxygen to another style, it does crash. But not always. Sometimes something resembling your description also occurs, but all this is not regularly reproducible.
Comment 18 Hugo Pereira Da Costa 2012-03-12 07:39:41 UTC
ok. I'll merge to the release branches and will release today.