Bug 1635352 (part 1) - Add a new bridged extension-storage engine. r=lina

Differential Revision: https://phabricator.services.mozilla.com/D74609

browser/app/profile/firefox.js

@@ -1244,6 +1244,7 @@ pref("services.sync.prefs.sync.security.default_personal_cert", true);
 pref("services.sync.prefs.sync.services.sync.syncedTabs.showRemoteIcons", true);
 pref("services.sync.prefs.sync.signon.rememberSignons", true);
 pref("services.sync.prefs.sync.spellchecker.dictionary", true);
+pref("services.sync.prefs.sync.webextensions.storage.sync.kinto", true);
 
 // A preference which, if false, means sync will only apply incoming preference
 // changes if there's already a local services.sync.prefs.sync.* control pref.

services/sync/modules/bridged_engine.js

@@ -56,6 +56,7 @@ class BridgedStore {
       let incomingEnvelopesAsJSON = chunk.map(record =>
         JSON.stringify(record.toIncomingEnvelope())
       );
+      this._log.trace("incoming envelopes", incomingEnvelopesAsJSON);
       await promisify(
         this.engine._bridge.storeIncoming,
         incomingEnvelopesAsJSON
@@ -291,12 +292,17 @@ BridgedEngine.prototype = {
   },
 
   async getLastSync() {
-    let lastSync = await promisify(this._bridge.getLastSync);
+    // The bridge defines lastSync as integer ms, but sync itself wants to work
-    return lastSync;
+    // in a float seconds with 2 decimal places.
+    let lastSyncMS = await promisify(this._bridge.getLastSync);
+    return Math.round(lastSyncMS / 10) / 100;
   },
 
-  async setLastSync(lastSyncMillis) {
+  async setLastSync(lastSyncSeconds) {
-    await promisify(this._bridge.setLastSync, lastSyncMillis);
+    await promisify(
+      this._bridge.setLastSync,
+      Math.round(lastSyncSeconds * 1000)
+    );
   },
 
   /**
@@ -341,6 +347,7 @@ BridgedEngine.prototype = {
     let outgoingEnvelopesAsJSON = await promisify(this._bridge.apply);
     let changeset = {};
     for (let envelopeAsJSON of outgoingEnvelopesAsJSON) {
+      this._log.trace("outgoing envelope", envelopeAsJSON);
       let record = BridgedRecord.fromOutgoingEnvelope(
         this.name,
         JSON.parse(envelopeAsJSON)

services/sync/modules/engines.js

@@ -1852,7 +1852,7 @@ SyncEngine.prototype = {
         // the batch is complete, however we want to remember success/failure
         // indicators for when that happens.
         if (!resp.success) {
-          this._log.debug("Uploading records failed: " + resp);
+          this._log.debug(`Uploading records failed: ${resp.status}`);
           resp.failureCode =
             resp.status == 412 ? ENGINE_BATCH_INTERRUPTED : ENGINE_UPLOAD_FAIL;
           throw resp;

services/sync/modules/engines/extension-storage.js

@@ -4,24 +4,73 @@
 
 "use strict";
 
-var EXPORTED_SYMBOLS = ["ExtensionStorageEngine"];
+var EXPORTED_SYMBOLS = [
+  "ExtensionStorageEngineKinto",
+  "ExtensionStorageEngineBridge",
+];
 
-const { SCORE_INCREMENT_MEDIUM, MULTI_DEVICE_THRESHOLD } = ChromeUtils.import(
-  "resource://services-sync/constants.js"
-);
-const { SyncEngine, Tracker } = ChromeUtils.import(
-  "resource://services-sync/engines.js"
-);
-const { Svc } = ChromeUtils.import("resource://services-sync/util.js");
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
-ChromeUtils.defineModuleGetter(
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  BridgedEngine: "resource://services-sync/bridged_engine.js",
+  extensionStorageSync: "resource://gre/modules/ExtensionStorageSyncKinto.jsm",
+  Svc: "resource://services-sync/util.js",
+  SyncEngine: "resource://services-sync/engines.js",
+  Tracker: "resource://services-sync/engines.js",
+  SCORE_INCREMENT_MEDIUM: "resource://services-sync/constants.js",
+  MULTI_DEVICE_THRESHOLD: "resource://services-sync/constants.js",
+});
+
+XPCOMUtils.defineLazyServiceGetter(
   this,
-  "extensionStorageSync",
+  "StorageSyncService",
-  "resource://gre/modules/ExtensionStorageSyncKinto.jsm"
+  "@mozilla.org/extensions/storage/sync;1",
+  "nsIInterfaceRequestor"
 );
 
+// A helper to indicate whether extension-storage is enabled - it's based on
+// the "addons" pref. The same logic is shared between both engine impls.
+function isEngineEnabled() {
+  // By default, we sync extension storage if we sync addons. This
+  // lets us simplify the UX since users probably don't consider
+  // "extension preferences" a separate category of syncing.
+  // However, we also respect engine.extension-storage.force, which
+  // can be set to true or false, if a power user wants to customize
+  // the behavior despite the lack of UI.
+  const forced = Svc.Prefs.get("engine.extension-storage.force", undefined);
+  if (forced !== undefined) {
+    return forced;
+  }
+  return Svc.Prefs.get("engine.addons", false);
+}
+
+// A "bridged engine" to our webext-storage component.
+function ExtensionStorageEngineBridge(service) {
+  let bridge = StorageSyncService.getInterface(Ci.mozIBridgedSyncEngine);
+  BridgedEngine.call(this, bridge, "Extension-Storage", service);
+}
+
+ExtensionStorageEngineBridge.prototype = {
+  __proto__: BridgedEngine.prototype,
+  syncPriority: 10,
+  // we don't support repair at all!
+  _skipPercentageChance: 100,
+
+  get enabled() {
+    return isEngineEnabled();
+  },
+};
+
+/**
+ *****************************************************************************
+ *
+ * Deprecated support for Kinto
+ *
+ *****************************************************************************
+ */
+
 /**
  * The Engine that manages syncing for the web extension "storage"
  * API, and in particular ext.storage.sync.
@@ -30,7 +79,7 @@ ChromeUtils.defineModuleGetter(
  * for syncing that we do not need to integrate in the Firefox Sync
  * framework, so this is something of a stub.
  */
-function ExtensionStorageEngine(service) {
+function ExtensionStorageEngineKinto(service) {
   SyncEngine.call(this, "Extension-Storage", service);
   XPCOMUtils.defineLazyPreferenceGetter(
     this,
@@ -39,7 +88,7 @@ function ExtensionStorageEngine(service) {
     0
   );
 }
-ExtensionStorageEngine.prototype = {
+ExtensionStorageEngineKinto.prototype = {
   __proto__: SyncEngine.prototype,
   _trackerObj: ExtensionStorageTracker,
   // we don't need these since we implement our own sync logic
@@ -54,20 +103,7 @@ ExtensionStorageEngine.prototype = {
   },
 
   get enabled() {
-    // By default, we sync extension storage if we sync addons. This
+    return isEngineEnabled();
-    // lets us simplify the UX since users probably don't consider
-    // "extension preferences" a separate category of syncing.
-    // However, we also respect engine.extension-storage.force, which
-    // can be set to true or false, if a power user wants to customize
-    // the behavior despite the lack of UI.
-    const forced = Svc.Prefs.get(
-      "engine." + this.prefName + ".force",
-      undefined
-    );
-    if (forced !== undefined) {
-      return forced;
-    }
-    return Svc.Prefs.get("engine.addons", false);
   },
 
   _wipeClient() {

services/sync/modules/service.js

@@ -88,10 +88,6 @@ function getEngineModules() {
     Password: { module: "passwords.js", symbol: "PasswordEngine" },
     Prefs: { module: "prefs.js", symbol: "PrefsEngine" },
     Tab: { module: "tabs.js", symbol: "TabEngine" },
-    ExtensionStorage: {
-      module: "extension-storage.js",
-      symbol: "ExtensionStorageEngine",
-    },
   };
   if (Svc.Prefs.get("engine.addresses.available", false)) {
     result.Addresses = {
@@ -111,6 +107,12 @@ function getEngineModules() {
     whenFalse: "BookmarksEngine",
     whenTrue: "BufferedBookmarksEngine",
   };
+  result.ExtensionStorage = {
+    module: "extension-storage.js",
+    controllingPref: "webextensions.storage.sync.kinto",
+    whenTrue: "ExtensionStorageEngineKinto",
+    whenFalse: "ExtensionStorageEngineBridge",
+  };
   return result;
 }
 

services/sync/tests/unit/test_bridged_engine.js

@@ -150,7 +150,7 @@ add_task(async function test_interface() {
 
     info("Sync the engine");
     // Advance the last sync time to skip the Backstreet Boys...
-    bridge.lastSyncMillis = now + 2;
+    bridge.lastSyncMillis = 1000 * (now + 2);
     await sync_engine_and_validate_telem(engine, false);
 
     let metaGlobal = foo

services/sync/tests/unit/test_extension_storage_engine.js

@@ -3,123 +3,76 @@
 
 "use strict";
 
-const { ExtensionStorageEngine } = ChromeUtils.import(
+XPCOMUtils.defineLazyModuleGetters(this, {
-  "resource://services-sync/engines/extension-storage.js"
+  BridgedRecord: "resource://services-sync/bridged_engine.js",
-);
+  extensionStorageSync: "resource://gre/modules/ExtensionStorageSync.jsm",
-const { Service } = ChromeUtils.import("resource://services-sync/service.js");
+  ExtensionStorageEngineBridge:
-const { extensionStorageSync } = ChromeUtils.import(
+    "resource://services-sync/engines/extension-storage.js",
-  "resource://gre/modules/ExtensionStorageSyncKinto.jsm"
+  Service: "resource://services-sync/service.js",
-);
-
-let engine;
-
-function mock(options) {
-  let calls = [];
-  let ret = function() {
-    calls.push(arguments);
-    return options.returns;
-  };
-  Object.setPrototypeOf(ret, {
-    __proto__: Function.prototype,
-    get calls() {
-      return calls;
-    },
-  });
-  return ret;
-}
-
-function setSkipChance(v) {
-  Services.prefs.setIntPref(
-    "services.sync.extension-storage.skipPercentageChance",
-    v
-  );
-}
-
-add_task(async function setup() {
-  await Service.engineManager.register(ExtensionStorageEngine);
-  engine = Service.engineManager.get("extension-storage");
-  do_get_profile(); // so we can use FxAccounts
-  loadWebExtensionTestFunctions();
-  setSkipChance(0);
 });
 
-add_task(async function test_calling_sync_calls__sync() {
+Services.prefs.setBoolPref("webextensions.storage.sync.kinto", false); // shouldn't need this
-  let oldSync = ExtensionStorageEngine.prototype._sync;
+Services.prefs.setStringPref("webextensions.storage.sync.log.level", "debug");
-  let syncMock = (ExtensionStorageEngine.prototype._sync = mock({
-    returns: true,
-  }));
-  try {
-    // I wanted to call the main sync entry point for the entire
-    // package, but that fails because it tries to sync ClientEngine
-    // first, which fails.
-    await engine.sync();
-  } finally {
-    ExtensionStorageEngine.prototype._sync = oldSync;
-  }
-  equal(syncMock.calls.length, 1);
-});
 
-add_task(async function test_sync_skip() {
+// It's difficult to know what to test - there's already tests for the bridged
-  try {
+// engine etc - so we just try and check that this engine conforms to the
-    // Do a few times to ensure we aren't getting "lucky" WRT Math.random()
+// mozIBridgedSyncEngine interface guarantees.
-    for (let i = 0; i < 10; ++i) {
+add_task(async function test_engine() {
-      setSkipChance(100);
+  let engine = new ExtensionStorageEngineBridge(Service);
-      engine._tracker._score = 0;
+  Assert.equal(engine.version, 1);
-      ok(
-        !engine.shouldSkipSync("user"),
-        "Should allow explicitly requested syncs"
-      );
-      ok(!engine.shouldSkipSync("startup"), "Should allow startup syncs");
-      ok(
-        engine.shouldSkipSync("schedule"),
-        "Should skip scheduled syncs if skipProbability is 100"
-      );
-      engine._tracker._score = MULTI_DEVICE_THRESHOLD;
-      ok(
-        !engine.shouldSkipSync("schedule"),
-        "should allow scheduled syncs if tracker score is high"
-      );
-      engine._tracker._score = 0;
-      setSkipChance(0);
-      ok(
-        !engine.shouldSkipSync("schedule"),
-        "Should allow scheduled syncs if probability is 0"
-      );
-    }
-  } finally {
-    engine._tracker._score = 0;
-    setSkipChance(0);
-  }
-});
 
-add_task(async function test_calling_wipeClient_calls_clearAll() {
+  Assert.deepEqual(await engine.getSyncID(), null);
-  let oldClearAll = extensionStorageSync.clearAll;
+  await engine.resetLocalSyncID();
-  let clearMock = (extensionStorageSync.clearAll = mock({
+  Assert.notEqual(await engine.getSyncID(), null);
-    returns: Promise.resolve(),
-  }));
-  try {
-    await engine.wipeClient();
-  } finally {
-    extensionStorageSync.clearAll = oldClearAll;
-  }
-  equal(clearMock.calls.length, 1);
-});
 
-add_task(async function test_calling_sync_calls_ext_storage_sync() {
+  Assert.equal(await engine.getLastSync(), 0);
-  const extension = { id: "my-extension" };
+  // lastSync is seconds on this side of the world, but milli-seconds on the other.
-  let oldSync = extensionStorageSync.syncAll;
+  await engine.setLastSync(1234.567);
-  let syncMock = (extensionStorageSync.syncAll = mock({
+  // should have 2 digit precision.
-    returns: Promise.resolve(),
+  Assert.equal(await engine.getLastSync(), 1234.57);
-  }));
+  await engine.setLastSync(0);
-  try {
-    await withSyncContext(async function(context) {
-      // Set something so that everyone knows that we're using storage.sync
-      await extensionStorageSync.set(extension, { a: "b" }, context);
 
-      await engine._sync();
+  // Set some data.
+  await extensionStorageSync.set({ id: "ext-2" }, { ext_2_key: "ext_2_value" });
+  // Now do a sync with out regular test server.
+  let server = await serverForFoo(engine);
+  try {
+    await SyncTestingInfrastructure(server);
+
+    info("Add server records");
+    let foo = server.user("foo");
+    let collection = foo.collection("extension-storage");
+    let now = new_timestamp();
+
+    collection.insert(
+      "fakeguid0000",
+      encryptPayload({
+        id: "fakeguid0000",
+        extId: "ext-1",
+        data: JSON.stringify({ foo: "bar" }),
+      }),
+      now
+    );
+
+    info("Sync the engine");
+    await sync_engine_and_validate_telem(engine, false);
+
+    // We should have applied the data from the existing collection record.
+    Assert.deepEqual(await extensionStorageSync.get({ id: "ext-1" }, null), {
+      foo: "bar",
     });
+
+    // should now be 2 records on the server.
+    let payloads = collection.payloads();
+    Assert.equal(payloads.length, 2);
+    // find the new one we wrote.
+    let newPayload =
+      payloads[0].id == "fakeguid0000" ? payloads[1] : payloads[0];
+    Assert.equal(newPayload.data, `{"ext_2_key":"ext_2_value"}`);
+    // should have updated the timestamp.
+    greater(await engine.getLastSync(), 0, "Should update last sync time");
   } finally {
-    extensionStorageSync.syncAll = oldSync;
+    await promiseStopServer(server);
+    await engine.finalize();
   }
-  Assert.ok(syncMock.calls.length >= 1);
 });

services/sync/tests/unit/test_extension_storage_engine_kinto.js

@@ -0,0 +1,125 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const {
+  ExtensionStorageEngineKinto: ExtensionStorageEngine,
+} = ChromeUtils.import("resource://services-sync/engines/extension-storage.js");
+const { Service } = ChromeUtils.import("resource://services-sync/service.js");
+const { extensionStorageSync } = ChromeUtils.import(
+  "resource://gre/modules/ExtensionStorageSyncKinto.jsm"
+);
+
+let engine;
+
+function mock(options) {
+  let calls = [];
+  let ret = function() {
+    calls.push(arguments);
+    return options.returns;
+  };
+  Object.setPrototypeOf(ret, {
+    __proto__: Function.prototype,
+    get calls() {
+      return calls;
+    },
+  });
+  return ret;
+}
+
+function setSkipChance(v) {
+  Services.prefs.setIntPref(
+    "services.sync.extension-storage.skipPercentageChance",
+    v
+  );
+}
+
+add_task(async function setup() {
+  await Service.engineManager.register(ExtensionStorageEngine);
+  engine = Service.engineManager.get("extension-storage");
+  do_get_profile(); // so we can use FxAccounts
+  loadWebExtensionTestFunctions();
+  setSkipChance(0);
+});
+
+add_task(async function test_calling_sync_calls__sync() {
+  let oldSync = ExtensionStorageEngine.prototype._sync;
+  let syncMock = (ExtensionStorageEngine.prototype._sync = mock({
+    returns: true,
+  }));
+  try {
+    // I wanted to call the main sync entry point for the entire
+    // package, but that fails because it tries to sync ClientEngine
+    // first, which fails.
+    await engine.sync();
+  } finally {
+    ExtensionStorageEngine.prototype._sync = oldSync;
+  }
+  equal(syncMock.calls.length, 1);
+});
+
+add_task(async function test_sync_skip() {
+  try {
+    // Do a few times to ensure we aren't getting "lucky" WRT Math.random()
+    for (let i = 0; i < 10; ++i) {
+      setSkipChance(100);
+      engine._tracker._score = 0;
+      ok(
+        !engine.shouldSkipSync("user"),
+        "Should allow explicitly requested syncs"
+      );
+      ok(!engine.shouldSkipSync("startup"), "Should allow startup syncs");
+      ok(
+        engine.shouldSkipSync("schedule"),
+        "Should skip scheduled syncs if skipProbability is 100"
+      );
+      engine._tracker._score = MULTI_DEVICE_THRESHOLD;
+      ok(
+        !engine.shouldSkipSync("schedule"),
+        "should allow scheduled syncs if tracker score is high"
+      );
+      engine._tracker._score = 0;
+      setSkipChance(0);
+      ok(
+        !engine.shouldSkipSync("schedule"),
+        "Should allow scheduled syncs if probability is 0"
+      );
+    }
+  } finally {
+    engine._tracker._score = 0;
+    setSkipChance(0);
+  }
+});
+
+add_task(async function test_calling_wipeClient_calls_clearAll() {
+  let oldClearAll = extensionStorageSync.clearAll;
+  let clearMock = (extensionStorageSync.clearAll = mock({
+    returns: Promise.resolve(),
+  }));
+  try {
+    await engine.wipeClient();
+  } finally {
+    extensionStorageSync.clearAll = oldClearAll;
+  }
+  equal(clearMock.calls.length, 1);
+});
+
+add_task(async function test_calling_sync_calls_ext_storage_sync() {
+  const extension = { id: "my-extension" };
+  let oldSync = extensionStorageSync.syncAll;
+  let syncMock = (extensionStorageSync.syncAll = mock({
+    returns: Promise.resolve(),
+  }));
+  try {
+    await withSyncContext(async function(context) {
+      // Set something so that everyone knows that we're using storage.sync
+      await extensionStorageSync.set(extension, { a: "b" }, context);
+
+      await engine._sync();
+    });
+  } finally {
+    extensionStorageSync.syncAll = oldSync;
+  }
+  Assert.ok(syncMock.calls.length >= 1);
+});

services/sync/tests/unit/xpcshell.ini

@@ -148,7 +148,8 @@ run-sequentially = Frequent timeouts, bug 1395148
 [test_clients_escape.js]
 [test_doctor.js]
 [test_extension_storage_engine.js]
-[test_extension_storage_tracker.js]
+[test_extension_storage_engine_kinto.js]
+[test_extension_storage_tracker_kinto.js]
 [test_forms_store.js]
 [test_forms_tracker.js]
 [test_form_validator.js]

toolkit/components/extensions/storage/webext_storage_bridge/src/area.rs

@@ -4,6 +4,7 @@
 
 use std::{
     cell::{Ref, RefCell},
+    convert::TryInto,
     ffi::OsString,
     mem, str,
     sync::Arc,
@@ -14,6 +15,7 @@ use moz_task::{self, DispatchOptions, TaskRunnable};
 use nserror::{nsresult, NS_OK};
 use nsstring::{nsACString, nsCString, nsString};
 use thin_vec::ThinVec;
+use webext_storage::STORAGE_VERSION;
 use xpcom::{
     interfaces::{
         mozIBridgedSyncEngineApplyCallback, mozIBridgedSyncEngineCallback,
@@ -293,7 +295,7 @@ impl StorageSyncArea {
 
     xpcom_method!(get_storage_version => GetStorageVersion() -> i32);
     fn get_storage_version(&self) -> Result<i32> {
-        Ok(1)
+        Ok(STORAGE_VERSION.try_into().unwrap())
     }
 
     xpcom_method!(