Bug 19939 - [GTK] webkit_web_history_item_get_title() fails with assertion
Summary: [GTK] webkit_web_history_item_get_title() fails with assertion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on: 19898
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-08 02:01 PDT by Sven Neumann
Modified: 2008-09-19 00:44 PDT (History)
2 users (show)

See Also:


Attachments
Fix history management (10.06 KB, patch)
2008-08-05 09:34 PDT, Alp Toker
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sven Neumann 2008-07-08 02:01:30 PDT
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
Comment 1 Sven Neumann 2008-07-22 09:09:15 PDT
Might be related to bug #19898, perhaps it is even fixed by the patch attached there.
Comment 2 Alp Toker 2008-08-05 09:34:20 PDT
Created attachment 22656 [details]
Fix history management

This patch fixes recently introduced regressions in WebHistory. Candidate for backporting to 1.0.1.
Comment 3 Jan Alonzo 2008-08-06 04:34:52 PDT
(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.
Comment 4 Alp Toker 2008-08-06 21:40:51 PDT
(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.
Comment 5 Jan Alonzo 2008-08-07 05:16:07 PDT
(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.
Comment 6 Eric Seidel (no email) 2008-08-27 16:36:07 PDT
Comment on attachment 22656 [details]
Fix history management

Looks sane enough.
Comment 7 Jan Alonzo 2008-09-01 05:45:24 PDT
landed in r36012
Comment 8 Sven Neumann 2008-09-19 00:44:37 PDT
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?