Bug 19939 – [GTK] webkit_web_history_item_get_title() fails with assertion

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sven Neumann]

This happens in the GIMP help-browser with webkit release 1.0.1. This code used to work correctly with webkit svn32442, so this looks like a regression in your code.

When calling webkit_web_history_item_get_title() on an item from the list obtained with webkit_web_back_forward_list_get_back_list_with_limit() a NULL title is returned and there's an assertion in the webkit code:

** CRITICAL **: const gchar* webkit_web_history_item_get_title(WebKitWebHistoryItem*): assertion `item != NULL' failed

[Sven Neumann]

Might be related to bug #19898, perhaps it is even fixed by the patch attached there.

[Alp Toker]

Created attachment 22656 [details]
Fix history management

This patch fixes recently introduced regressions in WebHistory. Candidate for backporting to 1.0.1.

[Jan Alonzo]

(In reply to comment #2)
> Created an attachment (id=22656) [edit]
> Fix history management
> 
> This patch fixes recently introduced regressions in WebHistory. Candidate for
> backporting to 1.0.1.
> 

@@ -257,9 +255,14 @@ WebKitWebHistoryItem* webkit_web_history
 {
     WebKitWebHistoryItem* webHistoryItem = kit(item);
 
-    if (!webHistoryItem) {
+    if (webHistoryItem)
+        g_object_ref(webHistoryItem);
+    else {

I don't think increasing the ref count is necessary here. The backforwardlist already does that when it uses this function and there's no way to unref the webHistoryItem when you do it here.

Looks good otherwise.

[Alp Toker]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=22656) [edit]
> > Fix history management
> > 
> > This patch fixes recently introduced regressions in WebHistory. Candidate for
> > backporting to 1.0.1.
> > 
> 
> @@ -257,9 +255,14 @@ WebKitWebHistoryItem* webkit_web_history
>  {
>      WebKitWebHistoryItem* webHistoryItem = kit(item);
> 
> -    if (!webHistoryItem) {
> +    if (webHistoryItem)
> +        g_object_ref(webHistoryItem);
> +    else {
> 
> I don't think increasing the ref count is necessary here. The backforwardlist
> already does that when it uses this function and there's no way to unref the
> webHistoryItem when you do it here.
> 
> Looks good otherwise.
> 

I did this simply to balance object reference counts, since the other code path creates a new instance:

 {
     WebKitWebHistoryItem* webHistoryItem = kit(item);
 
-    if (!webHistoryItem) {
+    if (webHistoryItem)
+        g_object_ref(webHistoryItem);
+    else {
         webHistoryItem = WEBKIT_WEB_HISTORY_ITEM(g_object_new(WEBKIT_TYPE_WEB_HISTORY_ITEM, NULL));
-        webkit_history_item_add(webHistoryItem, core(webHistoryItem));
+        WebKitWebHistoryItemPrivate* priv = webHistoryItem->priv;
+
+        priv->historyItem = item;
+        webkit_history_item_add(webHistoryItem, priv->historyItem.get());
     }

This probably doesn't solve memory management issues here but at least it's consistent this way.

Bug #19898 might solve memory issues more fully but I wanted to get the crasher fix in soonish. I'm happy to get this one in with or without the ref change to get things usable.

[Jan Alonzo]

(In reply to comment #4)

<snip>

> This probably doesn't solve memory management issues here but at least it's
> consistent this way.
> 
> Bug #19898 might solve memory issues more fully but I wanted to get the crasher
> fix in soonish. I'm happy to get this one in with or without the ref change to
> get things usable.
> 

That's cool. It's better to commit it without the ref change.

[Eric Seidel (no email)]

Comment on attachment 22656 [details]
Fix history management

Looks sane enough.

[Jan Alonzo]

landed in r36012

[Sven Neumann]

It would totally rock if there was a release with this fix. Any plans for an 1.0.2 release? Or did I just miss it and it already happened?