Bug 19528 - [GTK] BackForward history leak?
: [GTK] BackForward history leak?
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2008-06-12 18:21 PST by
Modified: 2009-03-11 05:06 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-06-12 18:21:06 PST
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 From 2008-06-15 03:00:50 PST -------
Alp,

Should we also look at other areas of webkit/gtk?
------- Comment #2 From 2008-08-06 07:07:10 PST -------
Created an attachment (id=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 From 2008-08-27 16:44:43 PST -------
(From update of attachment 22678 [details])
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 From 2009-03-10 11:21:12 PST -------
Created an attachment (id=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 From 2009-03-11 01:50:35 PST -------
(From update of attachment 28442 [details])
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 From 2009-03-11 05:06:21 PST -------
Landed in r41582. Thanks!