[Webkit-unassigned] [Bug 99492] [WK2][GTK] Favicons are incorrectly released before receiving the actual data

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99492


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #169005|review?                     |review-
               Flag|                            |




--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-10-17 00:16:21 PST ---
(From update of attachment 169005)
View in context: https://bugs.webkit.org/attachment.cgi?id=169005&action=review

I like the idea, thanks for the detailed analysis, but there are several problems with the patch. 

 - You are assuming that in finish() is always called after data-ready, which is not always true. So you might end up releasing an icon never retained.
 - In general retain/release is not balanced.
 - You can't be sure that finish will be called, in GIO async pattern call an async method with a NULL GASyncREadyCallback is perfectly valid, although unlikely in this case.

The first time getIconSurfaceSynchronously() is called is in webkit_favicon_database_get_favicon(), if it fails, finish is called in an idle and the icon is released. Then icon-data ready is called and pending icon requests are processed, if it fails finish is called directly and the icon is released. The only case that makes a difference is the first one, when we ask for the first time, instead of releasing the icon immediately we do it in an idle. So, I think we could try to do that always, in getIconSurfaceSynchronously() schedule an idle to release the icon or keep a list of icons to release and figure out the best moment to release them.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:-118
> -    GOwnPtr<GError> error;

I wonder why I did this.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:165
> -                g_propagate_error(&data->error.outPtr(), error.release());
> +                g_simple_async_result_take_error(result, error.release());

Makes a lot of sense. If you move this to another patch I'll r+ it, since this has nothing to do with the discussion on this bug.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.