Summary: | [GTK] WebView should hold a reference on WebContext because non-default contexts are a reality | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Debarshi Ray <rishi.is> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | berto, buildbot, cgarcia, commit-queue, gustavo, mcatanzaro, mrobinson, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Debarshi Ray
2015-03-03 10:38:16 PST
Created attachment 247772 [details]
Patch
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 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. 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. :) 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 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
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? (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. (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. :) Created attachment 247851 [details]
Patch
Created attachment 247854 [details]
Patch
Comment on attachment 247854 [details]
Patch
Thanks!
Comment on attachment 247854 [details] Patch Clearing flags on attachment: 247854 Committed r180998: <http://trac.webkit.org/changeset/180998> All reviewed patches have been landed. Closing bug. |