Bug 1542888 - avoid Rkv::Manager call to std::fs::canonicalize r=lina
authorMyk Melez <myk@mykzilla.org>
Tue, 09 Apr 2019 03:57:18 +0000
changeset 468470 a978703681f9b7816beeaceaa05694f10e1ce795
parent 468469 c8e85a802c77d13a8dc2f20414a0fd5ab608e04a
child 468471 70d5e0b1c0f874fe3845489c0a721ef143e53e6e
push id35838
push usernerli@mozilla.com
push dateTue, 09 Apr 2019 09:54:40 +0000
treeherdermozilla-central@6efaae3bcd67 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslina
bugs1542888, 1531887
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1542888 - avoid Rkv::Manager call to std::fs::canonicalize r=lina Avoid using Rkv::Manager, which calls std::fs::canonicalize, triggering bug 1531887 in Firefox on Android. Differential Revision: https://phabricator.services.mozilla.com/D26605
Cargo.lock
toolkit/components/kvstore/Cargo.toml
toolkit/components/kvstore/src/lib.rs
toolkit/components/kvstore/src/manager.rs
toolkit/components/kvstore/src/task.rs
toolkit/components/kvstore/test/xpcshell/test_kvstore.js
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1488,16 +1488,17 @@ source = "registry+https://github.com/ru
 
 [[package]]
 name = "kvstore"
 version = "0.1.0"
 dependencies = [
  "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "crossbeam-utils 0.6.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "failure 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "libc 0.2.51 (registry+https://github.com/rust-lang/crates.io-index)",
  "lmdb-rkv 0.11.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "moz_task 0.1.0",
  "nserror 0.1.0",
  "nsstring 0.1.0",
  "rkv 0.9.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "storage_variant 0.1.0",
--- a/toolkit/components/kvstore/Cargo.toml
+++ b/toolkit/components/kvstore/Cargo.toml
@@ -1,16 +1,17 @@
 [package]
 name = "kvstore"
 version = "0.1.0"
 authors = ["Myk Melez <myk@mykzilla.org>"]
 
 [dependencies]
 atomic_refcell = "0.1"
 crossbeam-utils = "0.6.3"
+lazy_static = "1"
 libc = "0.2"
 lmdb-rkv = "0.11.2"
 log = "0.4"
 moz_task = { path = "../../../xpcom/rust/moz_task" }
 nserror = { path = "../../../xpcom/rust/nserror" }
 nsstring = { path = "../../../xpcom/rust/nsstring" }
 rkv = "0.9.4"
 storage_variant = { path = "../../../storage/variant" }
--- a/toolkit/components/kvstore/src/lib.rs
+++ b/toolkit/components/kvstore/src/lib.rs
@@ -1,28 +1,31 @@
 /* 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 http://mozilla.org/MPL/2.0/. */
 
 extern crate atomic_refcell;
 extern crate crossbeam_utils;
 #[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;
 extern crate nsstring;
 extern crate rkv;
 extern crate storage_variant;
 extern crate thin_vec;
 extern crate xpcom;
 
 mod error;
+mod manager;
 mod owned_value;
 mod task;
 
 use atomic_refcell::AtomicRefCell;
 use error::KeyValueError;
 use libc::c_void;
 use moz_task::{create_thread, TaskRunnable};
 use nserror::{nsresult, NS_ERROR_FAILURE, NS_ERROR_NO_AGGREGATION, NS_OK};
new file mode 100644
--- /dev/null
+++ b/toolkit/components/kvstore/src/manager.rs
@@ -0,0 +1,61 @@
+/* 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 rkv::{Rkv, StoreError};
+use std::collections::HashMap;
+use std::collections::hash_map::Entry;
+use std::path::{
+    Path,
+    PathBuf,
+};
+use std::sync::{
+    Arc,
+    RwLock,
+};
+
+lazy_static! {
+    static ref MANAGER: RwLock<Manager> = RwLock::new(Manager::new());
+}
+
+pub(crate) struct Manager {
+    environments: HashMap<PathBuf, Arc<RwLock<Rkv>>>,
+}
+
+impl Manager {
+    fn new() -> Manager {
+        Manager {
+            environments: Default::default(),
+        }
+    }
+
+    pub(crate) fn singleton() -> &'static RwLock<Manager> {
+        &*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<Arc<RwLock<Rkv>>, StoreError>
+    where
+        F: FnOnce(&Path) -> Result<Rkv, StoreError>,
+        P: Into<&'p Path>,
+    {
+        Ok(match self.environments.entry(path.into().to_path_buf()) {
+            Entry::Occupied(e) => e.get().clone(),
+            Entry::Vacant(e) => {
+                let k = Arc::new(RwLock::new(f(e.key().as_path())?));
+                e.insert(k).clone()
+            },
+        })
+    }
+}
--- a/toolkit/components/kvstore/src/task.rs
+++ b/toolkit/components/kvstore/src/task.rs
@@ -5,33 +5,34 @@
 extern crate xpcom;
 
 use crossbeam_utils::atomic::AtomicCell;
 use error::KeyValueError;
 use moz_task::Task;
 use nserror::{nsresult, NS_ERROR_FAILURE};
 use nsstring::nsCString;
 use owned_value::owned_to_variant;
-use rkv::{Manager, OwnedValue, Rkv, SingleStore, StoreError, StoreOptions, Value};
+use rkv::{OwnedValue, Rkv, SingleStore, StoreError, StoreOptions, Value};
 use std::{
     path::Path,
     str,
     sync::{Arc, RwLock},
 };
 use storage_variant::VariantType;
 use xpcom::{
     interfaces::{
         nsIKeyValueDatabaseCallback, nsIKeyValueEnumeratorCallback, nsIKeyValueVariantCallback,
         nsIKeyValueVoidCallback, nsIThread, nsIVariant,
     },
     RefPtr, ThreadBoundRefPtr,
 };
 use KeyValueDatabase;
 use KeyValueEnumerator;
 use KeyValuePairResult;
+use manager::Manager;
 
 /// 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
 ///   void: the callback function doesn't take a value
 ///
 /// The "value" variant calls self.convert() to convert a successful result
 /// into the value to pass to the callback function.  So if you generate done()
--- a/toolkit/components/kvstore/test/xpcshell/test_kvstore.js
+++ b/toolkit/components/kvstore/test/xpcshell/test_kvstore.js
@@ -23,16 +23,25 @@ const gKeyValueService =
 add_task(async function getService() {
   Assert.ok(gKeyValueService);
 });
 
 add_task(async function getOrCreate() {
   const databaseDir = await makeDatabaseDir("getOrCreate");
   const database = await KeyValueService.getOrCreate(databaseDir, "db");
   Assert.ok(database);
+
+  // Test creating a database with a nonexistent path.
+  const nonexistentDir = OS.Path.join(OS.Constants.Path.profileDir, "nonexistent");
+  await Assert.rejects(KeyValueService.getOrCreate(nonexistentDir, "db"), /DirectoryDoesNotExistError/);
+
+  // Test creating a database with a non-normalized but fully-qualified path.
+  let nonNormalizedDir = await makeDatabaseDir("non-normalized");
+  nonNormalizedDir = OS.Path.join(nonNormalizedDir, "..", ".", "non-normalized");
+  Assert.ok(await KeyValueService.getOrCreate(nonNormalizedDir, "db"));
 });
 
 add_task(async function putGetHasDelete() {
   const databaseDir = await makeDatabaseDir("putGetHasDelete");
   const database = await KeyValueService.getOrCreate(databaseDir, "db");
 
   // Getting key/value pairs that don't exist (yet) returns default values
   // or null, depending on whether you specify a default value.