[Webkit-unassigned] [Bug 99492] New: [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

           Summary: [WK2][GTK] Favicons are incorrectly released before
                    receiving the actual data
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: Unspecified
        OS/Version: Unspecified
            Status: NEW
          Keywords: Gtk
          Severity: Normal
          Priority: P2
         Component: WebKit Gtk
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: mario at webkit.org
                CC: cgarcia at igalia.com


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

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