From 2956e5494d108664074e63467cfb7281f51474d0 Mon Sep 17 00:00:00 2001 From: Victor Porof Date: Fri, 24 Jul 2020 13:53:02 +0000 Subject: [PATCH] Bug 1597898 - Part 3: Update kvstore to use RKV in safe mode, r=nanj Differential Revision: https://phabricator.services.mozilla.com/D54282 --- toolkit/components/kvstore/src/error.rs | 2 +- toolkit/components/kvstore/src/lib.rs | 9 +-- toolkit/components/kvstore/src/manager.rs | 76 ------------------- toolkit/components/kvstore/src/task.rs | 41 +++++----- .../kvstore/test/xpcshell/test_kvstore.js | 27 +------ 5 files changed, 30 insertions(+), 125 deletions(-) delete mode 100644 toolkit/components/kvstore/src/manager.rs diff --git a/toolkit/components/kvstore/src/error.rs b/toolkit/components/kvstore/src/error.rs index 726a19c75607..012237356a59 100644 --- a/toolkit/components/kvstore/src/error.rs +++ b/toolkit/components/kvstore/src/error.rs @@ -7,7 +7,7 @@ use nserror::{ NS_ERROR_NULL_POINTER, NS_ERROR_UNEXPECTED, }; use nsstring::nsCString; -use rkv::{migrate::MigrateError, StoreError}; +use rkv::{MigrateError, StoreError}; use std::{io::Error as IoError, str::Utf8Error, string::FromUtf16Error, sync::PoisonError}; #[derive(Debug, Fail)] diff --git a/toolkit/components/kvstore/src/lib.rs b/toolkit/components/kvstore/src/lib.rs index 4619ffe9f749..3723d9e910dd 100644 --- a/toolkit/components/kvstore/src/lib.rs +++ b/toolkit/components/kvstore/src/lib.rs @@ -8,10 +8,7 @@ extern crate crossbeam_utils; extern crate cstr; #[macro_use] extern crate failure; -#[macro_use] -extern crate lazy_static; extern crate libc; -extern crate lmdb; extern crate log; extern crate moz_task; extern crate nserror; @@ -23,7 +20,6 @@ extern crate thin_vec; extern crate xpcom; mod error; -mod manager; mod owned_value; mod task; @@ -37,7 +33,8 @@ use moz_task::{ use nserror::{nsresult, NS_ERROR_FAILURE, NS_ERROR_NO_AGGREGATION, NS_OK}; use nsstring::{nsACString, nsCString}; use owned_value::{owned_to_variant, variant_to_owned}; -use rkv::{OwnedValue, Rkv, SingleStore}; +use rkv::backend::{SafeModeDatabase, SafeModeEnvironment}; +use rkv::OwnedValue; use std::{ ptr, sync::{Arc, RwLock}, @@ -57,6 +54,8 @@ use xpcom::{ nsIID, xpcom, xpcom_method, RefPtr, }; +type Rkv = rkv::Rkv; +type SingleStore = rkv::SingleStore; type KeyValuePairResult = Result<(String, OwnedValue), KeyValueError>; #[no_mangle] diff --git a/toolkit/components/kvstore/src/manager.rs b/toolkit/components/kvstore/src/manager.rs deleted file mode 100644 index d96916af5099..000000000000 --- a/toolkit/components/kvstore/src/manager.rs +++ /dev/null @@ -1,76 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ - -//! A manager of handles to Rkv environments that ensures that multiple calls -//! to `nsIKeyValueService.get_or_create()` with the same path reuse the same -//! environment handle. -//! -//! The Rkv crate itself provides this functionality, but it calls -//! `Path.canonicalize()` on the path, which crashes in Firefox on Android -//! because of https://bugzilla.mozilla.org/show_bug.cgi?id=1531887. -//! -//! Since kvstore consumers use canonical paths in practice, this manager -//! works around that bug by not doing so itself. - -use error::KeyValueError; -use lmdb::Error as LmdbError; -use rkv::{migrate::Migrator, Rkv, StoreError}; -use std::collections::hash_map::Entry; -use std::collections::HashMap; -use std::fs::copy; -use std::path::{Path, PathBuf}; -use std::sync::{Arc, RwLock}; -use tempfile::tempdir; - -lazy_static! { - static ref MANAGER: RwLock = RwLock::new(Manager::new()); -} - -pub(crate) struct Manager { - environments: HashMap>>, -} - -impl Manager { - fn new() -> Manager { - Manager { - environments: Default::default(), - } - } - - pub(crate) fn singleton() -> &'static RwLock { - &*MANAGER - } - - /// Return the open env at `path`, or create it by calling `f`. - pub fn get_or_create<'p, F, P>( - &mut self, - path: P, - f: F, - ) -> Result>, KeyValueError> - where - F: Fn(&Path) -> Result, - P: Into<&'p Path>, - { - let path = path.into(); - Ok(match self.environments.entry(path.to_path_buf()) { - Entry::Occupied(e) => e.get().clone(), - Entry::Vacant(e) => { - let env = match f(e.key().as_path()) { - Ok(env) => env, - Err(StoreError::LmdbError(LmdbError::Invalid)) => { - let temp_env = tempdir()?; - let mut migrator = Migrator::new(path)?; - migrator.migrate(temp_env.path())?; - copy(temp_env.path().join("data.mdb"), path.join("data.mdb"))?; - copy(temp_env.path().join("lock.mdb"), path.join("lock.mdb"))?; - f(e.key().as_path())? - } - Err(err) => return Err(err.into()), - }; - let k = Arc::new(RwLock::new(env)); - e.insert(k).clone() - } - }) - } -} diff --git a/toolkit/components/kvstore/src/task.rs b/toolkit/components/kvstore/src/task.rs index d67329d9bf95..a6f8039eb82d 100644 --- a/toolkit/components/kvstore/src/task.rs +++ b/toolkit/components/kvstore/src/task.rs @@ -6,12 +6,12 @@ extern crate xpcom; use crossbeam_utils::atomic::AtomicCell; use error::KeyValueError; -use manager::Manager; use moz_task::Task; use nserror::{nsresult, NS_ERROR_FAILURE}; use nsstring::nsCString; use owned_value::owned_to_variant; -use rkv::{OwnedValue, Rkv, SingleStore, StoreError, StoreOptions, Value}; +use rkv::backend::{BackendInfo, SafeMode, SafeModeDatabase, SafeModeEnvironment}; +use rkv::{Migrator, OwnedValue, StoreError, StoreOptions, Value}; use std::{ path::Path, str, @@ -29,6 +29,10 @@ use KeyValueDatabase; use KeyValueEnumerator; use KeyValuePairResult; +type Manager = rkv::Manager; +type Rkv = rkv::Rkv; +type SingleStore = rkv::SingleStore; + /// A macro to generate a done() implementation for a Task. /// Takes one argument that specifies the type of the Task's callback function: /// value: a callback function that takes a value @@ -98,14 +102,14 @@ const INCREMENTAL_RESIZE_THRESHOLD: usize = 52_428_800; /// The incremental resize step (5 MB) const INCREMENTAL_RESIZE_STEP: usize = 5_242_880; -/// The LMDB disk page size and mask. +/// The RKV disk page size and mask. const PAGE_SIZE: usize = 4096; const PAGE_SIZE_MASK: usize = 0b_1111_1111_1111; /// Round the non-zero size to the multiple of page size greater or equal. /// /// It does not handle the special cases such as size zero and overflow, -/// because even if that happens (extremely unlikely though), LMDB will +/// because even if that happens (extremely unlikely though), RKV will /// ignore the new size if it's smaller than the current size. /// /// E.g: @@ -190,11 +194,15 @@ impl Task for GetOrCreateTask { self.result .store(Some(|| -> Result { let store; - let mut writer = Manager::singleton().write()?; - let rkv = writer.get_or_create(Path::new(str::from_utf8(&self.path)?), Rkv::new)?; + let mut manager = Manager::singleton().write()?; + // Note that path canonicalization is diabled to work around crashes on Fennec: + // https://bugzilla.mozilla.org/show_bug.cgi?id=1531887 + let path = Path::new(str::from_utf8(&self.path)?); + let rkv = manager.get_or_create(path, Rkv::new::)?; + Migrator::auto_migrate_lmdb_to_safe_mode(path, |builder| builder, rkv.read()?)?; { let env = rkv.read()?; - let load_ratio = env.load_ratio()?; + let load_ratio = env.load_ratio()?.unwrap_or(0.0); if load_ratio > RESIZE_RATIO { active_resize(&env)?; } @@ -246,7 +254,7 @@ impl Task for PutTask { let mut resized = false; // Use a loop here in case we want to retry from a recoverable - // error such as `lmdb::Error::MapFull`. + // error such as `StoreError::MapFull`. loop { let mut writer = env.write()?; @@ -255,7 +263,7 @@ impl Task for PutTask { // Only handle the first MapFull error via passive resizing. // Propogate the subsequent MapFull error. - Err(StoreError::LmdbError(lmdb::Error::MapFull)) if !resized => { + Err(StoreError::MapFull) if !resized => { // abort the failed transaction for resizing. writer.abort(); @@ -331,7 +339,7 @@ impl Task for WriteManyTask { let mut resized = false; // Use a loop here in case we want to retry from a recoverable - // error such as `lmdb::Error::MapFull`. + // error such as `StoreError::MapFull`. 'outer: loop { let mut writer = env.write()?; @@ -345,7 +353,7 @@ impl Task for WriteManyTask { // Only handle the first MapFull error via passive resizing. // Propogate the subsequent MapFull error. - Err(StoreError::LmdbError(lmdb::Error::MapFull)) if !resized => { + Err(StoreError::MapFull) if !resized => { // Abort the failed transaction for resizing. writer.abort(); @@ -365,10 +373,10 @@ impl Task for WriteManyTask { match self.store.delete(&mut writer, key) { Ok(_) => (), - // LMDB fails with an error if the key to delete wasn't found, + // RKV fails with an error if the key to delete wasn't found, // and Rkv returns that error, but we ignore it, as we expect most // of our consumers to want this behavior. - Err(StoreError::LmdbError(lmdb::Error::NotFound)) => (), + Err(StoreError::KeyValuePairNotFound) => (), Err(err) => return Err(KeyValueError::StoreError(err)), }; @@ -528,10 +536,10 @@ impl Task for DeleteTask { match self.store.delete(&mut writer, key) { Ok(_) => (), - // LMDB fails with an error if the key to delete wasn't found, + // RKV fails with an error if the key to delete wasn't found, // and Rkv returns that error, but we ignore it, as we expect most // of our consumers to want this behavior. - Err(StoreError::LmdbError(lmdb::Error::NotFound)) => (), + Err(StoreError::KeyValuePairNotFound) => (), Err(err) => return Err(KeyValueError::StoreError(err)), }; @@ -672,9 +680,8 @@ impl Task for EnumerateTask { // Convert the key/value pair to owned. .map(|result| match result { Ok((key, val)) => match (key, val) { - (Ok(key), Some(val)) => Ok((key.to_owned(), OwnedValue::from(&val))), + (Ok(key), val) => Ok((key.to_owned(), OwnedValue::from(&val))), (Err(err), _) => Err(err.into()), - (_, None) => Err(KeyValueError::UnexpectedValue), }, Err(err) => Err(KeyValueError::StoreError(err)), }) diff --git a/toolkit/components/kvstore/test/xpcshell/test_kvstore.js b/toolkit/components/kvstore/test/xpcshell/test_kvstore.js index 55716e6a9cd8..c7defd229428 100644 --- a/toolkit/components/kvstore/test/xpcshell/test_kvstore.js +++ b/toolkit/components/kvstore/test/xpcshell/test_kvstore.js @@ -40,7 +40,7 @@ add_task(async function getOrCreate() { ); await Assert.rejects( KeyValueService.getOrCreate(nonexistentDir, "db"), - /DirectoryDoesNotExistError/ + /UnsuitableEnvironmentPath/ ); // Test creating a database with a non-normalized but fully-qualified path. @@ -592,28 +592,3 @@ add_task(async function enumeration() { await database.delete("string-key"); await database.delete("bool-key"); }); - -add_task(async function migration() { - const currentDir = await OS.File.getCurrentDirectory(); - const databaseDir = await makeDatabaseDir("migration"); - - // We're testing migration from a different architecture to our own, - // so we choose the 32-bit database if we're a 64-bit build, and vice-versa. - const testEnvDir = Services.appinfo.is64Bit ? "test-env-32" : "test-env-64"; - - await OS.File.copy( - OS.Path.join(currentDir, "data", testEnvDir, "data.mdb"), - OS.Path.join(databaseDir, "data.mdb") - ); - await OS.File.copy( - OS.Path.join(currentDir, "data", testEnvDir, "lock.mdb"), - OS.Path.join(databaseDir, "lock.mdb") - ); - - const database = await KeyValueService.getOrCreate(databaseDir, "db"); - - Assert.strictEqual(await database.get("int-key"), 1234); - Assert.strictEqual(await database.get("double-key"), 56.78); - Assert.strictEqual(await database.get("string-key"), "Héllo, wőrld!"); - Assert.strictEqual(await database.get("bool-key"), true); -});