RESOLVED FIXED 175719
[GTK][WPE] ASSERTION FAILED: !isOpen() in WebKit::IconDatabase::~IconDatabase()
https://bugs.webkit.org/show_bug.cgi?id=175719
Summary [GTK][WPE] ASSERTION FAILED: !isOpen() in WebKit::IconDatabase::~IconDatabase()
Carlos Garcia Campos
Reported 2017-08-18 06:09:13 PDT
This is happening always when running /webkit2/WebKitFaviconDatabase/favicon-database-test in debug builds. The last step we do is removing all icons, then the test finishes, which destroys the WebKitFaviconDatabase object that closes the icon database on dispose. The problem is that removing all icons schedules a main thread notification and IconDatabase is not considered closed until all main thread callback have been dispatched. This is never going to happen in the test, because the main loop is no longer running at that point. I don't think it's worth it to consider the database open while main thread callbacks are pending, they are just notification and the client is no longer insterested on tehm afer closing the database. I think it's bettter and simpler to simple cancel the pending callbacks on database close. That ensures that isOpen() after close() is always false.
Attachments
Patch (11.68 KB, patch)
2017-08-18 06:15 PDT, Carlos Garcia Campos
mcatanzaro: review+
Carlos Garcia Campos
Comment 1 2017-08-18 06:15:57 PDT
Build Bot
Comment 2 2017-08-18 06:17:49 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
Michael Catanzaro
Comment 3 2017-08-18 09:23:13 PDT
Comment on attachment 318492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318492&action=review Nice! > Source/WebKit/UIProcess/API/glib/IconDatabase.h:196 > + if (!m_timer.isActive()) I would add a blank line here. > Source/WebKit/UIProcess/API/glib/IconDatabase.h:229 > + RunLoop::Timer<MainThreadNotifier> m_timer; You're certain m_timer does not need to be guarded by a lock? Why not?
Carlos Garcia Campos
Comment 4 2017-08-19 00:52:57 PDT
(In reply to Michael Catanzaro from comment #3) > Comment on attachment 318492 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318492&action=review > > Nice! > > > Source/WebKit/UIProcess/API/glib/IconDatabase.h:196 > > + if (!m_timer.isActive()) > > I would add a blank line here. > > > Source/WebKit/UIProcess/API/glib/IconDatabase.h:229 > > + RunLoop::Timer<MainThreadNotifier> m_timer; > > You're certain m_timer does not need to be guarded by a lock? Why not? Yes, because it's thread-safe
Carlos Garcia Campos
Comment 5 2017-08-28 04:18:35 PDT
Note You need to log in before you can comment on or make changes to this bug.