Bug 129783

Summary: [GTK] Close the page when the view is disposed instead of when finalized
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, commit-queue, csaavedra, gustavo, mrobinson, svillar
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Updated patch including a unit test svillar: review+

Description Carlos Garcia Campos 2014-03-05 23:25:28 PST
When a GtkWidget is destroyed, the GObject is disposed, but not finalized if it has additional references. When the destroyed widget is leaked by the application, we leak the page proxy and what is more important the web process in multi-process mode.
Comment 1 Carlos Garcia Campos 2014-03-05 23:27:16 PST
Created attachment 225956 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-05 23:29:23 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Sergio Villar Senin 2014-03-06 01:06:59 PST
Comment on attachment 225956 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:391
> +    webView->priv->pageProxy->close();

Hmm, dispose() might be called multiple times, what would happen to the pageProxy if we call close() many times?.

I'd better add some condition there.
Comment 4 Carlos Garcia Campos 2014-03-06 01:08:53 PST
(In reply to comment #3)
> (From update of attachment 225956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225956&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:391
> > +    webView->priv->pageProxy->close();
> 
> Hmm, dispose() might be called multiple times, what would happen to the pageProxy if we call close() many times?.

Nothing, there's an early return in WebPageProxy:close();

> I'd better add some condition there.

It's not needed.
Comment 5 Carlos Garcia Campos 2014-03-06 02:10:00 PST
Created attachment 225971 [details]
Updated patch including a unit test

The test is disabled though, because it's affected by bug #129684
Comment 6 Sergio Villar Senin 2014-03-06 03:14:28 PST
Comment on attachment 225971 [details]
Updated patch including a unit test

Much better now.
Comment 7 Carlos Garcia Campos 2014-03-06 04:06:45 PST
Committed r165182: <http://trac.webkit.org/changeset/165182>