[webkit-reviews] review denied: [Bug 99492] [WK2][GTK] Favicons are incorrectly released before receiving the actual data : [Attachment 169005] Patch proposal

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 99492: [WK2][GTK] Favicons are incorrectly released before receiving the
actual data
https://bugs.webkit.org/show_bug.cgi?id=99492

Attachment 169005: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=169005&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.