nagai/fune
3
0
Fork 0
forked from mirrors/gecko-dev
Code Pull requests Activity

Bug 1650201 - Fix mozStorage prefs read before profile and fallback to a non-exclusive VFS when it can't get an exclusive lock. r=asuth,geckoview-reviewers,agi

Browse source 
mozStorage used to read prefs on service init, because they could only be read
on the main-thread. When service init was moved earlier, it started trying
to read prefs too early, before the profile was set up, thus it ended up always
reading the default value.

This patch moves the only relevant pref to mirrored StaticPrefs that can be accessed
from different threads, and removes two preferences that apparently are not necessary
(they have been broken from a long time) for now.
In particular, providing a global synchronous setting is a footgun, each consumer should
decide about their synchronous needs, rather than abusing a dangerous "go fast" setting.
The page size is something we don't change from quite some time, and it's unlikely to be
used to run experiments in the wild before doing local measurements first, for which Try
builds are enough.

The remaining exclusiveLock pref is a bit controversial, because in general exclusive lock
is better for various reasons, and mostly it is necessary to use WAL on network shares.
Though developers may find it useful for debugging, and some third parties are doing
dangerous things (like copying over databases) to work around it, for which it's safer to
provide a less dangerous alternative.
Note exclusive lock only works on Unix-derived systems for now (no Windows implementation).

Finally, this introduces a fallback to exclusive lock, so that if a third party is using our
databases, so that we can't get an exclusive lock, we'll fallback to normal locking.

Differential Revision: https://phabricator.services.mozilla.com/D82717
This commit is contained in:
Marco Bonardo 2020-07-10 21:45:53 +00:00
parent 470d1dde92
commit 14784dc42d
9 changed files with 130 additions and 136 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

7
mobile/android/app/mobile.js
View file

@ -51,13 +51,6 @@ pref("browser.tabs.useCache", false);
pref("toolkit.zoomManager.zoomValues", ".2,.3,.5,.67,.8,.9,1,1.1,1.2,1.33,1.5,1.7,2,2.4,3,4");
// Mobile will use faster, less durable mode.
pref("toolkit.storage.synchronous", 0);
// Android needs concurrent access to the same database from multiple processes,
// thus we can't use exclusive locking on it.
pref("storage.multiProcessAccess.enabled", true);
// The default fallback zoom level to render pages at. Set to -1 to fit page; otherwise
// the value is divided by 1000 and clamped to hard-coded min/max scale values.
pref("browser.viewport.defaultZoom", -1);

22
modules/libpref/init/StaticPrefList.yaml
View file

@ -8998,6 +8998,28 @@
#endif
mirror: once
#---------------------------------------------------------------------------
# Prefs starting with "storage."
#---------------------------------------------------------------------------
# Whether to use a non-exclusive VFS.
# By default we use the unix-excl VFS, for the following reasons:
# 1. It improves compatibility with NFS shares, whose implementation
# is incompatible with SQLite's locking requirements (reliable fcntl), and
# in particular with WAL journaling.
# Bug 433129 attempted to automatically identify such file-systems,
# but a reliable way was not found and the fallback locking is slower than
# POSIX locking, so we do not want to do it by default.
# 2. It allows wal mode to avoid the memory mapped -shm file, reducing the
# likelihood of SIGBUS failures when disk space is exhausted.
# 3. It provides some protection from third party database tampering while a
# connection is open.
# Note there's no win32-excl VFS, so this has no effect on Windows.
- name: storage.sqlite.exclusiveLock.enabled
type: RelaxedAtomicBool
value: @IS_NOT_ANDROID@
mirror: always
#---------------------------------------------------------------------------
# Prefs starting with "svg."
#---------------------------------------------------------------------------

1
modules/libpref/moz.build
View file

@ -74,6 +74,7 @@ pref_groups = [
'prompts',
'security',
'slider',
'storage',
'svg',
'telemetry',
'test',

29
storage/TelemetryVFS.cpp
View file

@ -6,7 +6,6 @@
#include <string.h>
#include "mozilla/Telemetry.h"
#include "mozilla/Preferences.h"
#include "sqlite3.h"
#include "nsThreadUtils.h"
#include "mozilla/dom/quota/PersistenceType.h"
@ -15,6 +14,7 @@
#include "mozilla/net/IOActivityMonitor.h"
#include "mozilla/IOInterposer.h"
#include "nsEscape.h"
#include "mozilla/StaticPrefs_storage.h"
#ifdef XP_WIN
# include "mozilla/StaticPrefs_dom.h"
@ -26,23 +26,6 @@
// The last io_methods version for which this file has been updated.
#define LAST_KNOWN_IOMETHODS_VERSION 3
/**
* By default use the unix-excl VFS, for the following reasons:
* 1. It improves compatibility with NFS shares, whose implementation
* is incompatible with SQLite's locking requirements.
* Bug 433129 attempted to automatically identify such file-systems,
* but a reliable way was not found and the fallback locking is slower than
* POSIX locking, so we do not want to do it by default.
* 2. It allows wal mode to avoid the memory mapped -shm file, reducing the
* likelihood of SIGBUS failures when disk space is exhausted.
* 3. It provides some protection from third party database tampering while a
* connection is open.
* This preference allows to revert to the "unix" VFS, that is not exclusive,
* thus it can be used by developers to query a database through the Sqlite
* command line while it's already in use.
*/
#define PREF_MULTI_PROCESS_ACCESS "storage.multiProcessAccess.enabled"
namespace {
using namespace mozilla;
@ -681,9 +664,11 @@ static const char* xNextSystemCall(sqlite3_vfs* vfs, const char* zName) {
namespace mozilla {
namespace storage {
const char* GetVFSName() { return "telemetry-vfs"; }
const char* GetVFSName(bool exclusive) {
return exclusive ? "telemetry-vfs-excl" : "telemetry-vfs";
}
sqlite3_vfs* ConstructTelemetryVFS() {
sqlite3_vfs* ConstructTelemetryVFS(bool exclusive) {
#if defined(XP_WIN)
# define EXPECTED_VFS "win32"
# define EXPECTED_VFS_EXCL "win32"
@ -694,7 +679,7 @@ sqlite3_vfs* ConstructTelemetryVFS() {
bool expected_vfs;
sqlite3_vfs* vfs;
if (Preferences::GetBool(PREF_MULTI_PROCESS_ACCESS, false)) {
if (!exclusive) {
// Use the non-exclusive VFS.
vfs = sqlite3_vfs_find(nullptr);
expected_vfs = vfs->zName && !strcmp(vfs->zName, EXPECTED_VFS);
@ -716,7 +701,7 @@ sqlite3_vfs* ConstructTelemetryVFS() {
tvfs->szOsFile =
sizeof(telemetry_file) - sizeof(sqlite3_file) + vfs->szOsFile;
tvfs->mxPathname = vfs->mxPathname;
tvfs->zName = GetVFSName();
tvfs->zName = GetVFSName(exclusive);
tvfs->pAppData = vfs;
tvfs->xOpen = xOpen;
tvfs->xDelete = xDelete;

3
storage/VacuumManager.cpp
View file

@ -15,6 +15,7 @@
#include "nsThreadUtils.h"
#include "mozilla/Logging.h"
#include "prtime.h"
#include "mozilla/StaticPrefs_storage.h"
#include "mozStorageConnection.h"
#include "mozIStorageStatement.h"
@ -137,7 +138,7 @@ bool Vacuumer::execute() {
if (NS_FAILED(rv) || !Service::pageSizeIsValid(expectedPageSize)) {
NS_WARNING("Invalid page size requested for database, will use default ");
NS_WARNING(mDBFilename.get());
expectedPageSize = Service::getDefaultPageSize();
expectedPageSize = Service::kDefaultPageSize;
}
// Get the database filename. Last vacuum time is stored under this name

121
storage/mozStorageConnection.cpp
View file

@ -19,6 +19,7 @@
#include "mozilla/Unused.h"
#include "mozilla/dom/quota/QuotaObject.h"
#include "mozilla/ScopeExit.h"
#include "mozilla/StaticPrefs_storage.h"
#include "mozIStorageCompletionCallback.h"
#include "mozIStorageFunction.h"
@ -72,7 +73,7 @@ namespace mozilla::storage {
using mozilla::dom::quota::QuotaObject;
const char* GetVFSName();
const char* GetVFSName(bool);
namespace {
@ -565,7 +566,7 @@ nsresult Connection::initialize() {
AUTO_PROFILER_LABEL("Connection::initialize", OTHER);
// in memory database requested, sqlite uses a magic file name
int srv = ::sqlite3_open_v2(":memory:", &mDBConn, mFlags, GetVFSName());
int srv = ::sqlite3_open_v2(":memory:", &mDBConn, mFlags, GetVFSName(true));
if (srv != SQLITE_OK) {
mDBConn = nullptr;
return convertResultCode(srv);
@ -593,6 +594,8 @@ nsresult Connection::initialize(nsIFile* aDatabaseFile) {
"Initialize called on already opened database!");
AUTO_PROFILER_LABEL("Connection::initialize", OTHER);
// Do not set mFileURL here since this is database does not have an associated
// URL.
mDatabaseFile = aDatabaseFile;
nsAutoString path;
@ -604,27 +607,41 @@ nsresult Connection::initialize(nsIFile* aDatabaseFile) {
#else
static const char* sIgnoreLockingVFS = "unix-none";
#endif
const char* vfs = mIgnoreLockingMode ? sIgnoreLockingVFS : GetVFSName();
int srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn,
mFlags, vfs);
bool exclusive = StaticPrefs::storage_sqlite_exclusiveLock_enabled();
int srv;
if (mIgnoreLockingMode) {
exclusive = false;
srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn, mFlags,
sIgnoreLockingVFS);
} else {
srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn, mFlags,
GetVFSName(exclusive));
if (exclusive && (srv == SQLITE_LOCKED || srv == SQLITE_BUSY)) {
// Retry without trying to get an exclusive lock.
exclusive = false;
srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn,
mFlags, GetVFSName(false));
}
}
if (srv != SQLITE_OK) {
mDBConn = nullptr;
return convertResultCode(srv);
}
#ifdef MOZ_SQLITE_FTS3_TOKENIZER
srv =
::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, 1, 0);
MOZ_ASSERT(srv == SQLITE_OK,
"SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER should be enabled");
#endif
// Do not set mFileURL here since this is database does not have an associated
// URL.
mDatabaseFile = aDatabaseFile;
rv = initializeInternal();
if (exclusive &&
(rv == NS_ERROR_STORAGE_BUSY || rv == NS_ERROR_FILE_IS_LOCKED)) {
// Usually SQLite will fail to acquire an exclusive lock on opening, but in
// some cases it may successfully open the database and then lock on the
// first query execution. When initializeInternal fails it closes the
// connection, so we can try to restart it in non-exclusive mode.
srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn, mFlags,
GetVFSName(false));
if (srv == SQLITE_OK) {
rv = initializeInternal();
}
}
NS_ENSURE_SUCCESS(rv, rv);
return NS_OK;
@ -640,28 +657,40 @@ nsresult Connection::initialize(nsIFileURL* aFileURL) {
nsresult rv = aFileURL->GetFile(getter_AddRefs(databaseFile));
NS_ENSURE_SUCCESS(rv, rv);
// Set both mDatabaseFile and mFileURL here.
mFileURL = aFileURL;
mDatabaseFile = databaseFile;
nsAutoCString spec;
rv = aFileURL->GetSpec(spec);
NS_ENSURE_SUCCESS(rv, rv);
int srv = ::sqlite3_open_v2(spec.get(), &mDBConn, mFlags, GetVFSName());
bool exclusive = StaticPrefs::storage_sqlite_exclusiveLock_enabled();
int srv =
::sqlite3_open_v2(spec.get(), &mDBConn, mFlags, GetVFSName(exclusive));
if (exclusive && (srv == SQLITE_LOCKED || srv == SQLITE_BUSY)) {
// Retry without trying to get an exclusive lock.
exclusive = false;
srv = ::sqlite3_open_v2(spec.get(), &mDBConn, mFlags, GetVFSName(false));
}
if (srv != SQLITE_OK) {
mDBConn = nullptr;
return convertResultCode(srv);
}
#ifdef MOZ_SQLITE_FTS3_TOKENIZER
srv =
::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, 1, 0);
MOZ_ASSERT(srv == SQLITE_OK,
"SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER should be enabled");
#endif
// Set both mDatabaseFile and mFileURL here.
mFileURL = aFileURL;
mDatabaseFile = databaseFile;
rv = initializeInternal();
if (exclusive &&
(rv == NS_ERROR_STORAGE_BUSY || rv == NS_ERROR_FILE_IS_LOCKED)) {
// Usually SQLite will fail to acquire an exclusive lock on opening, but in
// some cases it may successfully open the database and then lock on the
// first query execution. When initializeInternal fails it closes the
// connection, so we can try to restart it in non-exclusive mode.
srv = ::sqlite3_open_v2(spec.get(), &mDBConn, mFlags, GetVFSName(false));
if (srv == SQLITE_OK) {
rv = initializeInternal();
}
}
NS_ENSURE_SUCCESS(rv, rv);
return NS_OK;
@ -669,9 +698,17 @@ nsresult Connection::initialize(nsIFileURL* aFileURL) {
nsresult Connection::initializeInternal() {
MOZ_ASSERT(mDBConn);
auto guard = MakeScopeExit([&]() { initializeFailed(); });
mConnectionClosed = false;
#ifdef MOZ_SQLITE_FTS3_TOKENIZER
DebugOnly<int> srv =
::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER, 1, 0);
MOZ_ASSERT(srv == SQLITE_OK,
"SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER should be enabled");
#endif
if (mFileURL) {
const char* dbPath = ::sqlite3_db_filename(mDBConn, "main");
MOZ_ASSERT(dbPath);
@ -705,7 +742,7 @@ nsresult Connection::initializeInternal() {
("Opening connection to '%s' (%p)", mTelemetryFilename.get(), this));
}
int64_t pageSize = Service::getDefaultPageSize();
int64_t pageSize = Service::kDefaultPageSize;
// Set page_size to the preferred default value. This is effective only if
// the database has just been created, otherwise, if the database does not
@ -741,19 +778,15 @@ nsresult Connection::initializeInternal() {
return convertResultCode(srv);
}
// Set the synchronous PRAGMA, according to the preference.
switch (Service::getSynchronousPref()) {
case 2:
(void)ExecuteSimpleSQL("PRAGMA synchronous = FULL;"_ns);
break;
case 0:
(void)ExecuteSimpleSQL("PRAGMA synchronous = OFF;"_ns);
break;
case 1:
default:
(void)ExecuteSimpleSQL("PRAGMA synchronous = NORMAL;"_ns);
break;
}
// Set the default synchronous value. Each consumer can switch this
// accordingly to their needs.
#if defined(ANDROID)
// Android prefers synchronous = OFF for performance reasons.
Unused << ExecuteSimpleSQL("PRAGMA synchronous = OFF;"_ns);
#else
// Normal is the suggested value for WAL journals.
Unused << ExecuteSimpleSQL("PRAGMA synchronous = NORMAL;"_ns);
#endif
// Initialization succeeded, we can stop guarding for failures.
guard.release();
@ -1601,7 +1634,7 @@ Connection::Interrupt() {
NS_IMETHODIMP
Connection::GetDefaultPageSize(int32_t* _defaultPageSize) {
*_defaultPageSize = Service::getDefaultPageSize();
*_defaultPageSize = Service::kDefaultPageSize;
return NS_OK;
}

56
storage/mozStorageService.cpp
View file

@ -18,7 +18,6 @@
#include "nsIObserverService.h"
#include "nsIPropertyBag2.h"
#include "mozilla/Services.h"
#include "mozilla/Preferences.h"
#include "mozilla/LateWriteChecks.h"
#include "mozIStorageCompletionCallback.h"
#include "mozIStoragePendingStatement.h"
@ -31,18 +30,6 @@
# undef CompareString
#endif
////////////////////////////////////////////////////////////////////////////////
//// Defines
#define PREF_TS_SYNCHRONOUS "toolkit.storage.synchronous"
#define PREF_TS_SYNCHRONOUS_DEFAULT 1
#define PREF_TS_PAGESIZE "toolkit.storage.pageSize"
// This value must be kept in sync with the value of SQLITE_DEFAULT_PAGE_SIZE in
// third_party/sqlite3/src/Makefile.in.
#define PREF_TS_PAGESIZE_DEFAULT 32768
namespace mozilla {
namespace storage {
@ -188,15 +175,9 @@ already_AddRefed<Service> Service::getSingleton() {
return nullptr;
}
int32_t Service::sSynchronousPref;
// static
int32_t Service::getSynchronousPref() { return sSynchronousPref; }
int32_t Service::sDefaultPageSize = PREF_TS_PAGESIZE_DEFAULT;
Service::Service()
: mMutex("Service::mMutex"),
mSqliteExclVFS(nullptr),
mSqliteVFS(nullptr),
mRegistrationMutex("Service::mRegistrationMutex"),
mConnections() {}
@ -205,12 +186,16 @@ Service::~Service() {
mozilla::UnregisterWeakMemoryReporter(this);
mozilla::UnregisterStorageSQLiteDistinguishedAmount();
int rc = sqlite3_vfs_unregister(mSqliteVFS);
if (rc != SQLITE_OK) NS_WARNING("Failed to unregister sqlite vfs wrapper.");
int srv = sqlite3_vfs_unregister(mSqliteVFS);
if (srv != SQLITE_OK) NS_WARNING("Failed to unregister sqlite vfs wrapper.");
srv = sqlite3_vfs_unregister(mSqliteExclVFS);
if (srv != SQLITE_OK) NS_WARNING("Failed to unregister sqlite vfs wrapper.");
gService = nullptr;
delete mSqliteVFS;
mSqliteVFS = nullptr;
delete mSqliteExclVFS;
mSqliteExclVFS = nullptr;
}
void Service::registerConnection(Connection* aConnection) {
@ -309,8 +294,8 @@ void Service::minimizeMemory() {
}
}
sqlite3_vfs* ConstructTelemetryVFS();
const char* GetVFSName();
sqlite3_vfs* ConstructTelemetryVFS(bool);
const char* GetVFSName(bool);
static const char* sObserverTopics[] = {"memory-pressure",
"xpcom-shutdown-threads"};
@ -321,12 +306,17 @@ nsresult Service::initialize() {
int rc = AutoSQLiteLifetime::getInitResult();
if (rc != SQLITE_OK) return convertResultCode(rc);
mSqliteVFS = ConstructTelemetryVFS();
mSqliteVFS = ConstructTelemetryVFS(false);
MOZ_ASSERT(mSqliteVFS, "Non-exclusive VFS should be created");
if (mSqliteVFS) {
rc = sqlite3_vfs_register(mSqliteVFS, 0);
if (rc != SQLITE_OK) return convertResultCode(rc);
} else {
NS_WARNING("Failed to register telemetry VFS");
}
mSqliteExclVFS = ConstructTelemetryVFS(true);
MOZ_ASSERT(mSqliteExclVFS, "Exclusive VFS should be created");
if (mSqliteExclVFS) {
rc = sqlite3_vfs_register(mSqliteExclVFS, 0);
if (rc != SQLITE_OK) return convertResultCode(rc);
}
nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
@ -339,18 +329,6 @@ nsresult Service::initialize() {
}
}
// We need to obtain the toolkit.storage.synchronous preferences on the main
// thread because the preference service can only be accessed there. This
// is cached in the service for all future Open[Unshared]Database calls.
sSynchronousPref =
Preferences::GetInt(PREF_TS_SYNCHRONOUS, PREF_TS_SYNCHRONOUS_DEFAULT);
// We need to obtain the toolkit.storage.pageSize preferences on the main
// thread because the preference service can only be accessed there. This
// is cached in the service for all future Open[Unshared]Database calls.
sDefaultPageSize =
Preferences::GetInt(PREF_TS_PAGESIZE, PREF_TS_PAGESIZE_DEFAULT);
mozilla::RegisterWeakMemoryReporter(this);
mozilla::RegisterStorageSQLiteDistinguishedAmount(
StorageSQLiteDistinguishedAmount);

18
storage/mozStorageService.h
View file

@ -56,18 +56,6 @@ class Service : public mozIStorageService,
NS_DECL_NSIOBSERVER
NS_DECL_NSIMEMORYREPORTER
/**
* Obtains the cached data for the toolkit.storage.synchronous preference.
*/
static int32_t getSynchronousPref();
/**
* Obtains the default page size for this platform. The default value is
* specified in the SQLite makefile (SQLITE_DEFAULT_PAGE_SIZE) but it may be
* overriden with the PREF_TS_PAGESIZE hidden preference.
*/
static int32_t getDefaultPageSize() { return sDefaultPageSize; }
/**
* Returns a boolean value indicating whether or not the given page size is
* valid (currently understood as a power of 2 between 512 and 65536).
@ -78,6 +66,8 @@ class Service : public mozIStorageService,
aPageSize == 32768 || aPageSize == 65536;
}
static const int32_t kDefaultPageSize = 32768;
/**
* Registers the connection with the storage service. Connections are
* registered so they can be iterated over.
@ -124,6 +114,7 @@ class Service : public mozIStorageService,
*/
Mutex mMutex;
sqlite3_vfs* mSqliteExclVFS;
sqlite3_vfs* mSqliteVFS;
/**
@ -166,9 +157,6 @@ class Service : public mozIStorageService,
nsCOMPtr<nsIMemoryReporter> mStorageSQLiteReporter;
static Service* gService;
static int32_t sSynchronousPref;
static int32_t sDefaultPageSize;
};
} // namespace storage

9
toolkit/components/contentprefs/ContentPrefService2.jsm
View file

@ -1216,14 +1216,7 @@ ContentPrefService2.prototype = {
// Note: this could cause database corruption if the OS crashes or machine
// loses power before the data gets written to disk, but this is considered
// a reasonable risk for the not-so-critical data stored in this database.
//
// If you really don't want to take this risk, however, just set the
// toolkit.storage.synchronous pref to 1 (NORMAL synchronization) or 2
// (FULL synchronization), in which case mozStorageConnection::Initialize
// will use that value, and we won't override it here.
if (!Services.prefs.prefHasUserValue("toolkit.storage.synchronous")) {
await conn.execute("PRAGMA synchronous = OFF");
}
await conn.execute("PRAGMA synchronous = OFF");
return conn;
},