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

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has asked  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 169117: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=169117&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
(In reply to comment #3)
> (From update of attachment 169005 [details])
> 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.

Ok. I see. Thanks for the detailed clarifications.

> 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.

I'm now attaching a new patch implementing this idea, which works fine with
ephy as well and it's probably a better approach, as you mentioned.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:-118
> > -	 GOwnPtr<GError> error;
> 
> I wonder why I did this.

Probably I did :P

> > 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.

Ok. Will file a separate bug for this now. Stay tuned