Today I found an issue with the following piece of code in WebKitFaviconDatabase.cpp: static cairo_surface_t* getIconSurfaceSynchronously(WebKitFaviconDatabase* database, const String& pageURL, GError** error) { ASSERT(isMainThread()); database->priv->iconDatabase->retainIconForPageURL(pageURL); // The exact size we pass is irrelevant to the iconDatabase code. // We must pass something greater than 0x0 to get an icon. WebCore::Image* iconImage = database->priv->iconDatabase->imageForPageURL(pageURL, WebCore::IntSize(1, 1)); if (!iconImage) { g_set_error(error, WEBKIT_FAVICON_DATABASE_ERROR, WEBKIT_FAVICON_DATABASE_ERROR_FAVICON_UNKNOWN, _("Unknown favicon for page %s"), pageURL.utf8().data()); database->priv->iconDatabase->releaseIconForPageURL(pageURL); return 0; } WebCore::NativeImagePtr icon = iconImage->nativeImageForCurrentFrame(); if (!icon) { g_set_error(error, WEBKIT_FAVICON_DATABASE_ERROR, WEBKIT_FAVICON_DATABASE_ERROR_FAVICON_NOT_FOUND, _("Page %s does not have a favicon"), pageURL.utf8().data()); database->priv->iconDatabase->releaseIconForPageURL(pageURL); return 0; } } The problem with this is that sometimes this method is called from places (webkit_favicon_database_get_favicon()) where getting a NULL value out of this function does not necessarily mean that there's no icon for the page URL, but simply that is not a known one yet and that we might have to wait for the iconDataReady callback to be called to be really sure whether there's a valid icon or not. The issue with the code above is that it always calls to retainIconForPageURL() before imageForPageURL *but* then it directly calls to releaseIconForPageURL() once a valid image is not there. And this is not correct since it could be too early to make such a decision, since we must have to wait for the iconDataReady callback to be called before being completely sure about this. So, we should not release the icon until we are completely sure that there is not a known icon for the given page URL, that is, in the webkit_favicon_database_get_favicon_finish() method. I found this issue while testing the patches I have for Epiphany [1], following these steps: 1. Remove the favicon database file on disk and open Ephy. 2. Load a web with a favicon (e.g. www.igalia.com) and save it as a bookmark (Ctrl+D) 3. Close the only tab (causing Ephy to close), and check that the icon and its data is stored on disk, by inspecting the sqlite DB and the IconData table (should be an entry with NON NULL data in there). 4. Open Ephy again (should show you the overview), but do *NOT* load any web yet. 5. Navigate through the "gear" menu until "Bookmarks", causing a submenu to be created on-the-fly and shown with the "Igalia" entry we saved in (2). The expected outcome at this point is that the "Igalia" entry should show the favicon (saved to the sqlite DB in the previous session). However, no favicon is shown. After some investigation, I found out that the data for the icon in DB was there all the time until that the submenu was tried to be created in (5), which called webkit_favicon_database_get_favicon() to try to retrieve that icon. And the problem seemed to be that this *first* call to webkit_favicon_database_get_favicon() after restarting ephy was caused releaseIconForPageURL() to be called in getIconSurfaceSynchronously() too early, before waiting for the iconDataReady callback to be executed (as it should). Then, as this was the first time the database was actually used, the icon's data would be removed from the DB right away while performing the initial synchronization process in the IconDatabase, causing that no icon would finally be available when the iconDataReady callback was finally called. I've a patch in a local branch fixing this issue. Will upload it soon [1] https://bugzilla.gnome.org/show_bug.cgi?id=679370
Created attachment 169005 [details] Patch proposal Here's a patch that makes favicons work fine in Ephy (while still making the unit tests pass) Please review, thanks
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 169005 [details] Patch proposal 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.
Created attachment 169117 [details] 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
Comment on attachment 169117 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=169117&action=review I'm still worried that the idle thing works in some cases only, because in the end you don't know when the idle will be called. So, since what we want is to release the icons when we are sure, and after finish, I think we could schedule the idle when the data struct finishes or even do it in the async data struct destructor directly. We retain the icon in get_favicon() right before calling getIconSurfaceSynchronously(). if we don't have an icon we mark the icon to be released (with a bool parameter in the async data struct, for example). If we eventually get an icon in processPendingIconRequests we set the flag to false. In the async data destrcutor we check the flag and release the icon (or schedule a release in an idle). > Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:131 > +static void releaseIconOnIdle(WebKitFaviconDatabase* database, const String& pageURL) releaseIconForPageURLInIdle() ? > Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:133 > + GetFaviconSurfaceAsyncData* data = createGetFaviconSurfaceAsyncData(); I'm not sure using the same data struct is a good idea, I would define a new one. > Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:136 > + g_idle_add(releaseIconOnIdleSourceFunc, data); You can use g_idle_add_full and pass the destroy func as GDestroyNotify, so that you can be sure the data will be destroyed even if the source is destroyed before the idle func is called.
Created attachment 169155 [details] Patch proposal (In reply to comment #5) > (From update of attachment 169117 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169117&action=review > > I'm still worried that the idle thing works in some cases only, because in the end > you don't know when the idle will be called. So, since what we want is to release > the icons when we are sure, and after finish, I think we could schedule the idle > when the data struct finishes or even do it in the async data struct destructor > directly. We retain the icon in get_favicon() right before calling > getIconSurfaceSynchronously(). if we don't have an icon we mark the icon to be > released (with a bool parameter in the async data struct, for example). If we > eventually get an icon in processPendingIconRequests we set the flag to false. In > the async data destrcutor we check the flag and release the icon (or schedule a > release in an idle). Ok. I'm attaching now a new patch implementing this (third) new approach :)
Comment on attachment 169155 [details] Patch proposal Excellent, thanks!
Committed r131588: <http://trac.webkit.org/changeset/131588>