Skip to content

Commit

Permalink
Crash under SQLiteDatabase::~SQLiteDatabase
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=269648
rdar://123160407

Reviewed by David Kilzer.

The crash was occurring because SQLiteStorageArea::handleDatabaseCorruptionIfNeeded()
was destroying the SQLiteDatabase object (m_database) but was failing to destroy the
potential transaction (m_transaction) and cached statements (m_cachedStatements), all
of which have a CheckedRef pointing to the database.

Update handleDatabaseCorruptionIfNeeded() to call close(), which clears m_cache,
m_cacheSize, m_transaction, m_cachedStatements and m_database.

Also update SQLiteStatement / SQLiteTransaction to keep a WeakRef to the database
instead of a CheckedRef, as per our updated smart pointer guidelines. Using
CheckedPtr/CheckedRef for database members is no longer recommended.

* Source/WebCore/platform/sql/SQLiteDatabase.h:
* Source/WebCore/platform/sql/SQLiteStatement.h:
* Source/WebCore/platform/sql/SQLiteTransaction.h:
* Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:
(WebKit::SQLiteStorageArea::handleDatabaseCorruptionIfNeeded):

Canonical link: https://commits.webkit.org/274937@main
  • Loading branch information
cdumez committed Feb 18, 2024
1 parent 345a315 commit f6305e2
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 9 deletions.
5 changes: 3 additions & 2 deletions Source/WebCore/platform/sql/SQLiteDatabase.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@

#include <functional>
#include <sqlite3.h>
#include <wtf/CheckedRef.h>
#include <wtf/CheckedPtr.h>
#include <wtf/Expected.h>
#include <wtf/Lock.h>
#include <wtf/OptionSet.h>
#include <wtf/Threading.h>
#include <wtf/UniqueRef.h>
#include <wtf/WeakPtr.h>
#include <wtf/text/CString.h>
#include <wtf/text/WTFString.h>

Expand All @@ -49,7 +50,7 @@ class DatabaseAuthorizer;
class SQLiteStatement;
class SQLiteTransaction;

class SQLiteDatabase : public CanMakeThreadSafeCheckedPtr {
class SQLiteDatabase : public CanMakeWeakPtr<SQLiteDatabase>, public CanMakeThreadSafeCheckedPtr {
WTF_MAKE_FAST_ALLOCATED;
WTF_MAKE_NONCOPYABLE(SQLiteDatabase);
friend class SQLiteTransaction;
Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/platform/sql/SQLiteStatement.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#include "SQLValue.h"
#include "SQLiteDatabase.h"
#include <span>
#include <wtf/CheckedRef.h>
#include <wtf/WeakRef.h>

struct sqlite3_stmt;

Expand Down Expand Up @@ -93,7 +93,7 @@ class SQLiteStatement {
template<typename T, typename... Args> bool bindImpl(int i, T first, Args&&... args);
template<typename T> bool bindImpl(int, T);

CheckedRef<SQLiteDatabase> m_database;
WeakRef<SQLiteDatabase> m_database;
sqlite3_stmt* m_statement;
};

Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/platform/sql/SQLiteTransaction.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
#ifndef SQLiteTransaction_h
#define SQLiteTransaction_h

#include <wtf/CheckedRef.h>
#include <wtf/FastMalloc.h>
#include <wtf/Noncopyable.h>
#include <wtf/WeakRef.h>

namespace WebCore {

Expand All @@ -51,7 +51,7 @@ class SQLiteTransaction {
SQLiteDatabase& database() const { return m_db.get(); }

private:
CheckedRef<SQLiteDatabase> m_db;
WeakRef<SQLiteDatabase> m_db;
bool m_inProgress;
bool m_readOnly;
};
Expand Down
5 changes: 2 additions & 3 deletions Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -455,9 +455,8 @@ bool SQLiteStorageArea::handleDatabaseCorruptionIfNeeded(int databaseError)
if (databaseError != SQLITE_CORRUPT && databaseError != SQLITE_NOTADB)
return false;

m_database = nullptr;
m_cache = std::nullopt;
m_cacheSize = std::nullopt;
close();

RELEASE_LOG(Storage, "SQLiteStorageArea::handleDatabaseCorruption deletes corrupted database file '%s'", m_path.utf8().data());
WebCore::SQLiteFileSystem::deleteDatabaseFile(m_path);
return true;
Expand Down

0 comments on commit f6305e2

Please sign in to comment.