Bug 102564

Summary: [GTK] GtkSocket is leaked until webview is destroyed.
Product: WebKit Reporter: arno. <a.renevier>
Component: Plug-insAssignee: arno. <a.renevier>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, d-r, mrobinson, naginenis, webkit.review.bot
Priority: P2    
Version: 525.x (Safari 3.2)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Indeed. The updated patch checks if widget is null before going on none

Description arno. 2012-11-16 15:00:25 PST
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.
Comment 1 arno. 2012-11-16 15:12:22 PST
Created attachment 174767 [details]
Patch

patch proposal
Comment 2 Martin Robinson 2012-11-16 15:21:13 PST
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?
Comment 3 WebKit Review Bot 2012-11-16 17:38:35 PST
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
Comment 4 arno. 2012-11-16 17:46:11 PST
(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
Comment 5 arno. 2012-11-16 17:47:08 PST
(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
Comment 6 Martin Robinson 2012-11-19 15:09:53 PST
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.
Comment 7 arno. 2012-11-21 17:12:49 PST
Created attachment 175549 [details]
Indeed. The updated patch checks if widget is null before going on

updated patch: check if widget is null
Comment 8 WebKit Review Bot 2012-11-21 19:07:51 PST
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
Comment 9 WebKit Review Bot 2012-11-26 09:52:23 PST
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
Comment 10 Martin Robinson 2012-11-26 10:44:32 PST
(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. :(
Comment 11 Martin Robinson 2012-11-26 14:01:41 PST
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>
Comment 12 Martin Robinson 2012-11-26 14:01:45 PST
All reviewed patches have been landed.  Closing bug.