Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-27 14:52:16 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1702686 - Removed all instances/implementations of weak upload. r=markh

Browse Source 
Differential Revision: https://phabricator.services.mozilla.com/D113718
This commit is contained in:
Sammy Khamis 2021-04-29 03:43:16 +00:00
parent 992351bc5c
commit 91a0120e03
13 changed files with 5 additions and 297 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@ -420,7 +420,6 @@ dependencies = [
"nsstring",
"storage",
"storage_variant",
"thin-vec",
"url",
"xpcom",
]

42
services/sync/modules/engines.js
View File

@ -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();
},
/**

5
services/sync/modules/engines/bookmarks.js
View File

@ -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(

3
services/sync/tests/unit/test_bookmark_engine.js
View File

@ -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(

1
services/sync/tests/unit/test_bookmark_tracker.js
View File

@ -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();

21
toolkit/components/places/SyncedBookmarksMirror.jsm
View File

@ -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 {

4
toolkit/components/places/bookmark_sync/Cargo.toml
View File

@ -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"]

10
toolkit/components/places/bookmark_sync/src/merger.rs
View File

@ -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<nsString>>,
callback: &mozISyncedBookmarksMirrorCallback,
) -> Result<RefPtr<mozIPlacesPendingOperation>, 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<ThreadPtrHandle<mozIServicesLogSink>>,
local_time_millis: i64,
remote_time_millis: i64,
weak_uploads: Vec<nsString>,
progress: Option<ThreadPtrHandle<mozISyncedBookmarksMirrorProgressListener>>,
callback: ThreadPtrHandle<mozISyncedBookmarksMirrorCallback>,
result: AtomicRefCell<error::Result<store::ApplyStatus>>,
@ -141,7 +134,6 @@ impl MergeTask {
logger: Option<RefPtr<mozIServicesLogSink>>,
local_time_seconds: i64,
remote_time_seconds: i64,
weak_uploads: Vec<nsString>,
callback: RefPtr<mozISyncedBookmarksMirrorCallback>,
) -> Result<MergeTask, nsresult> {
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()?;

26
toolkit/components/places/bookmark_sync/src/store.rs
View File

@ -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<ApplyStatus> {
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.

1
toolkit/components/places/mozISyncedBookmarksMirror.idl
View File

@ -91,7 +91,6 @@ interface mozISyncedBookmarksMerger : nsISupports {
mozIPlacesPendingOperation merge(
in long long localTimeSeconds,
in long long remoteTimeSeconds,
in Array<AString> weakUploads,
in mozISyncedBookmarksMirrorCallback callback
);

47
toolkit/components/places/tests/sync/test_bookmark_chunking.js
View File

@ -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();
});

140
toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js
View File

@ -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();
});

1
toolkit/components/places/tests/sync/xpcshell.ini
View File

@ -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]