[GTK][WPE] ASSERTION FAILED: !isOpen() in WebKit::IconDatabase::~IconDatabase()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 318492 [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

[Michael Catanzaro]

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]

(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]

Committed r221238: <http://trac.webkit.org/changeset/221238>