Bug 174788 – [GTK] ASSERTION FAILED: client in WebKit::IconDatabase::setClient

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

After some of the recent icon database changes, this assertion is hit 100% of the time when closing Epiphany:

ASSERTION FAILED: client
../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp(198) : void WebKit::IconDatabase::setClient(std::unique_ptr<WebKit::IconDatabaseClient>&&)

It looks simple:

#0  0x00007f6ff5ee4fdf in WTFCrash ()
    at ../../Source/WTF/wtf/Assertions.cpp:278
No locals.
#1  0x00007f6ffccc1e81 in (anonymous namespace)::IconDatabase::setClient (
    this=0x7f6fe38cf000, client=...)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:198
        __PRETTY_FUNCTION__ = "void WebKit::IconDatabase::setClient(std::unique_ptr<WebKit::IconDatabaseClient>&&)"
#2  0x00007f6ffcced0aa in _WebKitFaviconDatabasePrivate::~_WebKitFaviconDatabasePrivate (this=0x273db00, __in_chrg=<optimized out>)
    at ../../Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:75
No locals.
#3  0x00007f6ffcceb941 in webkit_favicon_database_finalize (object=0x273db70)
    at ../../Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:85
        self = 0x273db70
#4  0x00007f7005e2422b in g_object_unref (_object=0x273db70)
    at /home/mcatanzaro/Projects/GNOME/glib/gobject/gobject.c:3314
        weak_locations = 0x0
        object = 0x273db70
        old_ref = 1
        __func__ = "g_object_unref"
#5  0x00007f6ffccee26d in WTF::derefGPtr<_WebKitFaviconDatabase> (
    ptr=0x273db70) at ../../Source/WTF/wtf/glib/GRefPtr.h:250
No locals.
#6  0x00007f6ffcced7ba in WTF::GRefPtr<_WebKitFaviconDatabase>::~GRefPtr (
    this=0x259c010, __in_chrg=<optimized out>)
    at ../../Source/WTF/wtf/glib/GRefPtr.h:76
        ptr = 0x273db70
#7  0x00007f6ffcd18c10 in _WebKitWebContextPrivate::~_WebKitWebContextPrivate (
    this=0x259c000, __in_chrg=<optimized out>)
    at ../../Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:163
No locals.
#8  0x00007f6ffcd1376e in webkit_web_context_finalize (object=0x259c100)
    at ../../Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:229
        self = 0x259c100
#9  0x00007f7005e2422b in g_object_unref (_object=0x259c100)
    at /home/mcatanzaro/Projects/GNOME/glib/gobject/gobject.c:3314
        weak_locations = 0x0
        object = 0x259c100
        old_ref = 1
        __func__ = "g_object_unref"
#10 0x00007f70066e851d in ephy_embed_shell_dispose (object=0x2567280)
    at ../../../../Projects/GNOME/epiphany/embed/ephy-embed-shell.c:122
        _pp = 0x2567130
        _p = 0x259c100
        _destroy = 0x7f7005e23f80 <g_object_unref>
        priv = 0x2567130
#11 0x00007f70066ceb16 in ephy_shell_dispose (object=0x2567280)
    at ../../../../Projects/GNOME/epiphany/src/ephy-shell.c:658
        shell = 0x2567280
#12 0x00007f7005e2412f in g_object_unref (_object=0x2567280)
    at /home/mcatanzaro/Projects/GNOME/glib/gobject/gobject.c:3277
        weak_locations = 0x0
        object = 0x2567280
        old_ref = 1
        __func__ = "g_object_unref"
#13 0x000000000040347d in main (argc=1, argv=0x7fff4b864638)
    at ../../../../Projects/GNOME/epiphany/src/ephy-main.c:436
        option_context = 0x24cb3f0
        option_group = 0x24cb480
        error = 0x0
        user_time = 0
        arbitrary_url = 0
        ctx = 0x2581380
        startup_flags = EPHY_STARTUP_NEW_TAB
        mode = EPHY_EMBED_SHELL_MODE_PRIVATE
        status = 0
        flags = (EPHY_FILE_HELPERS_PRIVATE_PROFILE | EPHY_FILE_HELPERS_ENSURE_EXISTS)
        desktop_info = 0x0

[Michael Catanzaro]

We have:

void IconDatabase::setClient(std::unique_ptr<IconDatabaseClient>&& client)
{
    // We don't allow a null client, because we never null check it anywhere in this code
    // Also don't allow a client change after the thread has already began
    // (setting the client should occur before the database is opened)
    ASSERT(client);
    ASSERT(!m_syncThreadRunning);
    if (!client || m_syncThreadRunning)
        return;

    m_client = WTFMove(client);
}

And:

struct _WebKitFaviconDatabasePrivate {
    ~_WebKitFaviconDatabasePrivate()
    {
        iconDatabase->setClient(nullptr);
    }

    // ...
};

I guess we have to null-check client everywhere. That's not so hard.

[Michael Catanzaro]

Well I didn't read. Null-checking is easy, but it's still not safe to unset the client while the sync thread is running. We have to somehow stop the sync thread before we can do that.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> We have to somehow stop the sync thread before we can do that.

No, because m_client is only ever used on the main thread. The IconDatabase code is careful to ensure this.

Still, it's probably best to leave the assertions and instead unset the client in IconDatabase::close.

[Michael Catanzaro]

Created attachment 316297 [details]
Patch

[Build Bot]

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

[Carlos Garcia Campos]

Comment on attachment 316297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316297&action=review

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:192
> -    // We don't allow a null client, because we never null check it anywhere in this code
> -    // Also don't allow a client change after the thread has already began
> +    // Don't allow a client change after the thread has already began

Yes, that comment was already not true, there are actually a lot of m_client null checks. Maybe we can also remove the default client thing now that we check it everywhere.

[WebKit Commit Bot]

Comment on attachment 316297 [details]
Patch

Rejecting attachment 316297 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 316297, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
succeeded at 188 (offset -1 lines).
Hunk #2 succeeded at 252 (offset -1 lines).
Hunk #3 succeeded at 586 with fuzz 2 (offset -3 lines).
Hunk #4 FAILED at 656.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit/UIProcess/API/glib/IconDatabase.cpp.rej
patching file Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Carlos Garcia Campos']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4182739

[Carlos Garcia Campos]

Committed r219863: <http://trac.webkit.org/changeset/219863>