Skip to content

Commit

Permalink
Merge r221238 - [GTK][WPE] ASSERTION FAILED: !isOpen() in WebKit::Ico…
Browse files Browse the repository at this point in the history 
…nDatabase::~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.
  • Loading branch information
@carlosgcampos
carlosgcampos committed Aug 30, 2017
1 parent 8d7b252 commit cb4384a
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 60 deletions.
42 changes: 42 additions & 0 deletions Source/WebKit/ChangeLog
View file
@@ -1,3 +1,45 @@
2017-08-28 Carlos Garcia Campos <cgarcia@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 <cdumez@apple.com>

Regression(r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9
Expand Down
60 changes: 7 additions & 53 deletions Source/WebKit/UIProcess/API/glib/IconDatabase.cpp
View file
Expand Up @@ -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 { }
};

Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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();
});
}

Expand Down
69 changes: 62 additions & 7 deletions Source/WebKit/UIProcess/API/glib/IconDatabase.h
View file
Expand Up @@ -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>

Expand All @@ -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 {
Expand Down Expand Up @@ -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<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();
Expand Down Expand Up @@ -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&);
Expand All @@ -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<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;
Expand All @@ -329,6 +382,8 @@ class IconDatabase {
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

0 comments on commit cb4384a

Please sign in to comment.