webext-storage: Do not check total bytes quota on storage.sync.remove

CHANGES_UNRELEASED.md

@@ -4,7 +4,11 @@
 
 [Full Changelog](https://github.com/mozilla/application-services/compare/v62.0.0...main)
 
- ## FxA Client
+## General
+
+- Do not check total bytes quota on storage.sync.remote operations ([Bug 1656947](https://bugzilla.mozilla.org/1656947))
+
+## FxA Client
 
 ### What's new ###
 - Send-tab metrics are recorded. A new function, `fxa_gather_telemetry` on the

components/webext-storage/src/api.rs

@@ -23,6 +23,12 @@ pub const SYNC_MAX_ITEMS: usize = 512;
 
 type JsonMap = Map<String, JsonValue>;
 
+enum StorageChangeOp {
+    Clear,
+    Set(JsonValue),
+    SetWithoutQuota(JsonValue),
+}
+
 fn get_from_db(conn: &Connection, ext_id: &str) -> Result<Option<JsonMap>> {
     Ok(
         match conn.try_query_one::<String>(
@@ -42,13 +48,14 @@ fn get_from_db(conn: &Connection, ext_id: &str) -> Result<Option<JsonMap>> {
     )
 }
 
-fn save_to_db(tx: &Transaction<'_>, ext_id: &str, val: &JsonValue) -> Result<()> {
+fn save_to_db(tx: &Transaction<'_>, ext_id: &str, val: &StorageChangeOp) -> Result<()> {
     // This function also handles removals. Either an empty map or explicit null
     // is a removal. If there's a mirror record for this extension ID, then we
     // must leave a tombstone behind for syncing.
     let is_delete = match val {
-        JsonValue::Null => true,
-        JsonValue::Object(m) => m.is_empty(),
+        StorageChangeOp::Clear => true,
+        StorageChangeOp::Set(JsonValue::Object(v)) => v.is_empty(),
+        StorageChangeOp::SetWithoutQuota(JsonValue::Object(v)) => v.is_empty(),
         _ => false,
     };
     if is_delete {
@@ -84,11 +91,19 @@ fn save_to_db(tx: &Transaction<'_>, ext_id: &str, val: &JsonValue) -> Result<()>
             )?;
         }
     } else {
-        // Convert to bytes so we can enforce the quota.
-        let sval = val.to_string();
-        if sval.len() > SYNC_QUOTA_BYTES {
-            return Err(ErrorKind::QuotaError(QuotaReason::TotalBytes).into());
-        }
+        // Convert to bytes so we can enforce the quota if necessary.
+        let sval = match val {
+            StorageChangeOp::Set(v) => {
+                let sv = v.to_string();
+                if sv.len() > SYNC_QUOTA_BYTES {
+                    return Err(ErrorKind::QuotaError(QuotaReason::TotalBytes).into());
+                }
+                sv
+            }
+            StorageChangeOp::SetWithoutQuota(v) => v.to_string(),
+            StorageChangeOp::Clear => unreachable!(),
+        };
+
         log::trace!("saving data for '{}': writing", ext_id);
         tx.execute_named_cached(
             "INSERT INTO storage_sync_data(ext_id, data, sync_change_counter)
@@ -105,7 +120,7 @@ fn save_to_db(tx: &Transaction<'_>, ext_id: &str, val: &JsonValue) -> Result<()>
 }
 
 fn remove_from_db(tx: &Transaction<'_>, ext_id: &str) -> Result<()> {
-    save_to_db(tx, ext_id, &JsonValue::Null)
+    save_to_db(tx, ext_id, &StorageChangeOp::Clear)
 }
 
 // This is a "helper struct" for the callback part of the chrome.storage spec,
@@ -206,7 +221,11 @@ pub fn set(tx: &Transaction<'_>, ext_id: &str, val: JsonValue) -> Result<Storage
         current.insert(k, v);
     }
 
-    save_to_db(tx, ext_id, &JsonValue::Object(current))?;
+    save_to_db(
+        tx,
+        ext_id,
+        &StorageChangeOp::Set(JsonValue::Object(current)),
+    )?;
     Ok(changes)
 }
 
@@ -281,7 +300,11 @@ pub fn remove(tx: &Transaction<'_>, ext_id: &str, keys: JsonValue) -> Result<Sto
         }
     }
     if !result.is_empty() {
-        save_to_db(tx, ext_id, &JsonValue::Object(existing))?;
+        save_to_db(
+            tx,
+            ext_id,
+            &StorageChangeOp::SetWithoutQuota(JsonValue::Object(existing)),
+        )?;
     }
     Ok(result)
 }
@@ -620,6 +643,43 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_quota_bytes() -> Result<()> {
+        let mut db = new_mem_db();
+        let tx = db.transaction()?;
+        let ext_id = "xyz";
+        let val = "x".repeat(SYNC_QUOTA_BYTES + 1);
+
+        // Init an over quota db with a single key.
+        save_to_db(
+            &tx,
+            ext_id,
+            &StorageChangeOp::SetWithoutQuota(json!({ "x": val })),
+        )?;
+
+        // Adding more data fails.
+        let e = set(&tx, &ext_id, json!({ "y": "newvalue" })).unwrap_err();
+        match e.kind() {
+            ErrorKind::QuotaError(QuotaReason::TotalBytes) => {}
+            _ => panic!("unexpected error type"),
+        };
+
+        // Remove data does not fails.
+        remove(&tx, &ext_id, json!["x"])?;
+
+        // Restore the over quota data.
+        save_to_db(
+            &tx,
+            ext_id,
+            &StorageChangeOp::SetWithoutQuota(json!({ "y": val })),
+        )?;
+
+        // Overwrite with less data does not fail.
+        set(&tx, &ext_id, json!({ "y": "lessdata" }))?;
+
+        Ok(())
+    }
+
     #[test]
     fn test_get_bytes_in_use() -> Result<()> {
         let mut db = new_mem_db();