Bug 174788 - [GTK] ASSERTION FAILED: client in WebKit::IconDatabase::setClient
Summary: [GTK] ASSERTION FAILED: client in WebKit::IconDatabase::setClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-24 09:02 PDT by Michael Catanzaro
Modified: 2017-07-24 23:49 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.14 KB, patch)
2017-07-24 09:41 PDT, Michael Catanzaro
cgarcia: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-07-24 09:02:41 PDT
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
Comment 1 Michael Catanzaro 2017-07-24 09:18:38 PDT
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.
Comment 2 Michael Catanzaro 2017-07-24 09:20:36 PDT
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.
Comment 3 Michael Catanzaro 2017-07-24 09:29:53 PDT
(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.
Comment 4 Michael Catanzaro 2017-07-24 09:41:29 PDT
Created attachment 316297 [details]
Patch
Comment 5 Build Bot 2017-07-24 09:42:55 PDT
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 6 Carlos Garcia Campos 2017-07-24 23:03:40 PDT
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.
Comment 7 WebKit Commit Bot 2017-07-24 23:06:07 PDT
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
Comment 8 Carlos Garcia Campos 2017-07-24 23:49:20 PDT
Committed r219863: <http://trac.webkit.org/changeset/219863>