Bug 188110 – [GTK] API test /webkit/WebKitFaviconDatabase/get-favicon often times out

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

API test /webkit/WebKitFaviconDatabase/favicon-database-test sometimes times out.

[Adrian Perez]

I've been looking a bit at this. The issue is in this bit inside the test function:

    test->waitUntilLoadFinishedAndFaviconChanged();
    test->waitUntilFaviconURIChanged();  // <-- timeout due to this not returning

From the looks of it, the favicon URI has already been changed *before* the call
to waitUntilFaviconURIChanged(), as a consequence of the previous actions, and
then there is nothing to wait. I'll double check my hypothesis, and in that case
rewriting the function to check whether the change has already happened and
avoiding spinning the main loop in that case should be enough.

[Adrian Perez]

(In reply to Adrian Perez from comment #1)

> From the looks of it, the favicon URI has already been changed *before* the call
> to waitUntilFaviconURIChanged(), as a consequence of the previous actions, and
> then there is nothing to wait.

Confirmed, this is exactly what is happening. We need to make the checks more
robust, and if the favicon URI has already changed *before* trying to wait, skip
the wait and plow ahead.

[Adrian Perez]

(In reply to Adrian Perez from comment #2)
> (In reply to Adrian Perez from comment #1)
> 
> > From the looks of it, the favicon URI has already been changed *before* the call
> > to waitUntilFaviconURIChanged(), as a consequence of the previous actions, and
> > then there is nothing to wait.
> 
> Confirmed, this is exactly what is happening. We need to make the checks more
> robust, and if the favicon URI has already changed *before* trying to wait,
> skip
> the wait and plow ahead.

Scratch that. What is really happening is that this is testing the case in which
a page has *no favicon*: WebKit will try to load "/favicon.ico" as fallback, and
the test server returns HTTP 404 Not Found... and therefore it makes sense
that the WebKitFaviconDatabase:favicon-changed signal never gets emitted,
because a favicon was never loaded for that page.

I think it makes sense that the :favicon-changed signal is NOT emitted if the
icon could not be loaded. Reading the API documentation, there is an implicit
expectation that the favicon *will be available* if the :favicon-changed signal
was emitted:

   “This signal is emitted when the favicon URI of page_uri has been changed
    to favicon_uri in the database. You can connect to this signal and call
    webkit_favicon_database_get_favicon() to get the favicon.”

      -- https://webkitgtk.org/reference/webkitgtk/2.52.0/signal.FaviconDatabase.favicon-changed.html

If the signal was emitted, the user of the API would expect to be able to
fetch the favicon but it would not be available (as it was not loaded!).
Therefore, the current behaviour makes sense: if there is no favicon image
because it cannot be loaded, then there is no :favicon-changed emission.

No idea how this test had the chance to ever working properly. I suppose it
worked by chance at some point.

[Adrian Perez]

Pull request: https://github.com/WebKit/WebKit/pull/61000

[EWS]

Committed 309642@main (dd71161da413): <https://commits.webkit.org/309642@main>

Reviewed commits have been landed. Closing PR #61000 and removing active labels.

[Claudio Saavedra]

The test is failing in the API bots:

  /webkit/WebKitFaviconDatabase/get-favicon:                          Leaked objects: WebKitFaviconDatabase(0x55c9a31f24b0 - 1 left)
FAIL