Bug 19898 – [Gtk] WebKitWebHistoryItem: Fixes and improvements

• Component: WebKitGTK
• Status: RESOLVED DUPLICATE

[Jan Alonzo]

Documentation and handling of RefPtr should be fixed. We're currently using the raw HistoryItem pointer which results to implicit conversions, hence messing up ref counting. There are also instances where Epiphany crashes when using the backforwardlist (due to WebKitWebHistoryItem's fault). And lastly, title and URI should be empty strings when initialised or assigned as NULL (rather than NULL).

[Jan Alonzo]

Created attachment 22091 [details]
Patch to fix issues in WebKitWebHistoryItem

The attached patch fixes the issues I mentioned above. Feedback appreciated.

[Xan Lopez]

Looks pretty good to me.

[Jan Alonzo]

Created attachment 22254 [details]
updated documentation

This update improves the documentation on the previous patch, specifically documented when a return value should be freed or not.

[Jan Alonzo]

Comment on attachment 22254 [details]
updated documentation

This patch needs a bit more work.

[Jan Alonzo]

Comment on attachment 22254 [details]
updated documentation

wrong review bit. should be none.

[Sven Neumann]

This looks like it might also fix bug #19939.

[Sven Neumann]

There are some issues with the documentation changes in this patch though. The patch introduces documentation like this:

 * Return value: the title of the @webHistoryItem or %NULL if history item
 *               is %NULL. The string is owned by @webHistoryItem and must not
 *               be modified or freed.

It is not allowed to pass NULL for @webHistoryItem though. The changed documentation of the return value gives the impression that this would be allowed, but it isn't. There's a g_return_val_if_fail() check for this. It is correct that this assertion returns NULL. But by definition the behavior is undefined after such an assertion failed. It will only confuse users of the API to document this error case.

I very much welcome though that the patch makes it clearer who is responsible for freeing the returned strings.

[Jan Alonzo]

Created attachment 22492 [details]
updated patch

[Christian Dywan]

Comment on attachment 22492 [details]
updated patch

>-static void webkit_history_item_add(WebKitWebHistoryItem* webHistoryItem, WebCore::HistoryItem* historyItem)
>+static void webkit_history_item_add(WebKitWebHistoryItem* webHistoryItem, RefPtr<HistoryItem> historyItem)
> {
>     g_return_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem));
>+    g_return_if_fail(historyItem != NULL);
> 
>     GHashTable* table = webkit_history_items();

You shouldn't use g_return_if_fail in static functions. That macro is meant to warn about errorneous use of the library. Use ASSERT instead.

>-/* Helper function to create a new WebHistoryItem instance when needed */
>-WebKitWebHistoryItem* webkit_web_history_item_new_with_core_item(WebCore::HistoryItem* item)
>+/* Helper function to create a new WebHistoryItem instance if needed */
>+WebKitWebHistoryItem* webkit_web_history_item_get_item(RefPtr<HistoryItem> item)
> {
>+    g_return_val_if_fail(item != NULL, NULL);
>+
>     WebKitWebHistoryItem* webHistoryItem = kit(item);
> 
>     if (!webHistoryItem) {

Dito.

> WebKitWebHistoryItem* webkit_web_history_item_new_with_data(const gchar* uri, const gchar* title)
> {
>-    WebCore::KURL historyUri(uri);
>-    WebCore::String historyTitle(title);
>+    if (uri == NULL)
>+        uri = "";
>+
>+    if (title == NULL)
>+        title = "";
>+
>+    KURL historyUri(uri);
>+    String historyTitle(title);

I would have written that title ? title: "" so it's shorter. But I suppose that's a matter of personal preference. :)

>@@ -312,18 +322,23 @@ WebKitWebHistoryItem* webkit_web_history_item_new_with_data(const gchar* uri, co
>  * @webHistoryItem: a #WebKitWebHistoryItem
>  *
>  * Returns the page title of @webHistoryItem
>+ *
>+ * Return value: the title of the @webHistoryItem or %NULL if history item
>+ *               is %NULL. The string is owned by @webHistoryItem and must not
>+ *               be modified or freed.
>  */

As mentioned before, the wording suggests a NULL webHistoryItem were allowed which it isn't. NULL is just the _fail return value and not an expected value.

Also, while you are at fixing documentation, "webHistoryItem" is an internal identifier. The API argument is called "web_history_item". I'd appreciate if you could fix that where needed.

>     return priv->alternateTitle;
>@@ -358,15 +376,22 @@ G_CONST_RETURN gchar* webkit_web_history_item_get_alternate_title(WebKitWebHisto
>  * @webHistoryItem: a #WebKitWebHistoryItem
>  * @title: the alternate title for @this history item
>  *
>- * Sets an alternate title for @webHistoryItem
>+ * Sets an alternate title for @webHistoryItem. If @title is NULL, this
>+ * function will set the title to an empty string.
>  */
> void webkit_web_history_item_set_alternate_title(WebKitWebHistoryItem* webHistoryItem, const gchar* title)
> {
>     g_return_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem));
> 
>-    WebCore::HistoryItem* item = core(webHistoryItem);
>+    RefPtr<HistoryItem> item = core(webHistoryItem);
>+
>+    if (!item)
>+        return;

What was the intention here? Silently failing is definitely wrong here.

>  */
> G_CONST_RETURN gchar* webkit_web_history_item_get_uri(WebKitWebHistoryItem* webHistoryItem)
> {
>     g_return_val_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem), NULL);
> 
>-    WebCore::HistoryItem* item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
>+    RefPtr<HistoryItem> item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
>
>     g_return_val_if_fail(item != NULL, NULL);
> 
>-    WebCore::String uri = item->urlString();
>     WebKitWebHistoryItemPrivate* priv = webHistoryItem->priv;
>     g_free(priv->uri);
>+
>+    String uri = item->urlString();
>     priv->uri = g_strdup(uri.utf8().data());

That g_return_if_fail tests a condition after creating a HistoryItem pointer internally, which API users can't influence. Looks like an ASSERT is appropriate here. 

> G_CONST_RETURN gchar* webkit_web_history_item_get_original_uri(WebKitWebHistoryItem* webHistoryItem)
> {
>     g_return_val_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem), NULL);
> 
>-    WebCore::HistoryItem* item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
>+    RefPtr<HistoryItem> item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
> 
>     g_return_val_if_fail(item != NULL, NULL);
> 
>-    WebCore::String originalUri = item->originalURLString();
>     WebKitWebHistoryItemPrivate* priv = webHistoryItem->priv;
>     g_free(priv->originalUri);

Same as above, wrong use of g_return_if_fail.

>     return webHistoryItem->priv->originalUri;
>@@ -424,13 +455,13 @@ G_CONST_RETURN gchar* webkit_web_history_item_get_original_uri(WebKitWebHistoryI
>  *
>  * Returns the last time @webHistoryItem was visited
>  *
>- * Return value: the time in seconds this @webHistoryItem was last visited
>+ * Return value: the time in seconds this @webHistoryItem was last visited or 0 if @webHistoryItem is NULL
>  */
> gdouble webkit_web_history_item_get_last_visited_time(WebKitWebHistoryItem* webHistoryItem)

The history item being NULL is an undefined case of wrong API usage that should not be documented like an expected condition.

[Jan Alonzo]

Created attachment 22504 [details]
updated patch based on christian and sven's comments

I've updated the patch based on your comments. Pretty much everything said was taken into consideration. Thanks! =)

[Eric Seidel (no email)]

Comment on attachment 22504 [details]
updated patch based on christian and sven's comments

This just leads to unsafe access:
 53     WTF::RefPtr<WebCore::HistoryItem> core(WebKitWebHistoryItem*);


Returning a RefPtr is bad-news.  You end up churning the ref-count (the temporary RefPtr used for the return increments the ref count only to immediately decrement it again when it's destroyed).  Also things like this end up being wrong:

core(item).get()

If someone holds onto that pointer, it could have already gone away!   Cause the temporary RefPtr returned from core() could have been the only thing holding on to it.

If you already have a RefPtr, you should return:
const RefPtr& which is safe, although really not any better than Class*

PassRefPtr is how we return or pass ref counted objects.

[Jan Alonzo]

Most fixes are happening in bug #24493 so making this as duplicate of that bug.

Thanks.

*** This bug has been marked as a duplicate of 24493 ***