[GTK] BackForward history leak?

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alp Toker]

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

[Jan Alonzo]

Alp,

Should we also look at other areas of webkit/gtk?

[Jan Alonzo]

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.

[Eric Seidel (no email)]

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).

[Jan Alonzo]

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.

[Holger Freyther]

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.

[Jan Alonzo]

Landed in r41582. Thanks!