diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index c729a1db8766..5c7a8aa592b6 100644 --- a/browser/app/profile/firefox.js +++ b/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. diff --git a/services/sync/modules/bridged_engine.js b/services/sync/modules/bridged_engine.js index d0452ec0c3ab..f444b615521b 100644 --- a/services/sync/modules/bridged_engine.js +++ b/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); - return lastSync; + // The bridge defines lastSync as integer ms, but sync itself wants to work + // in a float seconds with 2 decimal places. + let lastSyncMS = await promisify(this._bridge.getLastSync); + return Math.round(lastSyncMS / 10) / 100; }, - async setLastSync(lastSyncMillis) { - await promisify(this._bridge.setLastSync, lastSyncMillis); + async setLastSync(lastSyncSeconds) { + 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) diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index 0a1538b4bc0f..11968d11333b 100644 --- a/services/sync/modules/engines.js +++ b/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; diff --git a/services/sync/modules/engines/extension-storage.js b/services/sync/modules/engines/extension-storage.js index a8d7e7e0e670..fdbe8796b7ad 100644 --- a/services/sync/modules/engines/extension-storage.js +++ b/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", - "resource://gre/modules/ExtensionStorageSyncKinto.jsm" + "StorageSyncService", + "@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 - // 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); + return isEngineEnabled(); }, _wipeClient() { diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index 6e8795673e05..7baaa9103dfb 100644 --- a/services/sync/modules/service.js +++ b/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; } diff --git a/services/sync/tests/unit/test_bridged_engine.js b/services/sync/tests/unit/test_bridged_engine.js index 147dac72a6a2..fdcaa11dc51d 100644 --- a/services/sync/tests/unit/test_bridged_engine.js +++ b/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 diff --git a/services/sync/tests/unit/test_extension_storage_engine.js b/services/sync/tests/unit/test_extension_storage_engine.js index 99df6dd5f91c..6614ca09a684 100644 --- a/services/sync/tests/unit/test_extension_storage_engine.js +++ b/services/sync/tests/unit/test_extension_storage_engine.js @@ -3,123 +3,76 @@ "use strict"; -const { 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); +XPCOMUtils.defineLazyModuleGetters(this, { + BridgedRecord: "resource://services-sync/bridged_engine.js", + extensionStorageSync: "resource://gre/modules/ExtensionStorageSync.jsm", + ExtensionStorageEngineBridge: + "resource://services-sync/engines/extension-storage.js", + Service: "resource://services-sync/service.js", }); -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); -}); +Services.prefs.setBoolPref("webextensions.storage.sync.kinto", false); // shouldn't need this +Services.prefs.setStringPref("webextensions.storage.sync.log.level", "debug"); -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); - } -}); +// It's difficult to know what to test - there's already tests for the bridged +// engine etc - so we just try and check that this engine conforms to the +// mozIBridgedSyncEngine interface guarantees. +add_task(async function test_engine() { + let engine = new ExtensionStorageEngineBridge(Service); + Assert.equal(engine.version, 1); -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); -}); + Assert.deepEqual(await engine.getSyncID(), null); + await engine.resetLocalSyncID(); + Assert.notEqual(await engine.getSyncID(), null); -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); + Assert.equal(await engine.getLastSync(), 0); + // lastSync is seconds on this side of the world, but milli-seconds on the other. + await engine.setLastSync(1234.567); + // should have 2 digit precision. + Assert.equal(await engine.getLastSync(), 1234.57); + await engine.setLastSync(0); - 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); }); diff --git a/services/sync/tests/unit/test_extension_storage_engine_kinto.js b/services/sync/tests/unit/test_extension_storage_engine_kinto.js new file mode 100644 index 000000000000..e167bb02d5a6 --- /dev/null +++ b/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); +}); diff --git a/services/sync/tests/unit/test_extension_storage_tracker.js b/services/sync/tests/unit/test_extension_storage_tracker_kinto.js similarity index 100% rename from services/sync/tests/unit/test_extension_storage_tracker.js rename to services/sync/tests/unit/test_extension_storage_tracker_kinto.js diff --git a/services/sync/tests/unit/xpcshell.ini b/services/sync/tests/unit/xpcshell.ini index 6f2ba9643737..7eb46145d618 100644 --- a/services/sync/tests/unit/xpcshell.ini +++ b/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] diff --git a/toolkit/components/extensions/storage/webext_storage_bridge/src/area.rs b/toolkit/components/extensions/storage/webext_storage_bridge/src/area.rs index 51ecb2ec874e..a1ba9298d2c0 100644 --- a/toolkit/components/extensions/storage/webext_storage_bridge/src/area.rs +++ b/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 { - Ok(1) + Ok(STORAGE_VERSION.try_into().unwrap()) } xpcom_method!(