diff --git a/Cargo.lock b/Cargo.lock index 9d5221fa88e4..042635fa7fd3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -420,7 +420,6 @@ dependencies = [ "nsstring", "storage", "storage_variant", - "thin-vec", "url", "xpcom", ] diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index d09cfaf23a28..5a7837e6a971 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -800,31 +800,6 @@ function SyncEngine(name, service) { ); // Async initializations can be made in the initialize() method. - // The map of ids => metadata for records needing a weak upload. - // - // Currently the "metadata" fields are: - // - // - forceTombstone: whether or not we should ignore the local information - // about the record, and write a tombstone for it anyway -- e.g. in the case - // of records that should exist locally, but should never be uploaded to the - // server (note that not all sync engines support tombstones) - // - // The difference between this and a "normal" upload is that these records - // are only tracked in memory, and if the upload attempt fails (shutdown, - // 412, etc), we abort uploading the "weak" set (by clearing the map). - // - // The rationale here is for the cases where we receive a record from the - // server that we know is wrong in some (small) way. For example, the - // dateAdded field on bookmarks -- maybe we have a better date, or the server - // record is entirely missing the date, etc. - // - // In these cases, we fix our local copy of the record, and mark it for - // weak upload. A normal ("strong") upload is problematic here because - // in the case of a conflict from the server, there's a window where our - // record would be marked as modified more recently than a change that occurs - // on another device change, and we lose data from the user. - this._needWeakUpload = new Map(); - this.asyncObserver = Async.asyncObserver(this, this._log); } @@ -1137,10 +1112,6 @@ SyncEngine.prototype = { return tombstone; }, - addForWeakUpload(id, { forceTombstone = false } = {}) { - this._needWeakUpload.set(id, { forceTombstone }); - }, - // Any setup that needs to happen at the beginning of each sync. async _syncStartup() { // Determine if we need to wipe on outdated versions @@ -1828,9 +1799,6 @@ SyncEngine.prototype = { // collection we'll upload let up = new Collection(this.engineURL, null, this.service); let modifiedIDs = new Set(this._modified.ids()); - for (let id of this._needWeakUpload.keys()) { - modifiedIDs.add(id); - } let counts = { failed: 0, sent: 0 }; this._log.info(`Uploading ${modifiedIDs.size} outgoing records`); if (modifiedIDs.size) { @@ -1896,12 +1864,7 @@ SyncEngine.prototype = { let out; let ok = false; try { - let { forceTombstone = false } = this._needWeakUpload.get(id) || {}; - if (forceTombstone) { - out = await this._createTombstone(id); - } else { - out = await this._createRecord(id); - } + out = await this._createRecord(id); if (this._log.level <= Log.Level.Trace) { this._log.trace("Outgoing: " + out); } @@ -1943,7 +1906,6 @@ SyncEngine.prototype = { } await postQueue.flush(true); } - this._needWeakUpload.clear(); if (counts.sent || counts.failed) { Observers.notify("weave:engine:sync:uploaded", counts, this.name); @@ -1987,7 +1949,6 @@ SyncEngine.prototype = { }, async _syncCleanup() { - this._needWeakUpload.clear(); try { // Mark failed WBOs as changed again so they are reuploaded next time. await this.trackRemainingChanges(); @@ -2159,7 +2120,6 @@ SyncEngine.prototype = { this.hasSyncedThisSession = false; this.previousFailed = new SerializableSet(); this.toFetch = new SerializableSet(); - this._needWeakUpload.clear(); }, /** diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index 2244cd74093f..912dd026f1a0 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -510,7 +510,6 @@ BookmarksEngine.prototype = { try { let recordsToUpload = await buf.apply({ remoteTimeSeconds: Resource.serverTime, - weakUpload: [...this._needWeakUpload.keys()], signal: watchdog.signal, }); this._modified.replace(recordsToUpload); @@ -519,7 +518,6 @@ BookmarksEngine.prototype = { if (watchdog.abortReason) { this._log.warn(`Aborting bookmark merge: ${watchdog.abortReason}`); } - this._needWeakUpload.clear(); } }, @@ -546,9 +544,6 @@ BookmarksEngine.prototype = { }, async _doCreateRecord(id) { - if (this._needWeakUpload.has(id)) { - return this._store.createRecord(id, this.name); - } let change = this._modified.changes[id]; if (!change) { this._log.error( diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js index a45ad3a64332..5e6d40908caa 100644 --- a/services/sync/tests/unit/test_bookmark_engine.js +++ b/services/sync/tests/unit/test_bookmark_engine.js @@ -1026,9 +1026,6 @@ add_task(async function test_buffer_hasDupe() { await PlacesUtils.bookmarks.remove(guid1); - // Make sure it works for weakly uploaded records - engine.addForWeakUpload(guid2); - await sync_engine_and_validate_telem(engine, false); let tombstone = JSON.parse( diff --git a/services/sync/tests/unit/test_bookmark_tracker.js b/services/sync/tests/unit/test_bookmark_tracker.js index 7f05a0e513a3..2b0e76b07201 100644 --- a/services/sync/tests/unit/test_bookmark_tracker.js +++ b/services/sync/tests/unit/test_bookmark_tracker.js @@ -64,7 +64,6 @@ function promiseSpinningly(promise) { async function cleanup() { await engine.setLastSync(0); - engine._needWeakUpload.clear(); await store.wipe(); await resetTracker(); await tracker.stop(); diff --git a/toolkit/components/places/SyncedBookmarksMirror.jsm b/toolkit/components/places/SyncedBookmarksMirror.jsm index 110595434bb0..8ee416f69c82 100644 --- a/toolkit/components/places/SyncedBookmarksMirror.jsm +++ b/toolkit/components/places/SyncedBookmarksMirror.jsm @@ -533,8 +533,6 @@ class SyncedBookmarksMirror { * The current local time, in seconds. * @param {Number} [options.remoteTimeSeconds] * The current server time, in seconds. - * @param {String[]} [options.weakUpload] - * GUIDs of bookmarks to weakly upload. * @param {Number} [options.maxFrecenciesToRecalculate] * The maximum number of bookmark URL frecencies to recalculate after * this merge. Frecency calculation blocks other Places writes, so we @@ -559,7 +557,6 @@ class SyncedBookmarksMirror { async apply({ localTimeSeconds, remoteTimeSeconds, - weakUpload, maxFrecenciesToRecalculate, notifyInStableOrder, signal = null, @@ -580,7 +577,6 @@ class SyncedBookmarksMirror { finalizeOrInterruptSignal, localTimeSeconds, remoteTimeSeconds, - weakUpload, maxFrecenciesToRecalculate, notifyInStableOrder ); @@ -595,12 +591,11 @@ class SyncedBookmarksMirror { signal, localTimeSeconds, remoteTimeSeconds, - weakUpload, maxFrecenciesToRecalculate = DEFAULT_MAX_FRECENCIES_TO_RECALCULATE, notifyInStableOrder = false ) { let wasMerged = await withTiming("Merging bookmarks in Rust", () => - this.merge(signal, localTimeSeconds, remoteTimeSeconds, weakUpload) + this.merge(signal, localTimeSeconds, remoteTimeSeconds) ); if (!wasMerged) { @@ -672,12 +667,7 @@ class SyncedBookmarksMirror { return changeRecords; } - merge( - signal, - localTimeSeconds = Date.now() / 1000, - remoteTimeSeconds = 0, - weakUpload = [] - ) { + merge(signal, localTimeSeconds = Date.now() / 1000, remoteTimeSeconds = 0) { return new Promise((resolve, reject) => { let op = null; function onAbort() { @@ -775,12 +765,7 @@ class SyncedBookmarksMirror { } }, }; - op = this.merger.merge( - localTimeSeconds, - remoteTimeSeconds, - weakUpload, - callback - ); + op = this.merger.merge(localTimeSeconds, remoteTimeSeconds, callback); if (signal.aborted) { op.cancel(); } else { diff --git a/toolkit/components/places/bookmark_sync/Cargo.toml b/toolkit/components/places/bookmark_sync/Cargo.toml index 9b2c4f313735..f3dd2d76a101 100644 --- a/toolkit/components/places/bookmark_sync/Cargo.toml +++ b/toolkit/components/places/bookmark_sync/Cargo.toml @@ -17,7 +17,3 @@ storage = { path = "../../../../storage/rust" } storage_variant = { path = "../../../../storage/variant" } url = "2.0" xpcom = { path = "../../../../xpcom/rust/xpcom" } - -[dependencies.thin-vec] -version = "0.2.1" -features = ["gecko-ffi"] diff --git a/toolkit/components/places/bookmark_sync/src/merger.rs b/toolkit/components/places/bookmark_sync/src/merger.rs index eb628966810a..e14042d58908 100644 --- a/toolkit/components/places/bookmark_sync/src/merger.rs +++ b/toolkit/components/places/bookmark_sync/src/merger.rs @@ -11,7 +11,6 @@ use moz_task::{Task, TaskRunnable, ThreadPtrHandle, ThreadPtrHolder}; use nserror::{nsresult, NS_ERROR_NOT_AVAILABLE, NS_OK}; use nsstring::nsString; use storage::Conn; -use thin_vec::ThinVec; use xpcom::{ interfaces::{ mozIPlacesPendingOperation, mozIServicesLogSink, mozIStorageConnection, @@ -74,7 +73,6 @@ impl SyncedBookmarksMerger { merge => Merge( local_time_seconds: i64, remote_time_seconds: i64, - weak_uploads: *const ThinVec<::nsstring::nsString>, callback: *const mozISyncedBookmarksMirrorCallback ) -> *const mozIPlacesPendingOperation ); @@ -82,7 +80,6 @@ impl SyncedBookmarksMerger { &self, local_time_seconds: i64, remote_time_seconds: i64, - weak_uploads: Option<&ThinVec>, callback: &mozISyncedBookmarksMirrorCallback, ) -> Result, nsresult> { let callback = RefPtr::new(callback); @@ -99,9 +96,6 @@ impl SyncedBookmarksMerger { logger.as_ref().cloned(), local_time_seconds, remote_time_seconds, - weak_uploads - .map(|w| w.as_slice().to_vec()) - .unwrap_or_default(), callback, )?; let runnable = TaskRunnable::new( @@ -128,7 +122,6 @@ struct MergeTask { logger: Option>, local_time_millis: i64, remote_time_millis: i64, - weak_uploads: Vec, progress: Option>, callback: ThreadPtrHandle, result: AtomicRefCell>, @@ -141,7 +134,6 @@ impl MergeTask { logger: Option>, local_time_seconds: i64, remote_time_seconds: i64, - weak_uploads: Vec, callback: RefPtr, ) -> Result { let max_log_level = logger @@ -175,7 +167,6 @@ impl MergeTask { logger, local_time_millis: local_time_seconds * 1000, remote_time_millis: remote_time_seconds * 1000, - weak_uploads, progress, callback: ThreadPtrHolder::new(cstr!("mozISyncedBookmarksMirrorCallback"), callback)?, result: AtomicRefCell::new(Err(error::Error::DidNotRun)), @@ -203,7 +194,6 @@ impl MergeTask { &self.controller, self.local_time_millis, self.remote_time_millis, - &self.weak_uploads, ); store.validate()?; store.prepare()?; diff --git a/toolkit/components/places/bookmark_sync/src/store.rs b/toolkit/components/places/bookmark_sync/src/store.rs index d69e870ce9d8..3e7aad69e16a 100644 --- a/toolkit/components/places/bookmark_sync/src/store.rs +++ b/toolkit/components/places/bookmark_sync/src/store.rs @@ -52,7 +52,6 @@ pub struct Store<'s> { local_time_millis: i64, remote_time_millis: i64, - weak_uploads: &'s [nsString], } impl<'s> Store<'s> { @@ -62,7 +61,6 @@ impl<'s> Store<'s> { controller: &'s AbortController, local_time_millis: i64, remote_time_millis: i64, - weak_uploads: &'s [nsString], ) -> Store<'s> { Store { db, @@ -71,7 +69,6 @@ impl<'s> Store<'s> { total_sync_changes: total_sync_changes(), local_time_millis, remote_time_millis, - weak_uploads, } } @@ -453,7 +450,7 @@ impl<'s> dogear::Store for Store<'s> { fn apply<'t>(&mut self, root: MergedRoot<'t>) -> Result { let ops = root.completion_ops_with_signal(self.controller)?; - if ops.is_empty() && self.weak_uploads.is_empty() { + if ops.is_empty() { // If we don't have any items to apply, upload, or delete, // no need to open a transaction at all. return Ok(ApplyStatus::Skipped); @@ -486,7 +483,6 @@ impl<'s> dogear::Store for Store<'s> { &self.controller, &ops.upload_items, &ops.upload_tombstones, - &self.weak_uploads, )?; cleanup(&tx)?; @@ -1080,31 +1076,11 @@ fn stage_items_to_upload( controller: &AbortController, upload_items: &[UploadItem], upload_tombstones: &[UploadTombstone], - weak_upload: &[nsString], ) -> Result<()> { debug!(driver, "Cleaning up staged items left from last sync"); controller.err_if_aborted()?; db.exec("DELETE FROM itemsToUpload")?; - debug!(driver, "Staging weak uploads"); - for chunk in weak_upload.chunks(db.variable_limit()?) { - let mut statement = db.prepare(format!( - "INSERT INTO itemsToUpload(id, guid, syncChangeCounter, parentGuid, - parentTitle, dateAdded, type, title, - placeId, isQuery, url, keyword, position, - tagFolderName) - {} - WHERE b.guid IN ({})", - UploadItemsFragment("b"), - repeat_sql_vars(chunk.len()), - ))?; - for (index, guid) in chunk.iter().enumerate() { - controller.err_if_aborted()?; - statement.bind_by_index(index as u32, nsString::from(guid.as_ref()))?; - } - statement.execute()?; - } - // Stage remotely changed items with older local creation dates. These are // tracked "weakly": if the upload is interrupted or fails, we won't // reupload the record on the next sync. diff --git a/toolkit/components/places/mozISyncedBookmarksMirror.idl b/toolkit/components/places/mozISyncedBookmarksMirror.idl index 3b0fe524bf25..0ceaeff5ad3f 100644 --- a/toolkit/components/places/mozISyncedBookmarksMirror.idl +++ b/toolkit/components/places/mozISyncedBookmarksMirror.idl @@ -91,7 +91,6 @@ interface mozISyncedBookmarksMerger : nsISupports { mozIPlacesPendingOperation merge( in long long localTimeSeconds, in long long remoteTimeSeconds, - in Array weakUploads, in mozISyncedBookmarksMirrorCallback callback ); diff --git a/toolkit/components/places/tests/sync/test_bookmark_chunking.js b/toolkit/components/places/tests/sync/test_bookmark_chunking.js index 1b3faccafebc..5ce37dfdcadf 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_chunking.js +++ b/toolkit/components/places/tests/sync/test_bookmark_chunking.js @@ -163,50 +163,3 @@ add_task(async function test_deletion_chunking() { await PlacesUtils.bookmarks.eraseEverything(); await PlacesSyncUtils.bookmarks.reset(); }); - -add_task(async function test_weak_upload_chunking() { - let buf = await openMirror("weak_upload_chunking"); - - info("Set up empty local tree"); - await PlacesTestUtils.markBookmarksAsSynced(); - - info("Set up remote tree with 1500 bookmarks"); - let toolbarRecord = makeRecord({ - id: "toolbar", - parentid: "places", - type: "folder", - children: [], - }); - let records = [toolbarRecord]; - for (let i = 0; i < 1500; ++i) { - let title = i.toString(10); - let guid = title.padStart(12, "B"); - toolbarRecord.children.push(guid); - records.push( - makeRecord({ - id: guid, - parentid: "toolbar", - type: "bookmark", - title, - bmkUri: "http://example.com/b", - }) - ); - } - await buf.store(shuffle(records)); - - info("Apply remote"); - let changesToUpload = await buf.apply({ - weakUpload: toolbarRecord.children, - }); - - let guidsToUpload = Object.keys(changesToUpload); - deepEqual( - guidsToUpload.sort(), - toolbarRecord.children.sort(), - "Should weakly upload records that haven't changed locally" - ); - - await buf.finalize(); - await PlacesUtils.bookmarks.eraseEverything(); - await PlacesSyncUtils.bookmarks.reset(); -}); diff --git a/toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js b/toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js deleted file mode 100644 index 005416c51107..000000000000 --- a/toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js +++ /dev/null @@ -1,140 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -add_task(async function test_explicit_weakupload() { - let buf = await openMirror("weakupload"); - - await PlacesUtils.bookmarks.insertTree({ - guid: PlacesUtils.bookmarks.menuGuid, - children: [ - { - guid: "mozBmk______", - url: "https://mozilla.org", - title: "Mozilla", - tags: ["moz", "dot", "org"], - }, - ], - }); - await storeRecords( - buf, - shuffle([ - { - id: "menu", - parentid: "places", - type: "folder", - children: ["mozBmk______"], - }, - { - id: "mozBmk______", - parentid: "menu", - type: "bookmark", - title: "Mozilla", - bmkUri: "https://mozilla.org", - tags: ["moz", "dot", "org"], - }, - ]), - { needsMerge: false } - ); - await PlacesTestUtils.markBookmarksAsSynced(); - - let changesToUpload = await buf.apply({ - weakUpload: ["mozBmk______"], - }); - - ok("mozBmk______" in changesToUpload); - equal(changesToUpload.mozBmk______.counter, 0); - - await buf.finalize(); - await PlacesUtils.bookmarks.eraseEverything(); - await PlacesSyncUtils.bookmarks.reset(); -}); - -add_task(async function test_explicit_weakupload_with_dateAdded() { - let buf = await openMirror("explicit_weakupload_with_dateAdded"); - - info("Set up mirror"); - let dateAdded = new Date(); - await PlacesUtils.bookmarks.insertTree({ - guid: PlacesUtils.bookmarks.menuGuid, - children: [ - { - guid: "mozBmk______", - url: "https://mozilla.org", - title: "Mozilla", - dateAdded, - }, - ], - }); - await storeRecords( - buf, - shuffle([ - { - id: "menu", - parentid: "places", - type: "folder", - children: ["mozBmk______"], - }, - { - id: "mozBmk______", - parentid: "menu", - type: "bookmark", - title: "Mozilla", - bmkUri: "https://mozilla.org", - dateAdded: dateAdded.getTime(), - }, - ]), - { needsMerge: false } - ); - await PlacesTestUtils.markBookmarksAsSynced(); - - info("Make remote change with older date added"); - await storeRecords(buf, [ - { - id: "mozBmk______", - parentid: "menu", - type: "bookmark", - title: "Firefox", - bmkUri: "http://getfirefox.com/", - dateAdded: dateAdded.getTime() + 5000, - }, - ]); - - info("Explicitly request changed item for weak upload"); - let changesToUpload = await buf.apply({ - weakUpload: ["mozBmk______"], - }); - deepEqual(changesToUpload, { - mozBmk______: { - tombstone: false, - counter: 0, - synced: false, - cleartext: { - id: "mozBmk______", - type: "bookmark", - title: "Firefox", - bmkUri: "http://getfirefox.com/", - parentid: "menu", - hasDupe: true, - parentName: "menu", - dateAdded: dateAdded.getTime(), - }, - }, - }); - - let localInfo = await PlacesUtils.bookmarks.fetch("mozBmk______"); - equal(localInfo.title, "Firefox", "Should take new title from mirror"); - equal( - localInfo.url.href, - "http://getfirefox.com/", - "Should take new URL from mirror" - ); - equal( - localInfo.dateAdded.getTime(), - dateAdded.getTime(), - "Should keep older local date added" - ); - - await buf.finalize(); - await PlacesUtils.bookmarks.eraseEverything(); - await PlacesSyncUtils.bookmarks.reset(); -}); diff --git a/toolkit/components/places/tests/sync/xpcshell.ini b/toolkit/components/places/tests/sync/xpcshell.ini index b8eaad5b6662..0f0953a83c1b 100644 --- a/toolkit/components/places/tests/sync/xpcshell.ini +++ b/toolkit/components/places/tests/sync/xpcshell.ini @@ -12,7 +12,6 @@ support-files = [test_bookmark_corruption.js] [test_bookmark_deduping.js] [test_bookmark_deletion.js] -[test_bookmark_explicit_weakupload.js] [test_bookmark_haschanges.js] [test_bookmark_kinds.js] [test_bookmark_mirror_meta.js]