Bug 102564 – [GTK] GtkSocket is leaked until webview is destroyed.

• Component: Plug-ins
• Status: RESOLVED FIXED

[arno.]

Hi,
in gtk PluginView::start, a GtkSocket is added to the webview.
But is not removed when pluginview in destroyed. Therefore, it stays a child of the webview until the webview is destroyed.
So, if user change pages, the GtkSocket, and possibly all its child widgets appended by the plugins are not freed.

[arno.]

Created attachment 174767 [details]
Patch

patch proposal

[Martin Robinson]

Comment on attachment 174767 [details]
Patch

In the ~WigetGtk g_object_unref is called on the platform widget. Normally gtk_widget_destroy will remove a widget from its container. I'm not sure if g_object_unref does the same here. Can you confirm?

[WebKit Review Bot]

Comment on attachment 174767 [details]
Patch

Attachment 174767 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14859571

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[arno.]

(In reply to comment #2)
> (From update of attachment 174767 [details])
> In the ~WigetGtk g_object_unref is called on the platform widget. Normally gtk_widget_destroy will remove a widget from its container. I'm not sure if g_object_unref does the same here. Can you confirm?

Actually, WidgetGtk has g_object_ref the platform widget before. So, after ~WidgetGtk, the refcount will be 1, and the object will not be disposed.
I tried to just use g_object_unref in platformDestroy.
But then gtk_container_remove is called from the dispose callbacks, which itselfs calls g_object_unref. This is one too much and results in a gobject assertion.
So, I think, as pluginView has gtk_container_add(ed) the socket, it is reasonable to perform the opposite operation and use gtk_container_remove

[arno.]

(In reply to comment #3)
> (From update of attachment 174767 [details])
> Attachment 174767 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14859571
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

I don't really understand why this happens

[Martin Robinson]

Comment on attachment 174767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174767&action=review

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:889
>          XFreePixmap(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()), m_drawable);
>          m_drawable = 0;
>      }
> +    GtkWidget* widget = platformWidget();
> +    GtkWidget* parent = gtk_widget_get_parent(widget);
> +    ASSERT(parent);
> +    gtk_container_remove(GTK_CONTAINER(parent), widget);

Is platformDestroy also called for windowless plugins? Windowless plugins do not have platform widgets.

[arno.]

Created attachment 175549 [details]
Indeed. The updated patch checks if widget is null before going on

updated patch: check if widget is null

[WebKit Review Bot]

Comment on attachment 175549 [details]
Indeed. The updated patch checks if widget is null before going on

Attachment 175549 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14959162

New failing tests:
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html

[WebKit Review Bot]

Comment on attachment 175549 [details]
Indeed. The updated patch checks if widget is null before going on

Rejecting attachment 175549 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
fast/text/atsui-small-caps-punctuation-size.html
Full output: http://queues.webkit.org/results/14990703

[Martin Robinson]

(In reply to comment #9)
> (From update of attachment 175549 [details])
> Rejecting attachment 175549 [details] from commit-queue.
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
> fast/text/atsui-small-caps-punctuation-size.html
> Full output: http://queues.webkit.org/results/14990703

It's pretty sad that flaky tests pretty much break the commit queue. :(

[Martin Robinson]

Comment on attachment 175549 [details]
Indeed. The updated patch checks if widget is null before going on

Clearing flags on attachment: 175549

Committed r135762: <http://trac.webkit.org/changeset/135762>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.