Summary: | [GTK] GtkSocket is leaked until webview is destroyed. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | arno. <a.renevier> | ||||||
Component: | Plug-ins | Assignee: | 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
arno.
2012-11-16 15:00:25 PST
Created attachment 174767 [details]
Patch
patch proposal
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 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 (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 (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 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. Created attachment 175549 [details]
Indeed. The updated patch checks if widget is null before going on
updated patch: check if widget is null
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 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 (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 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> All reviewed patches have been landed. Closing bug. |