[GTK] WebView should hold a reference on WebContext because non-default contexts are a reality

[Debarshi Ray]

Now that we can create our own non-default WebKitWebContexts (see bug 138826), it is possible that the reference on the context is dropped before the WebKitWebView that is using it is destroyed. eg., in gnome-online-accounts, we have a custom composite widget that contains a context and a view. During destruction of the widget hierarchy, the reference on the context gets dropped in the widget's dispose before the widget's parent actually destroys the widget. This leads to a crash.

WebKitWebViews should therefore hold a reference to the WebKitWebContext to avoid this.

Patch will follow.

[Debarshi Ray]

Created attachment 247772 [details]
Patch

[WebKit Commit Bot]

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

[Martin Robinson]

Comment on attachment 247772 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:70
> +    GRefPtr<WebKitWebContext> webContext = webkit_web_context_new();
> +    GRefPtr<WebKitWebView> webView = WEBKIT_WEB_VIEW(webkit_web_view_new_with_context(webContext.get()));

I think you might be leaking webContext here, because you didn't use adoptGRef.

[Michael Catanzaro]

Thank you!

It's not really clear from the test how it works, so I would add a comment at least. I guess the test crashes without your patch applied because the web view tries to use the context in dispose. So the test relies on an implementation detail, which is OK, but not great, especially since it is easy to test without relying on that. It also would be more obvious how the test works if you actually get the context from the web view after you've unreffed your copy of the context, and try to do something with the context.

Also not a bad idea to use assertObjectIsDeletedWhenTestFinishes() in a lifetime test, to test for a potential future leak. (Or an actual present tense leak, as Martin has pointed out. :)

[Build Bot]

Comment on attachment 247772 [details]
Patch

Attachment 247772 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5110226772557824

New failing tests:
fast/css/object-fit/object-fit-canvas.html

[Build Bot]

Created attachment 247777 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Carlos Garcia Campos]

Comment on attachment 247772 [details]
Patch

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

r- only because of the unit tests leaking.

> Source/WebKit2/ChangeLog:3
> +        WebView should hold a reference on WebContext because non-default contexts are a reality

Please use the bug title here, prepare-ChangeLog should do that automatically.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:70
>> +    GRefPtr<WebKitWebView> webView = WEBKIT_WEB_VIEW(webkit_web_view_new_with_context(webContext.get()));
> 
> I think you might be leaking webContext here, because you didn't use adoptGRef.

Yes, I think in this particular case that we are going to explicitly unref the object, the test is more obvious if it doesn't use "smart" pointers. Even more in the case of the WebView that is a widget and it's normally deleted with gtk_widget_destroy(). We should also use assertObjectIsDeletedWhenTestFinishes() with both the context and the web view to make sure we don't leak any of those.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:77
> +    webContext = webkit_web_context_new();
> +    webView = WEBKIT_WEB_VIEW(webkit_web_view_new_with_context(webContext.get()));
> +    webView = nullptr;
> +    webContext = nullptr;

What's the point of doing it twice?

[Debarshi Ray]

(In reply to comment #7)

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:77
> > +    webContext = webkit_web_context_new();
> > +    webView = WEBKIT_WEB_VIEW(webkit_web_view_new_with_context(webContext.get()));
> > +    webView = nullptr;
> > +    webContext = nullptr;
> 
> What's the point of doing it twice?

The idea was to make sure that nothing untoward happens if the context is destroyed before with view and the other way round.

[Debarshi Ray]

(In reply to comment #7)
> Comment on attachment 247772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247772&action=review
> 
> r- only because of the unit tests leaking.
> 
> > Source/WebKit2/ChangeLog:3
> > +        WebView should hold a reference on WebContext because non-default contexts are a reality
> 
> Please use the bug title here, prepare-ChangeLog should do that
> automatically.

I added the [GTK] prefix to the bug title afterwards, which might have confused the scripts. Let me see if I can convince the scripts now. :)

[Debarshi Ray]

Created attachment 247851 [details]
Patch

[Debarshi Ray]

Created attachment 247854 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 247854 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 247854 [details]
Patch

Clearing flags on attachment: 247854

Committed r180998: <http://trac.webkit.org/changeset/180998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.