GNOME Bugzilla – Bug 711552
Crash when processing list of GdkWindows
Last modified: 2013-12-18 17:32:47 UTC
Created attachment 259084 [details] reproducer When processing "update_windows" in GdkWindow.c which was not correctly cleared, it can happen that some windows in the list were already destroyed. E.g. if there are 2 consecutive gtk_main() called in one process then the second one can crash because of this. Correct reference count handling of added windows prevents such applications from crash. Better clearing of the list in _gdk_window_clear_update_area() helps too. Attached is my reproducer which is based on Selections test from lsb-gtkvts from lsb-test-desktop (compile it with gtk+-2.0). Marek
Created attachment 259086 [details] [review] Handle references in "update_windows" list correctly
Created attachment 259087 [details] [review] clear the list in _gdk_window_clear_update_area()
Review of attachment 259086 [details] [review]: ::: gdk/gdkwindow.c @@ +5342,3 @@ gdk_window_remove_update_window (GdkWindow *window) { + update_windows = slist_remove_full (update_windows, window, (GDestroyNotify) g_object_unref); Can just do update_windows = g_slist_remove (update_windows, window); g_object_unref (window); here
Review of attachment 259087 [details] [review]: The patch doesn't apply to git master, it seems. ::: gdk/gdkwindow.c @@ +6220,3 @@ if (private->update_area) { + gdk_window_remove_update_window (window, TRUE); I would probably just open-code the g_slist_remove_all call here, or add a separate gdk_window_remove_update_window_completely function, instead of a boolean parameter.
Created attachment 264073 [details] [review] Handle references in "update_windows" list correctly (In reply to comment #3) > Review of attachment 259086 [details] [review]: > > ::: gdk/gdkwindow.c > @@ +5342,3 @@ > gdk_window_remove_update_window (GdkWindow *window) > { > + update_windows = slist_remove_full (update_windows, window, (GDestroyNotify) > g_object_unref); > > Can just do > > update_windows = g_slist_remove (update_windows, window); > g_object_unref (window); > > here I did it this way to be sure to unref the window only if the window was present in the list. I've changed it in the attached patch so that we find the window in the list and unref it then (but it needs 2 iterations over the list instead of 1 now). I targeted those patches mainly to gtk+-2.24 but we can get them also to master and 3.10 to be sure that there will be no problems in the future (referencing those windows is always good thing to do I guess).
Created attachment 264074 [details] [review] emove GdkWindow with cleared update area from update list (In reply to comment #4) > Review of attachment 259087 [details] [review]: > > The patch doesn't apply to git master, it seems. > > ::: gdk/gdkwindow.c > @@ +6220,3 @@ > if (private->update_area) > { > + gdk_window_remove_update_window (window, TRUE); > > I would probably just open-code the g_slist_remove_all call here, or add a > separate gdk_window_remove_update_window_completely function, instead of a > boolean parameter. I've added the gdk_window_remove_update_window_completely() function. I had to add there unreferencing of removed window to it (through g_slist_foreach()).
Review of attachment 264073 [details] [review]: ::: gdk/gdkwindow.c @@ +3303,3 @@ + { + update_windows = g_slist_delete_link (update_windows, link); + g_object_unref (window); I think this is fine - the list should be very short, so the double iteration shouldn't matter too much
Review of attachment 264074 [details] [review]: ok
Review of attachment 264073 [details] [review]: .
patches look ok to me now, but lets ask alex for a review. this is his area
Review of attachment 264073 [details] [review]: This looks good to me.
Review of attachment 264074 [details] [review]: I don't understand this one though. How can we ever get the same GdkWindow twice in updates_windows?
(In reply to comment #12) > Review of attachment 264074 [details] [review]: > > I don't understand this one though. How can we ever get the same GdkWindow > twice in updates_windows? Hi Alex, you can find my findings about this here: https://bugzilla.redhat.com/show_bug.cgi?id=991365#c3 or here: https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3866#c1 Sorry for not mentioning this earlier. Marek
I should also mention that my findings are based on gtk+-2.0.
Comment on attachment 264073 [details] [review] Handle references in "update_windows" list correctly Thank you for the review. I've pushed this one to master and 2-24.
Review of attachment 264074 [details] [review]: Rather than having a specialized remove operation in some cases (not very clear when this should be called) I think it makes more sense to have gdk_window_add_update_window() catch the case where the window is already in the list. This function already iterates over update_windows, so that should not make anything less efficient.
Also, the code should have a comment pointing out why this can happen.
Created attachment 264422 [details] [review] Don't add the same window to "update_windows" twice (In reply to comment #16) > Review of attachment 264074 [details] [review]: > > Rather than having a specialized remove operation in some cases (not very clear > when this should be called) I think it makes more sense to have > gdk_window_add_update_window() catch the case where the window is already in > the list. This function already iterates over update_windows, so that should > not make anything less efficient. Hi, I've modified the patch according to your comment. It fixes the problem too. Btw, could we move the line "GdkWindow *parent = window->parent;" from the cycle? It seems that none of the variables is changed after that. Regards Marek
Review of attachment 264422 [details] [review]: Otherwise looks good to me. ::: gdk/gdkwindow.c @@ +3234,3 @@ + /* Check whether "window" is already in "update_windows" list. + * It could be added during execution of gtk_widget_destroy() when + * setting focus widget to NULL and redrawing old focus widget. Add: "See bug 711552"
We could move the parent assignment up, but I doubt it makes any difference, the compiler knows its not modified.
Created attachment 264470 [details] [review] Don't add the same window to "update_windows" twice Hi, I had to change the patch yet. There is no guarantee that the cycle gets to the same GdkWindow before it adds it again. The safest thing is to check it before the cycle (but it can happen that the list will be traversed twice :-( ). Marek
Review of attachment 264470 [details] [review]: Otherwise looks good. I don't think there is a performance issue here anyway, because this is generally a short list of toplevel windows. ::: gdk/gdkwindow.c @@ +3234,3 @@ + */ + tmp = g_slist_find (update_windows, window); + if (tmp != NULL && window == tmp->data) You don't have to check tmp->data, g_slist_find() will return NULL unless it finds the window.
Comment on attachment 264470 [details] [review] Don't add the same window to "update_windows" twice Hi, thank you for the review. (In reply to comment #22) > Review of attachment 264470 [details] [review]: > > Otherwise looks good. I don't think there is a performance issue here anyway, > because this is generally a short list of toplevel windows. > > ::: gdk/gdkwindow.c > @@ +3234,3 @@ > + */ > + tmp = g_slist_find (update_windows, window); > + if (tmp != NULL && window == tmp->data) > > You don't have to check tmp->data, g_slist_find() will return NULL unless it > finds the window. I'm not sure what I was thinking about when I was adding it there. Thank you for catching it. I've pushed the patch to master and gtk-2-24. Thank you all for your help Marek