diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog index 026e18736215..32a688a22642 100644 --- a/Source/WebKit/ChangeLog +++ b/Source/WebKit/ChangeLog @@ -1,3 +1,45 @@ +2017-08-28 Carlos Garcia Campos + + [GTK][WPE] ASSERTION FAILED: !isOpen() in WebKit::IconDatabase::~IconDatabase() + https://bugs.webkit.org/show_bug.cgi?id=175719 + + Reviewed by Michael Catanzaro. + + 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 callbacks 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 notifications + and the client is no longer insterested on them afer closing the database. I think it's bettter and simpler to + simply cancel the pending callbacks on database close. That ensures that isOpen() after close() is always + false. This patch adds a helper private class to schedule notifications to the main thread that can be cancelled + on database close. It also removes the didClose() notification because it was unused and because it's pointless + now that we know the database is closed after close(). + + * UIProcess/API/glib/IconDatabase.cpp: + (WebKit::IconDatabase::open): Mark the main thread notifier as active. + (WebKit::IconDatabase::close): Mark the main thread notifier as not active. + (WebKit::IconDatabase::IconDatabase): Remove m_mainThreadCallbackCount initialization. + (WebKit::IconDatabase::isOpen const): Do what isOpenBesidesMainThreadCallbacks() used to do. + (WebKit::IconDatabase::removeAllIconsOnThread): Remove the notification because it's currently unused. + (WebKit::IconDatabase::dispatchDidImportIconURLForPageURLOnMainThread): Use MainThreadNotifier. + (WebKit::IconDatabase::dispatchDidImportIconDataForPageURLOnMainThread): Ditto. + (WebKit::IconDatabase::dispatchDidFinishURLImportOnMainThread): Ditto. + (WebKit::IconDatabase::isOpenBesidesMainThreadCallbacks const): Deleted. + (WebKit::IconDatabase::checkClosedAfterMainThreadCallback): Deleted. + (WebKit::IconDatabase::dispatchDidRemoveAllIconsOnMainThread): Deleted. + * UIProcess/API/glib/IconDatabase.h: + (WebKit::IconDatabaseClient::didChangeIconForPageURL): + (WebKit::IconDatabaseClient::didFinishURLImport): + (WebKit::IconDatabase::MainThreadNotifier::MainThreadNotifier): + (WebKit::IconDatabase::MainThreadNotifier::setActive): + (WebKit::IconDatabase::MainThreadNotifier::notify): + (WebKit::IconDatabase::MainThreadNotifier::stop): + (WebKit::IconDatabase::MainThreadNotifier::timerFired): + (WebKit::IconDatabaseClient::didRemoveAllIcons): Deleted. + (WebKit::IconDatabaseClient::didClose): Deleted. + 2017-08-23 Chris Dumez Regression(r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9 diff --git a/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp b/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp index 0e25a67d60c4..2208eaef0501 100644 --- a/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp +++ b/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp @@ -86,7 +86,6 @@ class DefaultIconDatabaseClient final : public IconDatabaseClient { void didImportIconURLForPageURL(const String&) override { } void didImportIconDataForPageURL(const String&) override { } void didChangeIconForPageURL(const String&) override { } - void didRemoveAllIcons() override { } void didFinishURLImport() override { } }; @@ -210,6 +209,8 @@ bool IconDatabase::open(const String& directory, const String& filename) return false; } + m_mainThreadNotifier.setActive(true); + m_databaseDirectory = directory.isolatedCopy(); // Formulate the full path for the database file @@ -232,6 +233,8 @@ void IconDatabase::close() { ASSERT_NOT_SYNC_THREAD(); + m_mainThreadNotifier.stop(); + if (m_syncThreadRunning) { // Set the flag to tell the sync thread to wrap it up m_threadTerminationRequested = true; @@ -249,11 +252,6 @@ void IconDatabase::close() m_syncDB.close(); - // If there are still main thread callbacks in flight then the database might not actually be closed yet. - // But if it is closed, notify the client now. - if (!isOpen() && m_client) - m_client->didClose(); - m_client = nullptr; } @@ -734,7 +732,6 @@ void IconDatabase::checkIntegrityBeforeOpening() IconDatabase::IconDatabase() : m_syncTimer(RunLoop::main(), this, &IconDatabase::syncTimerFired) - , m_mainThreadCallbackCount(0) , m_client(defaultClient()) { LOG(IconDatabase, "Creating IconDatabase %p", this); @@ -779,11 +776,6 @@ void IconDatabase::syncTimerFired() // ****************** bool IconDatabase::isOpen() const -{ - return isOpenBesidesMainThreadCallbacks() || m_mainThreadCallbackCount; -} - -bool IconDatabase::isOpenBesidesMainThreadCallbacks() const { LockHolder locker(m_syncLock); return m_syncThreadRunning || m_syncDB.isOpen(); @@ -1619,9 +1611,6 @@ void IconDatabase::removeAllIconsOnThread() m_syncDB.clearAllTables(); m_syncDB.runVacuumCommand(); createDatabaseTables(m_syncDB); - - LOG(IconDatabase, "Dispatching notification that we removed all icons"); - dispatchDidRemoveAllIconsOnMainThread(); } void IconDatabase::deleteAllPreparedStatements() @@ -1932,68 +1921,33 @@ void IconDatabase::writeIconSnapshotToSQLDatabase(const IconSnapshot& snapshot) } } -void IconDatabase::checkClosedAfterMainThreadCallback() -{ - ASSERT_NOT_SYNC_THREAD(); - - // If there are still callbacks in flight from the sync thread we cannot possibly be closed. - if (--m_mainThreadCallbackCount) - return; - - // Even if there's no more pending callbacks the database might otherwise still be open. - if (isOpenBesidesMainThreadCallbacks()) - return; - - // This database is now actually closed! But first notify the client. - if (m_client) - m_client->didClose(); -} - void IconDatabase::dispatchDidImportIconURLForPageURLOnMainThread(const String& pageURL) { ASSERT_ICON_SYNC_THREAD(); - ++m_mainThreadCallbackCount; - callOnMainThread([this, pageURL = pageURL.isolatedCopy()] { + m_mainThreadNotifier.notify([this, pageURL = pageURL.isolatedCopy()] { if (m_client) m_client->didImportIconURLForPageURL(pageURL); - checkClosedAfterMainThreadCallback(); }); } void IconDatabase::dispatchDidImportIconDataForPageURLOnMainThread(const String& pageURL) { ASSERT_ICON_SYNC_THREAD(); - ++m_mainThreadCallbackCount; - callOnMainThread([this, pageURL = pageURL.isolatedCopy()] { + m_mainThreadNotifier.notify([this, pageURL = pageURL.isolatedCopy()] { if (m_client) m_client->didImportIconDataForPageURL(pageURL); - checkClosedAfterMainThreadCallback(); - }); -} - -void IconDatabase::dispatchDidRemoveAllIconsOnMainThread() -{ - ASSERT_ICON_SYNC_THREAD(); - ++m_mainThreadCallbackCount; - - callOnMainThread([this] { - if (m_client) - m_client->didRemoveAllIcons(); - checkClosedAfterMainThreadCallback(); }); } void IconDatabase::dispatchDidFinishURLImportOnMainThread() { ASSERT_ICON_SYNC_THREAD(); - ++m_mainThreadCallbackCount; - callOnMainThread([this] { + m_mainThreadNotifier.notify([this] { if (m_client) m_client->didFinishURLImport(); - checkClosedAfterMainThreadCallback(); }); } diff --git a/Source/WebKit/UIProcess/API/glib/IconDatabase.h b/Source/WebKit/UIProcess/API/glib/IconDatabase.h index 6f10e412d625..51659e654f2e 100644 --- a/Source/WebKit/UIProcess/API/glib/IconDatabase.h +++ b/Source/WebKit/UIProcess/API/glib/IconDatabase.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -49,9 +50,7 @@ class IconDatabaseClient { virtual void didImportIconURLForPageURL(const String&) { } virtual void didImportIconDataForPageURL(const String&) { } virtual void didChangeIconForPageURL(const String&) { } - virtual void didRemoveAllIcons() { } virtual void didFinishURLImport() { } - virtual void didClose() { } }; class IconDatabase { @@ -172,6 +171,65 @@ class IconDatabase { int m_retainCount { 0 }; }; + class MainThreadNotifier { + public: + MainThreadNotifier() + : m_timer(RunLoop::main(), this, &MainThreadNotifier::timerFired) + { + m_timer.setPriority(RunLoopSourcePriority::MainThreadDispatcherTimer); + } + + void setActive(bool active) + { + m_isActive.store(active); + } + + void notify(Function&& notification) + { + if (!m_isActive.load()) + return; + + { + LockHolder locker(m_notificationQueueLock); + m_notificationQueue.append(WTFMove(notification)); + } + + if (!m_timer.isActive()) + m_timer.startOneShot(0_s); + } + + void stop() + { + setActive(false); + m_timer.stop(); + LockHolder locker(m_notificationQueueLock); + m_notificationQueue.clear(); + } + + private: + void timerFired() + { + Deque> notificationQueue; + { + LockHolder locker(m_notificationQueueLock); + notificationQueue = WTFMove(m_notificationQueue); + } + + if (!m_isActive.load()) + return; + + while (!notificationQueue.isEmpty()) { + auto function = notificationQueue.takeFirst(); + function(); + } + } + + Deque> m_notificationQueue; + Lock m_notificationQueueLock; + Atomic m_isActive; + RunLoop::Timer m_timer; + }; + // *** Main Thread Only *** public: IconDatabase(); @@ -287,9 +345,6 @@ class IconDatabase { void performRetainIconForPageURL(const String&, int retainCount); void performReleaseIconForPageURL(const String&, int releaseCount); - bool isOpenBesidesMainThreadCallbacks() const; - void checkClosedAfterMainThreadCallback(); - bool m_initialPruningComplete { false }; void setIconURLForPageURLInSQLDatabase(const String&, const String&); @@ -306,9 +361,7 @@ class IconDatabase { // Methods to dispatch client callbacks on the main thread void dispatchDidImportIconURLForPageURLOnMainThread(const String&); void dispatchDidImportIconDataForPageURLOnMainThread(const String&); - void dispatchDidRemoveAllIconsOnMainThread(); void dispatchDidFinishURLImportOnMainThread(); - std::atomic m_mainThreadCallbackCount; // The client is set by the main thread before the thread starts, and from then on is only used by the sync thread std::unique_ptr m_client; @@ -329,6 +382,8 @@ class IconDatabase { std::unique_ptr m_updateIconDataStatement; std::unique_ptr m_setIconInfoStatement; std::unique_ptr m_setIconDataStatement; + + MainThreadNotifier m_mainThreadNotifier; }; } // namespace WebKit