Bug 306671 - Timers use g_timeout_add instead of gdk_threads_add_timeout
Summary: Timers use g_timeout_add instead of gdk_threads_add_timeout
Status: RESOLVED FIXED
Alias: None
Product: Oxygen
Classification: Plasma
Component: gtk2-engine (show other bugs)
Version: unspecified
Platform: unspecified Other
: NOR critical
Target Milestone: ---
Assignee: Hugo Pereira Da Costa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-12 12:39 UTC by Yehouda Harpaz
Modified: 2013-01-16 12:24 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Suggested patch, to replace g_timeout_add by gdk_threads_add_timeout (1.65 KB, application/octet-stream)
2012-09-12 22:18 UTC, Hugo Pereira Da Costa
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yehouda Harpaz 2012-09-12 12:39:28 UTC
The timers in oxygen-gtk use g_timeout_add, but they need to use gdk_threads_add_timeout instead, because otherwise they end up calling gtk/gdk functions without a lock.

src/animations/oxygentimer.cpp Oxygen::Timer::start

It is also possible to fix this by looking around the call to the func inside Timer::timeOut, but using gdk_threads_add_timeout is the normal way.

In general, in code that uses GTK any usage of g_timeout_add, g_timeout_add_full, g_idle_add, and g_idle_add_full are suspicious. I search the oxygen-gtk code 1.3
 and there is another call to g_timeout_add in TimeLineServer::start with TimeLineServer::update. I think that needs fixing too.

Reproducible: Sometimes

Steps to Reproduce:
Because it is missing the lock, it randomly corrupts something, so it is not reproducible. 
We don't use Oxygen normally, but we have a client that has repeated crashes. 




A backtrace that the client sent us, showing a call to gtk_widget_queue_draw without a lock from MainWindowData::delayedUpdate, which is put in timer by MainWindowData::updateSize. 



 *** glibc detected *** /usr/lib/LispWorks/lispworks-6-1-0-x86-
linux: double free or corruption (fasttop): 0xb3003268 ***
======= Backtrace: =========
/lib/libc.so.6(+0x6de2b)[0xb7669e2b]
/lib/libc.so.6(+0x6ebab)[0xb766abab]
/lib/libc.so.6(cfree+0x6d)[0xb766ea6d]
/lib/libglib-2.0.so.0(g_free+0x36)[0xb752c5e6]
/usr/lib/libgdk-x11-2.0.so.0(+0x2bf0b)[0xb6fbdf0b]
/usr/lib/libgdk-x11-2.0.so.0(gdk_region_union+0x88)[0xb6fbfc98]
/usr/lib/libgdk-x11-2.0.so.0(+0x3766c)[0xb6fc966c]
/usr/lib/libgdk-x11-2.0.so.0(+0x3792a)[0xb6fc992a]
/usr/lib/libgdk-x11-2.0.so.0(+0x378af)[0xb6fc98af]
/usr/lib/libgdk-x11-2.0.so.0(+0x378af)[0xb6fc98af]
/usr/lib/libgdk-x11-2.0.so.0(+0x37a3e)[0xb6fc9a3e]
/usr/lib/libgdk-x11-2.0.so.0(+0x37afb)[0xb6fc9afb]
/usr/lib/libgtk-x11-2.0.so.0(gtk_widget_queue_draw_area+0x17a)[0xb72b3f5a]
/usr/lib/libgtk-x11-2.0.so.0(gtk_widget_queue_draw+0x96)[0xb72bc2c6]
/usr/lib/gtk-2.0/2.10.0/engines/liboxygen-
gtk.so(_ZN6Oxygen14MainWindowData13delayedUpdateEPv+0x38)[0xb4d7d4d8]
/usr/lib/gtk-2.0/2.10.0/engines/liboxygen-
gtk.so(_ZN6Oxygen5Timer7timeOutEPv+0x11)[0xb4d806d1]
/lib/libglib-2.0.so.0(+0x45e5f)[0xb7526e5f]
/lib/libglib-2.0.so.0(g_main_context_dispatch+0x1e9)[0xb7525509]
/lib/libglib-2.0.so.0(+0x44d10)[0xb7525d10]
/lib/libglib-2.0.so.0(g_main_loop_run+0x18f)[0xb75263ef]
Comment 1 Yehouda Harpaz 2012-09-12 12:40:31 UTC
Will affect gtk3-engine too
Comment 2 Hugo Pereira Da Costa 2012-09-12 12:49:22 UTC
Thanks (alot) for reporting this.
I was not aware of the existence of the gdk_thread_add_timeout method, nor of the limitations of g_timeout_add.
Early test after replacing the latter by the former causes no issue.
I'll push to master and gtk3 first, will test further for a couple of days, then backport to the stable branches

Hugo
Comment 3 Hugo Pereira Da Costa 2012-09-12 12:52:56 UTC
Git commit 850dfa7663d778e728afa1bd506b5f8cd7a3018f by Hugo Pereira Da Costa.
Committed on 12/09/2012 at 14:50.
Pushed by hpereiradacosta into branch 'master'.

Replaced g_timeout_add by thread-safe gdk_threads_add_timeout function.

M  +2    -1    demo/oxygentimer.cpp
M  +2    -1    src/animations/oxygentimelineserver.cpp
M  +2    -1    src/animations/oxygentimer.cpp

http://commits.kde.org/oxygen-gtk/850dfa7663d778e728afa1bd506b5f8cd7a3018f
Comment 4 Matias De lellis 2012-09-12 21:05:55 UTC
Hi people,
Comment on blog of Hugo, but this is the right place: Sorry. =)

Well. The dilemma is that gdk_thread will be deprecated after the upcoming release.

Here some info: https://mail.gnome.org/archives/gtk-devel-list/2012-July/msg00066.html
This does not have any replacement, but according http://developer.gnome.org/gtk-faq/stable/x499.html , if not initiate gdk_thread (Oxygen-gtk not start it), any function call from g_timeout/idle, is executed in the main thread, and therefore does not need to worry about the lock.

Therefore i suggest reverse the change. Probably the crash is due to something else. Could you upload a backtrace with full debug activated in oxygen-gtk?

Regards.

P.s. I'm any professional, but these are the conclusions we draw when updating Pragha.
Comment 5 Hugo Pereira Da Costa 2012-09-12 21:11:51 UTC
Thanks Mattias !
Indeed, I'll likely revert the change, and post a patch here instead, to be applied to both oxygen-gtk2 and gtk3, so that people can check whether it fixes the issue or not ... 

Will keep you both posted

Hugo
Comment 6 Hugo Pereira Da Costa 2012-09-12 22:13:29 UTC
Git commit ab1e6e634ce85130125d8589adfba75150cb2652 by Hugo Pereira Da Costa.
Committed on 13/09/2012 at 00:12.
Pushed by hpereiradacosta into branch 'gtk3'.

Revert "Replaced g_timeout_add by thread-safe gdk_threads_add_timeout function."
Reason: gdk_threads_add_timeout is being obsoleted
This reverts commit 850dfa7663d778e728afa1bd506b5f8cd7a3018f.

M  +1    -2    demo/oxygentimer.cpp
M  +1    -2    src/animations/oxygentimelineserver.cpp
M  +1    -2    src/animations/oxygentimer.cpp

http://commits.kde.org/oxygen-gtk/ab1e6e634ce85130125d8589adfba75150cb2652
Comment 7 Hugo Pereira Da Costa 2012-09-12 22:14:39 UTC
Ok. So revert done, as per comment #4 and #5
Attaching patch for reporter to test directly in next comment.
In case the patch does fix the problem, then we'll need to think about something else.
Comment 8 Hugo Pereira Da Costa 2012-09-12 22:18:25 UTC
Created attachment 73875 [details]
Suggested patch, to replace  g_timeout_add by gdk_threads_add_timeout

I verified that the patch applies both to the master (gtk2) and the gtk3 branch. 
Please give it a shot and report whether this does indeed fix the reported issue. 
Also, see comment #4 about posting here full BackTrace with debug symbols.

Thanks for the help,

Hugo
Comment 9 Yehouda Harpaz 2012-09-14 11:18:41 UTC
I have asked for clarification about the deprecation, and a "core developer" has answered here

 https://mail.gnome.org/archives/gtk-list/2012-September/msg00013.html

You should revert the reversion. 

opensuse themselves are patches version, we are going to try to test their patch.
Comment 10 Hugo Pereira Da Costa 2012-09-14 11:34:53 UTC
@Yehouda
thanks !

@Matias
(counter) comments ? 
If none, I'll re-revert.
Still, Yehouda, I'd rather the patch to be tested before it goes in the stable branches ...
Comment 11 Matias De lellis 2012-09-14 17:54:53 UTC
Uff.. A very interesting discussion in the list.

According ebassi the GDK threading API is available in GTK+ 2.x and 3.x.. will only be obsolete/romoved only in Gtk 4. When I wrote, was worried about it. So you can Re-Revert it.

Still I have my doubts if it is absolutely necessary use gdk_theads, but it will work.

mm.. Adding another question .. No need to start everything with gdk_threads_init ()
Comment 12 Hugo Pereira Da Costa 2012-09-14 18:05:41 UTC
@Matias, ok, will re-revert. Thanks to you two for the inputs
Comment 13 Hugo Pereira Da Costa 2012-09-17 09:19:58 UTC
Git commit ef9460a27f0508f6702f33d5ce055146041ade9c by Hugo Pereira Da Costa.
Committed on 12/09/2012 at 14:50.
Pushed by hpereiradacosta into branch '1.3'.

Replaced g_timeout_add by thread-safe gdk_threads_add_timeout function.

M  +2    -1    demo/oxygentimer.cpp
M  +2    -1    src/animations/oxygentimelineserver.cpp
M  +2    -1    src/animations/oxygentimer.cpp

http://commits.kde.org/oxygen-gtk/ef9460a27f0508f6702f33d5ce055146041ade9c
Comment 14 Hugo Pereira Da Costa 2012-09-17 09:19:59 UTC
Git commit 0c9b47affae3006bc6f759c47a50efe06e3a228b by Hugo Pereira Da Costa.
Committed on 12/09/2012 at 14:50.
Pushed by hpereiradacosta into branch 'gtk3-1.1'.

Replaced g_timeout_add by thread-safe gdk_threads_add_timeout function.

M  +2    -1    demo/oxygentimer.cpp
M  +2    -1    src/animations/oxygentimelineserver.cpp
M  +2    -1    src/animations/oxygentimer.cpp

http://commits.kde.org/oxygen-gtk/0c9b47affae3006bc6f759c47a50efe06e3a228b
Comment 15 Hugo Pereira Da Costa 2012-09-17 09:21:03 UTC
re-reverted, so change is in.
Also backported to 1.3 and gtk3-1.1
So closing ... 
@Yehouda
Feel free to re-open if problem still persists.
Comment 16 Hugo Pereira Da Costa 2013-01-16 12:24:08 UTC
Git commit 87e66e6c85ca2f19fc60cfebadb476e8e6425c4d by Hugo Pereira Da Costa.
Committed on 13/09/2012 at 00:12.
Pushed by hpereiradacosta into branch 'master'.

Revert "Replaced g_timeout_add by thread-safe gdk_threads_add_timeout function."
Reason: gdk_threads_add_timeout is being obsoleted
This reverts commit 850dfa7663d778e728afa1bd506b5f8cd7a3018f.

M  +1    -2    demo/oxygentimer.cpp
M  +1    -2    src/animations/oxygentimelineserver.cpp
M  +1    -2    src/animations/oxygentimer.cpp

http://commits.kde.org/oxygen-gtk/87e66e6c85ca2f19fc60cfebadb476e8e6425c4d