Bug 19528 - [GTK] BackForward history leak?
: [GTK] BackForward history leak?
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-12 18:21 PDT by Alp Toker
Modified: 2009-03-11 05:06 PDT (History)
1 user (show)

See Also:


Attachments
Refactor WebKitWebBackForwardList and leak fixes (20.47 KB, patch)
2008-08-06 07:07 PDT, Jan Alonzo
eric: review-
Details | Formatted Diff | Diff
Don't ref items when returning a back/forward list (6.80 KB, patch)
2009-03-10 11:21 PDT, Jan Alonzo
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2008-06-12 18:21:06 PDT
Got this report from a developer integrating WebKit into GIMP:

[16:27] <neo> webkit_web_view_get_back_forward_list() returns a pointer to an object without increasing the reference count on it

[16:28] <neo> webkit_web_back_forward_list_get_back_list_with_limit() on the other hand returns a newly allocated GList with referenced objects

[16:28] <neo> I find that very difficult to work with

[16:28] <neo> it should at least be documented

[16:29] <neo> I ended up leaking the objects in the list and I unreffed the back_forward_list

[16:29] <alp> neo: can you think of a nicer API for that?

[16:30] <neo> alp: well, be consistent and always ref the objects you hand out
Comment 1 Jan Alonzo 2008-06-15 03:00:50 PDT
Alp,

Should we also look at other areas of webkit/gtk?
Comment 2 Jan Alonzo 2008-08-06 07:07:10 PDT
Created attachment 22678 [details]
Refactor WebKitWebBackForwardList and leak fixes

This patch refactors WebKitWebBackForwardList as well as fixes possible leaks of WebKitWebHistoryItems. This also fixes the documentation wrt when to free and when not to free return values.
Comment 3 Eric Seidel 2008-08-27 16:44:43 PDT
Comment on attachment 22678 [details]
Refactor WebKitWebBackForwardList and leak fixes

I would encourage you to read:
http://webkit.org/coding/RefPtr.html

Methods shouldn't return RefPtr.  Possibly "const RefPtr&" , but in that case "Class*" is almost always better.

We also don't keep PassRefPtrs on the stack.  If you need more clarification about RefPtr design, I encourage you to read that document or ask in #webkit.  (or read some of hte other gtk bugs I've recently reviewed).
Comment 4 Jan Alonzo 2009-03-10 11:21:12 PDT
Created attachment 28442 [details]
Don't ref items when returning a back/forward list

Don't ref the returned items to be consistent with the rest of the API. Also a added a test case to verify this.
Comment 5 Holger Freyther 2009-03-11 01:50:35 PDT
Comment on attachment 28442 [details]
Don't ref items when returning a back/forward list

Okay, a change in behaviour but I assume the reality is that this just leaked in midori/epi? Thanks for adding a test. The only thing to change is probably the static void\nMETHOD_NAME as this is inconsistent with the rest of the file.
Comment 6 Jan Alonzo 2009-03-11 05:06:21 PDT
Landed in r41582. Thanks!