Modified: releases/WebKitGTK/webkit-2.18/Source/WebKit/ChangeLog (221360 => 221361)
--- releases/WebKitGTK/webkit-2.18/Source/WebKit/ChangeLog 2017-08-30 10:09:40 UTC (rev 221360)
+++ releases/WebKitGTK/webkit-2.18/Source/WebKit/ChangeLog 2017-08-30 10:12:18 UTC (rev 221361)
@@ -1,3 +1,45 @@
+2017-08-28 Carlos Garcia Campos <cgar...@igalia.com>
+
+ [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 <cdu...@apple.com>
Regression(r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9
Modified: releases/WebKitGTK/webkit-2.18/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp (221360 => 221361)
--- releases/WebKitGTK/webkit-2.18/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp 2017-08-30 10:09:40 UTC (rev 221360)
+++ releases/WebKitGTK/webkit-2.18/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp 2017-08-30 10:12:18 UTC (rev 221361)
@@ -86,7 +86,6 @@
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 @@
return false;
}
+ m_mainThreadNotifier.setActive(true);
+
m_databaseDirectory = directory.isolatedCopy();
// Formulate the full path for the database file
@@ -232,6 +233,8 @@
{
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 @@
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 @@
IconDatabase::IconDatabase()
: m_syncTimer(RunLoop::main(), this, &IconDatabase::syncTimerFired)
- , m_mainThreadCallbackCount(0)
, m_client(defaultClient())
{
LOG(IconDatabase, "Creating IconDatabase %p", this);
@@ -780,11 +777,6 @@
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 @@
m_syncDB.clearAllTables();
m_syncDB.runVacuumCommand();
createDatabaseTables(m_syncDB);
-
- LOG(IconDatabase, "Dispatching notification that we removed all icons");
- dispatchDidRemoveAllIconsOnMainThread();
}
void IconDatabase::deleteAllPreparedStatements()
@@ -1932,32 +1921,13 @@
}
}
-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();
});
}
@@ -1964,36 +1934,20 @@
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();
});
}
Modified: releases/WebKitGTK/webkit-2.18/Source/WebKit/UIProcess/API/glib/IconDatabase.h (221360 => 221361)
--- releases/WebKitGTK/webkit-2.18/Source/WebKit/UIProcess/API/glib/IconDatabase.h 2017-08-30 10:09:40 UTC (rev 221360)
+++ releases/WebKitGTK/webkit-2.18/Source/WebKit/UIProcess/API/glib/IconDatabase.h 2017-08-30 10:12:18 UTC (rev 221361)
@@ -33,6 +33,7 @@
#include <wtf/HashMap.h>
#include <wtf/HashSet.h>
#include <wtf/RunLoop.h>
+#include <wtf/glib/RunLoopSourcePriority.h>
#include <wtf/text/StringHash.h>
#include <wtf/text/WTFString.h>
@@ -49,9 +50,7 @@
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 @@
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<void()>&& 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<Function<void()>> notificationQueue;
+ {
+ LockHolder locker(m_notificationQueueLock);
+ notificationQueue = WTFMove(m_notificationQueue);
+ }
+
+ if (!m_isActive.load())
+ return;
+
+ while (!notificationQueue.isEmpty()) {
+ auto function = notificationQueue.takeFirst();
+ function();
+ }
+ }
+
+ Deque<Function<void()>> m_notificationQueue;
+ Lock m_notificationQueueLock;
+ Atomic<bool> m_isActive;
+ RunLoop::Timer<MainThreadNotifier> m_timer;
+ };
+
// *** Main Thread Only ***
public:
IconDatabase();
@@ -287,9 +345,6 @@
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 @@
// Methods to dispatch client callbacks on the main thread
void dispatchDidImportIconURLForPageURLOnMainThread(const String&);
void dispatchDidImportIconDataForPageURLOnMainThread(const String&);
- void dispatchDidRemoveAllIconsOnMainThread();
void dispatchDidFinishURLImportOnMainThread();
- std::atomic<uint32_t> 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<IconDatabaseClient> m_client;
@@ -329,6 +382,8 @@
std::unique_ptr<WebCore::SQLiteStatement> m_updateIconDataStatement;
std::unique_ptr<WebCore::SQLiteStatement> m_setIconInfoStatement;
std::unique_ptr<WebCore::SQLiteStatement> m_setIconDataStatement;
+
+ MainThreadNotifier m_mainThreadNotifier;
};
} // namespace WebKit
