174788 2017-07-24 09:02:41 -0700 [GTK] ASSERTION FAILED: client in WebKit::IconDatabase::setClient 2017-07-24 23:49:20 -0700 1 1 1 Unclassified WebKit WebKitGTK Other PC Linux RESOLVED FIXED P2 Normal --- 1 mcatanzaro mcatanzaro berto bugs-noreply buildbot cgarcia commit-queue gustavo mcatanzaro oldest_to_newest 1331621 0 mcatanzaro 2017-07-24 09:02:41 -0700 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 1331627 1 mcatanzaro 2017-07-24 09:18:38 -0700 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. 1331629 2 mcatanzaro 2017-07-24 09:20:36 -0700 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. 1331639 3 mcatanzaro 2017-07-24 09:29:53 -0700 (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. 1331643 4 316297 mcatanzaro 2017-07-24 09:41:29 -0700 Created attachment 316297 Patch 1331645 5 buildbot 2017-07-24 09:42:55 -0700 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 1331890 6 316297 cgarcia 2017-07-24 23:03:40 -0700 Comment on attachment 316297 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. 1331892 7 316297 commit-queue 2017-07-24 23:06:07 -0700 Comment on attachment 316297 Patch Rejecting attachment 316297 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 1331899 8 cgarcia 2017-07-24 23:49:20 -0700 Committed r219863: <http://trac.webkit.org/changeset/219863> 316297 2017-07-24 09:41:29 -0700 2017-07-24 23:06:07 -0700 Patch bug-174788-20170724114128.patch text/plain 4236 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjE5NzY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDk3YmMxMzdlNzlhM2Y3Yzcw MjRiNGM4ZjRjOWJlMDNiYTFlNTE2ZWMuLmQ1MDYxNTI3ZGY0MGYwNmRjYjM3ZjcwODhkMzUyMDZj NzFlNmNkNWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMgQEAKKzIwMTctMDctMjQgIE1pY2hhZWwg Q2F0YW56YXJvICA8bWNhdGFuemFyb0BpZ2FsaWEuY29tPgorCisgICAgICAgIFtHVEtdIEFTU0VS VElPTiBGQUlMRUQ6IGNsaWVudCBpbiBXZWJLaXQ6Okljb25EYXRhYmFzZTo6c2V0Q2xpZW50Cisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzQ3ODgKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJY29uRGF0YWJh c2UgZW5zdXJlcyB0aGF0IHNldENsaWVudCBpcyBuZXZlciBjYWxsZWQgd2l0aCBudWxscHRyLCBi dXQgV2ViS2l0RmF2aWNvbkRhdGFiYXNlCisgICAgICAgIGRvZXMgZXhhY3RseSB0aGF0LiBGaXJz dCwgZW5zdXJlIGl0J3Mgc2FmZSB0byB1bnNldCB0aGUgY2xpZW50LiBOZXh0LCB3ZSBjYW4gZWl0 aGVyIHJlbW92ZQorICAgICAgICB0aGUgYXNzZXJ0aW9ucyBmcm9tIHNldENsaWVudCBvciBlbHNl IGF1dG9tYXRpY2FsbHkgdW5zZXQgdGhlIGNsaWVudCBpbiBJY29uRGF0YWJhc2U6OmNsb3NlLgor ICAgICAgICBJIG9wdGVkIHRvIGRvIGJvdGguCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2ds aWIvSWNvbkRhdGFiYXNlLmNwcDoKKyAgICAgICAgKFdlYktpdDo6SWNvbkRhdGFiYXNlOjpjbG9z ZSk6CisgICAgICAgIChXZWJLaXQ6Okljb25EYXRhYmFzZTo6c2V0SWNvbkRhdGFGb3JJY29uVVJM KToKKyAgICAgICAgKFdlYktpdDo6SWNvbkRhdGFiYXNlOjpzZXRJY29uVVJMRm9yUGFnZVVSTCk6 CisgICAgICAgICogVUlQcm9jZXNzL0FQSS9nbGliL1dlYktpdEZhdmljb25EYXRhYmFzZS5jcHA6 CisgICAgICAgIChfV2ViS2l0RmF2aWNvbkRhdGFiYXNlUHJpdmF0ZTo6fl9XZWJLaXRGYXZpY29u RGF0YWJhc2VQcml2YXRlKTogRGVsZXRlZC4KKyAgICAgICAgKHdlYmtpdEZhdmljb25EYXRhYmFz ZURpc3Bvc2UpOiBEZWxldGVkLgorCiAyMDE3LTA3LTIyICBXZW5zb24gSHNpZWggIDx3ZW5zb25f aHNpZWhAYXBwbGUuY29tPgogCiAgICAgICAgIFtpT1MgV0syXSBSZW1vdmUgX1dLRHJhZ2dhYmxl RWxlbWVudEluZm8gYW5kIGZvbGQgUG9zaXRpb25JbmZvcm1hdGlvblRlc3RzIGludG8gV0tSZXF1 ZXN0QWN0aXZhdGVkRWxlbWVudEluZm8KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9j ZXNzL0FQSS9nbGliL0ljb25EYXRhYmFzZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9B UEkvZ2xpYi9JY29uRGF0YWJhc2UuY3BwCmluZGV4IDY2MDE5ZjRlZjUwMWI0YWY2NGQzMThmM2Yx MTA1YjdjODVlNWJlODQuLmQ3MDUxZDIzMzc0OGM2NTQwNDZiZTA1YTY4MzhmNmNiYjUyYzNhYmUg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9nbGliL0ljb25EYXRhYmFz ZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL2dsaWIvSWNvbkRhdGFiYXNl LmNwcApAQCAtMTg5LDEwICsxODksOCBAQCBJY29uRGF0YWJhc2U6OlBhZ2VVUkxTbmFwc2hvdCBJ Y29uRGF0YWJhc2U6OlBhZ2VVUkxSZWNvcmQ6OnNuYXBzaG90KGJvb2wgZm9yRGVsZQogCiB2b2lk IEljb25EYXRhYmFzZTo6c2V0Q2xpZW50KHN0ZDo6dW5pcXVlX3B0cjxJY29uRGF0YWJhc2VDbGll bnQ+JiYgY2xpZW50KQogewotICAgIC8vIFdlIGRvbid0IGFsbG93IGEgbnVsbCBjbGllbnQsIGJl Y2F1c2Ugd2UgbmV2ZXIgbnVsbCBjaGVjayBpdCBhbnl3aGVyZSBpbiB0aGlzIGNvZGUKLSAgICAv LyBBbHNvIGRvbid0IGFsbG93IGEgY2xpZW50IGNoYW5nZSBhZnRlciB0aGUgdGhyZWFkIGhhcyBh bHJlYWR5IGJlZ2FuCisgICAgLy8gRG9uJ3QgYWxsb3cgYSBjbGllbnQgY2hhbmdlIGFmdGVyIHRo ZSB0aHJlYWQgaGFzIGFscmVhZHkgYmVnYW4KICAgICAvLyAoc2V0dGluZyB0aGUgY2xpZW50IHNo b3VsZCBvY2N1ciBiZWZvcmUgdGhlIGRhdGFiYXNlIGlzIG9wZW5lZCkKLSAgICBBU1NFUlQoY2xp ZW50KTsKICAgICBBU1NFUlQoIW1fc3luY1RocmVhZFJ1bm5pbmcpOwogICAgIGlmICghY2xpZW50 IHx8IG1fc3luY1RocmVhZFJ1bm5pbmcpCiAgICAgICAgIHJldHVybjsKQEAgLTI1NSw2ICsyNTMs OCBAQCB2b2lkIEljb25EYXRhYmFzZTo6Y2xvc2UoKQogICAgIC8vIEJ1dCBpZiBpdCBpcyBjbG9z ZWQsIG5vdGlmeSB0aGUgY2xpZW50IG5vdy4KICAgICBpZiAoIWlzT3BlbigpICYmIG1fY2xpZW50 KQogICAgICAgICBtX2NsaWVudC0+ZGlkQ2xvc2UoKTsKKworICAgIG1fY2xpZW50ID0gbnVsbHB0 cjsKIH0KIAogdm9pZCBJY29uRGF0YWJhc2U6OnJlbW92ZUFsbEljb25zKCkKQEAgLTU4OSw3ICs1 ODksOCBAQCB2b2lkIEljb25EYXRhYmFzZTo6c2V0SWNvbkRhdGFGb3JJY29uVVJMKFJlZlB0cjxT aGFyZWRCdWZmZXI+JiYgZGF0YSwgY29uc3QgU3RyaQogICAgICAgICAgICAgQXV0b2RyYWluZWRQ b29sIHBvb2w7CiAKICAgICAgICAgICAgIExPRyhJY29uRGF0YWJhc2UsICJEaXNwYXRjaGluZyBu b3RpZmljYXRpb24gdGhhdCByZXRhaW5pbmcgcGFnZVVSTCAlcyBoYXMgYSBuZXcgaWNvbiIsIHVy bEZvckxvZ2dpbmcocGFnZVVSTCkuYXNjaWkoKS5kYXRhKCkpOwotICAgICAgICAgICAgbV9jbGll bnQtPmRpZENoYW5nZUljb25Gb3JQYWdlVVJMKHBhZ2VVUkwpOworICAgICAgICAgICAgaWYgKG1f Y2xpZW50KQorICAgICAgICAgICAgICAgIG1fY2xpZW50LT5kaWRDaGFuZ2VJY29uRm9yUGFnZVVS TChwYWdlVVJMKTsKICAgICAgICAgfQogICAgIH0KIH0KQEAgLTY1OCw3ICs2NTksOCBAQCB2b2lk IEljb25EYXRhYmFzZTo6c2V0SWNvblVSTEZvclBhZ2VVUkwoY29uc3QgU3RyaW5nJiBpY29uVVJM T3JpZ2luYWwsIGNvbnN0IFN0cgogCiAgICAgICAgIExPRyhJY29uRGF0YWJhc2UsICJEaXNwYXRj aGluZyBub3RpZmljYXRpb24gdGhhdCB3ZSBjaGFuZ2VkIGFuIGljb24gbWFwcGluZyBmb3IgdXJs ICVzIiwgdXJsRm9yTG9nZ2luZyhwYWdlVVJMKS5hc2NpaSgpLmRhdGEoKSk7CiAgICAgICAgIEF1 dG9kcmFpbmVkUG9vbCBwb29sOwotICAgICAgICBtX2NsaWVudC0+ZGlkQ2hhbmdlSWNvbkZvclBh Z2VVUkwocGFnZVVSTCk7CisgICAgICAgIGlmIChtX2NsaWVudCkKKyAgICAgICAgICAgIG1fY2xp ZW50LT5kaWRDaGFuZ2VJY29uRm9yUGFnZVVSTChwYWdlVVJMKTsKICAgICB9CiB9CiAKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9nbGliL1dlYktpdEZhdmljb25EYXRh YmFzZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9BUEkvZ2xpYi9XZWJLaXRGYXZpY29u RGF0YWJhc2UuY3BwCmluZGV4IDU4NjkyYmYyMTQ0ODUwZGRjMmU0MmExMDZiMjdhMWY0MWViMDAw MTAuLmVhMWE0MGMxY2M1NzYwNzY2ZDQ2MGNhOTczNDY3OWJmYjAzOGU3NGYgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9nbGliL1dlYktpdEZhdmljb25EYXRhYmFzZS5j cHAKKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL2dsaWIvV2ViS2l0RmF2aWNvbkRh dGFiYXNlLmNwcApAQCAtNzAsMTEgKzcwLDYgQEAgdHlwZWRlZiBWZWN0b3I8R1JlZlB0cjxHVGFz az4gPiBQZW5kaW5nSWNvblJlcXVlc3RWZWN0b3I7CiB0eXBlZGVmIEhhc2hNYXA8U3RyaW5nLCBQ ZW5kaW5nSWNvblJlcXVlc3RWZWN0b3IqPiBQZW5kaW5nSWNvblJlcXVlc3RNYXA7CiAKIHN0cnVj dCBfV2ViS2l0RmF2aWNvbkRhdGFiYXNlUHJpdmF0ZSB7Ci0gICAgfl9XZWJLaXRGYXZpY29uRGF0 YWJhc2VQcml2YXRlKCkKLSAgICB7Ci0gICAgICAgIGljb25EYXRhYmFzZS0+c2V0Q2xpZW50KG51 bGxwdHIpOwotICAgIH0KLQogICAgIHN0ZDo6dW5pcXVlX3B0cjxJY29uRGF0YWJhc2U+IGljb25E YXRhYmFzZTsKICAgICBWZWN0b3I8c3RkOjpwYWlyPFN0cmluZywgRnVuY3Rpb248dm9pZChib29s KT4+PiBwZW5kaW5nTG9hZERlY2lzaW9uczsKICAgICBQZW5kaW5nSWNvblJlcXVlc3RNYXAgcGVu ZGluZ0ljb25SZXF1ZXN0czsK