[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


Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #169005|0                           |1
        is obsolete|                            |
 Attachment #169117|                            |review?
               Flag|                            |




--- Comment #4 from Mario Sanchez Prada <mario at webkit.org>  2012-10-17 01:25:41 PST ---
Created an attachment (id=169117)
 --> (https://bugs.webkit.org/attachment.cgi?id=169117&action=review)
Patch proposal

(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

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