indexeddb: fix idbobjectstore rename error handling (#43754)

Implement the missing IDBObjectStore.name duplicate name checks so
renaming an object store to an existing store now throw ConstraintError

Testing: more indexeddb WPT test passed.

Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>

components/script/dom/indexeddb/idbdatabase.rs

@@ -237,36 +237,44 @@ impl IDBDatabaseMethods<crate::DomTypeHolder> for IDBDatabase {
         name: DOMString,
         options: &IDBObjectStoreParameters,
     ) -> Fallible<DomRoot<IDBObjectStore>> {
-        // Step 2
-        let upgrade_transaction = match self.upgrade_transaction.get() {
+        // Step 1. Let database be this’s associated database.
+
+        // Step 2. Let transaction be database’s upgrade transaction if it is not null,
+        // or throw an "InvalidStateError" DOMException otherwise.
+        let transaction = match self.upgrade_transaction.get() {
             Some(txn) => txn,
             None => return Err(Error::InvalidState(None)),
         };
 
-        // Step 3
-        if !upgrade_transaction.is_active() {
+        // Step 3. If transaction’s state is not active, then throw a
+        // "TransactionInactiveError" DOMException.
+        if !transaction.is_active() {
             return Err(Error::TransactionInactive(None));
         }
 
-        // Step 4
+        // Step 4. Let keyPath be options’s keyPath member if it is not undefined
+        // or null, or null otherwise.
         let key_path = options.keyPath.as_ref();
 
-        // Step 5
+        // Step 5. If keyPath is not null and is not a valid key path, throw a
+        // "SyntaxError" DOMException.
         if let Some(path) = key_path {
             if !is_valid_key_path(cx, path)? {
                 return Err(Error::Syntax(None));
             }
         }
 
-        // Step 6
+        // Step 6. If an object store named name already exists in database throw
+        // a "ConstraintError" DOMException.
         if self.object_store_names.borrow().contains(&name) {
             return Err(Error::Constraint(None));
         }
 
-        // Step 7
+        // Step 7. Let autoIncrement be options’s autoIncrement member.
         let auto_increment = options.autoIncrement;
 
-        // Step 8
+        // Step 8. If autoIncrement is true and keyPath is an empty string or any
+        // sequence (empty or otherwise), throw an "InvalidAccessError" DOMException.
         if auto_increment {
             match key_path {
                 Some(StringOrStringSequence::String(path)) => {
@@ -281,7 +289,10 @@ impl IDBDatabaseMethods<crate::DomTypeHolder> for IDBDatabase {
             }
         }
 
-        // Step 9
+        // Step 9. Let store be a new object store in database. Set the created
+        // object store’s name to name. If autoIncrement is true, then the
+        // created object store uses a key generator. If keyPath is not null,
+        // set the created object store’s key path to keyPath.
         let object_store = IDBObjectStore::new(
             &self.global(),
             self.name.clone(),
@@ -289,7 +300,7 @@ impl IDBDatabaseMethods<crate::DomTypeHolder> for IDBDatabase {
             Some(options),
             if auto_increment { Some(1) } else { None },
             CanGc::from_cx(cx),
-            &upgrade_transaction,
+            &transaction,
         );
 
         let (sender, receiver) = channel(self.global().time_profiler_chan().clone()).unwrap();
@@ -323,6 +334,8 @@ impl IDBDatabaseMethods<crate::DomTypeHolder> for IDBDatabase {
         };
 
         self.object_store_names.borrow_mut().push(name);
+
+        // Step 10. Return a new object store handle associated with store and transaction.
         Ok(object_store)
     }
 

components/script/dom/indexeddb/idbobjectstore.rs

@@ -786,12 +786,15 @@ impl IDBObjectStoreMethods<crate::DomTypeHolder> for IDBObjectStore {
         self.name.borrow().clone()
     }
 
-    /// <https://www.w3.org/TR/IndexedDB-3/#dom-idbobjectstore-setname>
+    /// <https://www.w3.org/TR/IndexedDB-3/#dom-idbobjectstore-name>
     fn SetName(&self, value: DOMString) -> ErrorResult {
+        // Step 1. Let name be the given value.
+        let name = value;
+
         // Step 2. Let transaction be this’s transaction.
         let transaction = &self.transaction;
 
-        // Step 3. Let store be this's object store.
+        // Step 3. Let store be this’s object store.
         // Step 4. If store has been deleted, throw an "InvalidStateError" DOMException.
         self.verify_not_deleted()?;
 
@@ -802,7 +805,20 @@ impl IDBObjectStoreMethods<crate::DomTypeHolder> for IDBObjectStore {
         // Step 6. If transaction’s state is not active, throw a "TransactionInactiveError" DOMException.
         self.check_transaction_active()?;
 
-        *self.name.borrow_mut() = value;
+        // Step 7. If store’s name is equal to name, terminate these steps.
+        if *self.name.borrow() == name {
+            return Ok(());
+        }
+
+        // Step 8. If an object store named name already exists in store’s database,
+        // throw a "ConstraintError" DOMException.
+        if transaction.Db().object_store_exists(&name) {
+            return Err(Error::Constraint(None));
+        }
+
+        // Step 9. Set store’s name to name.
+        // Step 10. Set this’s name to name.
+        *self.name.borrow_mut() = name;
         Ok(())
     }
 

components/storage/indexeddb/engines/sqlite.rs

@@ -69,6 +69,17 @@ pub struct SqliteEngine {
 }
 
 impl SqliteEngine {
+    fn object_store_by_name(
+        connection: &Connection,
+        store_name: &str,
+    ) -> Result<object_store_model::Model, Error> {
+        connection.query_row(
+            "SELECT * FROM object_store WHERE name = ?",
+            params![store_name.to_string()],
+            |row| object_store_model::Model::try_from(row),
+        )
+    }
+
     // TODO: intake dual pools
     pub fn new(
         base_dir: &Path,
@@ -369,9 +380,29 @@ impl KvsEngine for SqliteEngine {
     }
 
     fn delete_store(&self, store_name: &str) -> Result<(), Self::Error> {
+        // https://www.w3.org/TR/IndexedDB-3/#dom-idbdatabase-deleteobjectstore
+        // Step 7. Destroy store.
+        let object_store = Self::object_store_by_name(&self.connection, store_name)?;
+
+        self.connection.execute(
+            "DELETE FROM index_data WHERE object_store_id = ?",
+            params![object_store.id],
+        )?;
+        self.connection.execute(
+            "DELETE FROM unique_index_data WHERE object_store_id = ?",
+            params![object_store.id],
+        )?;
+        self.connection.execute(
+            "DELETE FROM object_store_index WHERE object_store_id = ?",
+            params![object_store.id],
+        )?;
+        self.connection.execute(
+            "DELETE FROM object_data WHERE object_store_id = ?",
+            params![object_store.id],
+        )?;
         let result = self.connection.execute(
-            "DELETE FROM object_store WHERE name = ?",
-            params![store_name.to_string()],
+            "DELETE FROM object_store WHERE id = ?",
+            params![object_store.id],
         )?;
         if result == 0 {
             Err(Error::QueryReturnedNoRows)
@@ -934,6 +965,58 @@ mod tests {
         assert!(result.is_err());
     }
 
+    #[test]
+    fn test_delete_store_removes_store_records() {
+        let base_dir = tempfile::tempdir().expect("Failed to create temp dir");
+        let thread_pool = get_pool();
+        let db = SqliteEngine::new(
+            base_dir.path(),
+            &IndexedDBDescription {
+                name: "test_db".to_string(),
+                origin: test_origin(),
+            },
+            thread_pool,
+        )
+        .unwrap();
+
+        db.create_store("test_store", None, false)
+            .expect("Failed to create store");
+        let object_store = SqliteEngine::object_store_by_name(&db.connection, "test_store")
+            .expect("Failed to fetch store metadata");
+        SqliteEngine::put_item(
+            &db.connection,
+            object_store.clone(),
+            IndexedDBKeyType::Number(1.0),
+            vec![1, 2, 3],
+            true,
+            None,
+        )
+        .expect("Failed to insert item");
+
+        let row_count_before: i64 = db
+            .connection
+            .query_row(
+                "SELECT COUNT(*) FROM object_data WHERE object_store_id = ?",
+                rusqlite::params![object_store.id],
+                |row| row.get(0),
+            )
+            .expect("Failed to count rows before delete");
+        assert_eq!(row_count_before, 1);
+
+        db.delete_store("test_store")
+            .expect("Failed to delete store");
+
+        let row_count_after: i64 = db
+            .connection
+            .query_row(
+                "SELECT COUNT(*) FROM object_data WHERE object_store_id = ?",
+                rusqlite::params![object_store.id],
+                |row| row.get(0),
+            )
+            .expect("Failed to count rows after delete");
+        assert_eq!(row_count_after, 0);
+    }
+
     #[test]
     fn test_async_operations() {
         fn get_channel<T>() -> (GenericSender<T>, GenericReceiver<T>)

components/storage/indexeddb/mod.rs

@@ -576,8 +576,8 @@ impl<E: KvsEngine> IndexedDBEnvironment<E> {
     }
 
     fn delete_object_store(&mut self, store_name: &str) -> DbResult<()> {
-        // IndexedDB §5.5: "For upgrade transactions this includes changes to the set of object stores and indexes".
-        // Delete index metadata first, then remove the store metadata row.
+        // https://www.w3.org/TR/IndexedDB-3/#dom-idbdatabase-deleteobjectstore
+        // Step 7. Destroy store.
         let indexes = self
             .engine
             .indexes(store_name)

tests/wpt/meta/IndexedDB/blob-delete-objectstore-db.any.js.ini

@@ -1,8 +0,0 @@
-[blob-delete-objectstore-db.any.worker.html]
-  [Deleting an object store and a database containing blobs doesn't crash.]
-    expected: FAIL
-
-
-[blob-delete-objectstore-db.any.html]
-  [Deleting an object store and a database containing blobs doesn't crash.]
-    expected: FAIL

tests/wpt/meta/IndexedDB/idbobjectstore-rename-errors.any.js.ini

@@ -1,19 +1,5 @@
 [idbobjectstore-rename-errors.any.serviceworker.html]
   expected: ERROR
 
-[idbobjectstore-rename-errors.any.worker.html]
-  [IndexedDB deleted object store rename throws]
-    expected: FAIL
-
-  [IndexedDB object store rename to the name of another store throws]
-    expected: FAIL
-
 [idbobjectstore-rename-errors.any.sharedworker.html]
   expected: ERROR
-
-[idbobjectstore-rename-errors.any.html]
-  [IndexedDB deleted object store rename throws]
-    expected: FAIL
-
-  [IndexedDB object store rename to the name of another store throws]
-    expected: FAIL