102564 2012-11-16 15:00:25 -0800 [GTK] GtkSocket is leaked until webview is destroyed. 2012-11-26 14:01:45 -0800 1 1 1 Unclassified WebKit Plug-ins 525.x (Safari 3.2) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 a.renevier a.renevier dglazkov d-r mrobinson naginenis webkit.review.bot oldest_to_newest 769694 0 a.renevier 2012-11-16 15:00:25 -0800 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. 769705 1 174767 a.renevier 2012-11-16 15:12:22 -0800 Created attachment 174767 Patch patch proposal 769711 2 174767 mrobinson 2012-11-16 15:21:13 -0800 Comment on attachment 174767 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? 769804 3 174767 webkit.review.bot 2012-11-16 17:38:35 -0800 Comment on attachment 174767 Patch Attachment 174767 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 769805 4 a.renevier 2012-11-16 17:46:11 -0800 (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 769806 5 a.renevier 2012-11-16 17:47:08 -0800 (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 771081 6 174767 mrobinson 2012-11-19 15:09:53 -0800 Comment on attachment 174767 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. 773433 7 175549 a.renevier 2012-11-21 17:12:49 -0800 Created attachment 175549 Indeed. The updated patch checks if widget is null before going on updated patch: check if widget is null 773489 8 175549 webkit.review.bot 2012-11-21 19:07:51 -0800 Comment on attachment 175549 Indeed. The updated patch checks if widget is null before going on Attachment 175549 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 775581 9 175549 webkit.review.bot 2012-11-26 09:52:23 -0800 Comment on attachment 175549 Indeed. The updated patch checks if widget is null before going on Rejecting attachment 175549 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 775643 10 mrobinson 2012-11-26 10:44:32 -0800 (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. :( 775864 11 175549 mrobinson 2012-11-26 14:01:41 -0800 Comment on attachment 175549 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> 775865 12 mrobinson 2012-11-26 14:01:45 -0800 All reviewed patches have been landed. Closing bug. 174767 2012-11-16 15:12:22 -0800 2012-11-21 17:12:46 -0800 Patch bug-102564-20121116151015.patch text/plain 1609 a.renevier U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0MTA1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2MxZjI0OWExYjIwMDUz NmEwZDI4OTgzZTJlNTRlYTZhNTE1YzU2YS4uYWQyMTM5NjYxYzJhMTU0MjY3ZTQ4NDk0N2E1YmMw YTEyZDVmYTg2ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTExLTE2ICBBcm5h dWQgUmVuZXZpZXIgIDxhLnJlbmV2aWVyQHNpc2Euc2Ftc3VuZy5jb20+CisKKyAgICAgICAgW0dU S10gR3RrU29ja2V0IGlzIGxlYWtlZCB1bnRpbCB3ZWJ2aWV3IGlzIGRlc3Ryb3llZC4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMjU2NAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbW92ZSBHdGtTb2Nr ZXQgZnJvbSBpdHMgcGFyZW50IHdoZW4gcGx1Z2ludmlldyBpcyBkZXN0cm95ZWQuIFRoZW4sCisg ICAgICAgIHRoZSBHdGtTb2NrZXQgYW5kIGl0J3MgcG9zc2libGUgY2hpbGQgd2lkZ2V0cyBhcmUg cmVhbGVhc2VkIHdoZW4gaXQgaXMKKyAgICAgICAgbm8gbW9yZSBuZWVkZWQuCisKKyAgICAgICAg Tm8gbmV3IHRlc3RzLCBhbHJlYWR5IGNvdmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisKKyAgICAg ICAgKiBwbHVnaW5zL2d0ay9QbHVnaW5WaWV3R3RrLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBs dWdpblZpZXc6OnBsYXRmb3JtRGVzdHJveSk6CisKIDIwMTItMTEtMDkgIFNoZXJpZmYgQm90ICA8 d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxp bmcgb3V0IHIxMzQxMDEuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbHVnaW5zL2d0ay9Q bHVnaW5WaWV3R3RrLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdH dGsuY3BwCmluZGV4IDQwMDYwNDBkOGNkNjFhYmYwNDBlOTc3MDljOTdmZGE1NWJhMWM5YzUuLmFi YzNjNDY1ODFiMTAxNGI2ZjljODVkYzFkMDlhNTQ4NmI1ODRhNGEgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJD b3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwCkBAIC04ODMsNiArODgzLDEwIEBAIHZv aWQgUGx1Z2luVmlldzo6cGxhdGZvcm1EZXN0cm95KCkKICAgICAgICAgWEZyZWVQaXhtYXAoR0RL X0RJU1BMQVlfWERJU1BMQVkoZ2RrX2Rpc3BsYXlfZ2V0X2RlZmF1bHQoKSksIG1fZHJhd2FibGUp OwogICAgICAgICBtX2RyYXdhYmxlID0gMDsKICAgICB9CisgICAgR3RrV2lkZ2V0KiB3aWRnZXQg PSBwbGF0Zm9ybVdpZGdldCgpOworICAgIEd0a1dpZGdldCogcGFyZW50ID0gZ3RrX3dpZGdldF9n ZXRfcGFyZW50KHdpZGdldCk7CisgICAgQVNTRVJUKHBhcmVudCk7CisgICAgZ3RrX2NvbnRhaW5l cl9yZW1vdmUoR1RLX0NPTlRBSU5FUihwYXJlbnQpLCB3aWRnZXQpOwogfQogCiB9IC8vIG5hbWVz cGFjZSBXZWJDb3JlCg== 175549 2012-11-21 17:12:49 -0800 2012-11-26 14:01:41 -0800 Indeed. The updated patch checks if widget is null before going on bug-102564-20121121171033.patch text/plain 1649 a.renevier U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0MTA1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2MxZjI0OWExYjIwMDUz NmEwZDI4OTgzZTJlNTRlYTZhNTE1YzU2YS4uYWQyMTM5NjYxYzJhMTU0MjY3ZTQ4NDk0N2E1YmMw YTEyZDVmYTg2ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTExLTE2ICBBcm5h dWQgUmVuZXZpZXIgIDxhLnJlbmV2aWVyQHNpc2Euc2Ftc3VuZy5jb20+CisKKyAgICAgICAgW0dU S10gR3RrU29ja2V0IGlzIGxlYWtlZCB1bnRpbCB3ZWJ2aWV3IGlzIGRlc3Ryb3llZC4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMjU2NAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbW92ZSBHdGtTb2Nr ZXQgZnJvbSBpdHMgcGFyZW50IHdoZW4gcGx1Z2ludmlldyBpcyBkZXN0cm95ZWQuIFRoZW4sCisg ICAgICAgIHRoZSBHdGtTb2NrZXQgYW5kIGl0J3MgcG9zc2libGUgY2hpbGQgd2lkZ2V0cyBhcmUg cmVhbGVhc2VkIHdoZW4gaXQgaXMKKyAgICAgICAgbm8gbW9yZSBuZWVkZWQuCisKKyAgICAgICAg Tm8gbmV3IHRlc3RzLCBhbHJlYWR5IGNvdmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisKKyAgICAg ICAgKiBwbHVnaW5zL2d0ay9QbHVnaW5WaWV3R3RrLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBs dWdpblZpZXc6OnBsYXRmb3JtRGVzdHJveSk6CisKIDIwMTItMTEtMDkgIFNoZXJpZmYgQm90ICA8 d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxp bmcgb3V0IHIxMzQxMDEuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbHVnaW5zL2d0ay9Q bHVnaW5WaWV3R3RrLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdH dGsuY3BwCmluZGV4IDQwMDYwNDBkOGNkNjFhYmYwNDBlOTc3MDljOTdmZGE1NWJhMWM5YzUuLjJl NjdiYzY3ZWM3N2M4MzIyODVkNTYwOTYzNTc2ZDczNmJlYThlZGMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJD b3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwCkBAIC04ODMsNiArODgzLDEzIEBAIHZv aWQgUGx1Z2luVmlldzo6cGxhdGZvcm1EZXN0cm95KCkKICAgICAgICAgWEZyZWVQaXhtYXAoR0RL X0RJU1BMQVlfWERJU1BMQVkoZ2RrX2Rpc3BsYXlfZ2V0X2RlZmF1bHQoKSksIG1fZHJhd2FibGUp OwogICAgICAgICBtX2RyYXdhYmxlID0gMDsKICAgICB9CisKKyAgICBHdGtXaWRnZXQqIHdpZGdl dCA9IHBsYXRmb3JtV2lkZ2V0KCk7CisgICAgaWYgKHdpZGdldCkgeworICAgICAgICBHdGtXaWRn ZXQqIHBhcmVudCA9IGd0a193aWRnZXRfZ2V0X3BhcmVudCh3aWRnZXQpOworICAgICAgICBBU1NF UlQocGFyZW50KTsKKyAgICAgICAgZ3RrX2NvbnRhaW5lcl9yZW1vdmUoR1RLX0NPTlRBSU5FUihw YXJlbnQpLCB3aWRnZXQpOworICAgIH0KIH0KIAogfSAvLyBuYW1lc3BhY2UgV2ViQ29yZQo=