Created attachment 281738 [details] [review]
W32: Don't unregister parent window of an embedded widget

This leads to an assertion failure, because parent window is never registered
in the first place, widget's own GdkWindow is. But that window is unregistered
in a generic fashion by GtkWidget code, so there's nothing for us to do here.

For reference: this code came from https://git.gnome.org/browse/gtk+/commit/gtk/gtkwin32embedwidget.c?id=3d4cd4db3ed9f6c47566a69d745441e2662ac2ab as a fix for bug 687842

Review of attachment 281738 [details] [review]:

This is weird, is there a possiblitiy whether could reach here with a registered window?

I don't know. I just observed that there's only one call to gtk_widget_register_window() in gtkwin32embedwidget.c, and that call registers the widget window itself, not its parent. Which means that unregistering parent later on makes no sense.

Alex probably knows for sure, since it's his code.

I've looked at how other widgets use this API. Here's what i think:

static void
gtk_revealer_real_realize (GtkWidget *widget)
{
...

  /* Create GdkWindow */
  priv->view_window =
    gdk_window_new (gtk_widget_get_parent_window ((GtkWidget*) revealer),
                    &attributes, attributes_mask);
  /* Make it THE window of the widget */
  gtk_widget_set_window (widget, priv->view_window);
  /* Register it */
  gtk_widget_register_window (widget, priv->view_window);

...

  /* Create another GdkWindow */
  priv->bin_window =
    gdk_window_new (priv->view_window, &attributes, attributes_mask);
  /* Register it */
  gtk_widget_register_window (widget, priv->bin_window);

...
}

static void
gtk_revealer_real_unrealize (GtkWidget *widget)
{
  GtkRevealer *revealer = GTK_REVEALER (widget);
  GtkRevealerPrivate *priv = gtk_revealer_get_instance_private (revealer);

  /* unregister the bin window*/
  gtk_widget_unregister_window (widget, priv->bin_window);
  gdk_window_destroy (priv->bin_window);
  priv->view_window = NULL;

  /* Presumably, widget's own window (view_window) is unregistered
   * somewhere down here. That is, i know that it's unregistered somewhere
   * in GtkWidget (i've seen it in gdb at a breakpoint), but i don't remember
   * how that place is reached. But it's likely through this call.
   */
  GTK_WIDGET_CLASS (gtk_revealer_parent_class)->unrealize (widget);
}


That is, widget's own GdkWindow needs to be registered, but does not require deregistration.
Extra GdkWindows of the widget also need to be registered, but, unlike the main window, they don't have generic unreg code, and thus need to be unregistered manually.

Review of attachment 281738 [details] [review]:

the patch looks correct to me: for sure the window that is unregistering is not the same that it is registering, so that definitely looks like a bug.

beside if you look for instance at gtkoffscreenwindow.c, it does the same thing and does not unregister

while changing those line you can clean things up using g_clear_object instead of the whole if block

Created attachment 282310 [details] [review]
Use g_clear_object() shorthand for unref-and-set-to-null

Review of attachment 282310 [details] [review]:

let's get this in. in any case it is better than crashing

Attachment 281738 [details] pushed as e1acb93 - W32: Don't unregister parent window of an embedded widget
Attachment 282310 [details] pushed as bbe475f - Use g_clear_object() shorthand for unref-and-set-to-null