From b5512a8ae71496e6370e92b63b816fc0c03582a1 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Wed, 18 Feb 2026 15:11:12 +1100 Subject: [PATCH] Bug 1908824 - bookmark sync should not always consider bookmarks as modified "now" after being reconciled. When applying remote bookmarks to the local database, lastModified was set to `MAX(v.dateAdded, {now})` which in practice is always "now". This caused all reconciled items appear "recently modified locally". This in turn caused a sync after a reset to often re-upload all bookmarks as the local bookmark "won" due to its recent lastModified, even though it had never actually changed. But a lastModified timestamp is not in the sync payload - it's purely a local concept. So while it's not perfect, we use the server's modified timestamp - that's likely to be close to when the record was most recently modified somewhere. The applyNewLocalStructure trigger was also problematic as this updated the timestamp to `now()`, and this trigger executed for new incoming items. This meant that the timestamp of an item might move backwards - it would end up as `now()` as it was created, but move backwards to the server timestamp if reapplied. There's no good reason for this trigger to change the timestamp - if the item moved parents remotely then it will have a new server timestamp. A desktop patch landing under bug 2018096 fixes the same problem there. --- .../places/sql/create_sync_temp_tables.sql | 3 +- .../places/sql/create_sync_triggers.sql | 3 +- components/places/src/bookmark_sync/engine.rs | 109 +++++++++++++++++- 3 files changed, 106 insertions(+), 9 deletions(-) diff --git a/components/places/sql/create_sync_temp_tables.sql b/components/places/sql/create_sync_temp_tables.sql index c7a60fd61a..1d2ecfd322 100644 --- a/components/places/sql/create_sync_temp_tables.sql +++ b/components/places/sql/create_sync_temp_tables.sql @@ -41,8 +41,7 @@ CREATE TEMP TABLE applyNewLocalStructureOps( mergedGuid TEXT PRIMARY KEY, mergedParentGuid TEXT NOT NULL, position INTEGER NOT NULL, - level INTEGER NOT NULL, - lastModified INTEGER NOT NULL -- In milliseconds. + level INTEGER NOT NULL ) WITHOUT ROWID; -- Stores locally changed items staged for upload. diff --git a/components/places/sql/create_sync_triggers.sql b/components/places/sql/create_sync_triggers.sql index 338a70982b..d1e876327c 100644 --- a/components/places/sql/create_sync_triggers.sql +++ b/components/places/sql/create_sync_triggers.sql @@ -63,7 +63,6 @@ BEGIN UPDATE moz_bookmarks SET parent = (SELECT id FROM moz_bookmarks WHERE guid = OLD.mergedParentGuid), - position = OLD.position, - lastModified = OLD.lastModified + position = OLD.position WHERE guid = OLD.mergedGuid; END; diff --git a/components/places/src/bookmark_sync/engine.rs b/components/places/src/bookmark_sync/engine.rs index 633320e1de..4d5097f8d1 100644 --- a/components/places/src/bookmark_sync/engine.rs +++ b/components/places/src/bookmark_sync/engine.rs @@ -163,7 +163,7 @@ fn update_local_items_in_places( SELECT n.mergedGuid, b.id, v.id, v.guid, n.level, n.remoteType, b.dateAdded, v.dateAdded, - MAX(v.dateAdded, {now}), b.title, v.title, + v.serverModified, b.title, v.title, b.fk, v.placeId, v.keyword FROM ops n @@ -178,7 +178,6 @@ fn update_local_items_in_places( op.level ) }), - now = now, ); // We can't avoid allocating here, since we're binding four @@ -264,13 +263,12 @@ fn update_local_items_in_places( |chunk, _| -> Result<()> { let sql = format!( "INSERT INTO applyNewLocalStructureOps( - mergedGuid, mergedParentGuid, position, level, - lastModified + mergedGuid, mergedParentGuid, position, level ) VALUES {}", sql_support::repeat_display(chunk.len(), ",", |index, f| { let op = &chunk[index]; - write!(f, "(?, ?, {}, {}, {})", op.position, op.level, now) + write!(f, "(?, ?, {}, {})", op.position, op.level) }), ); @@ -3919,6 +3917,107 @@ mod tests { Ok(()) } + #[test] + fn test_incoming_timestamps() -> anyhow::Result<()> { + let api = new_mem_api(); + let reader = api.open_connection(ConnectionType::ReadOnly)?; + + // The timestamp of each record on the server. We expect this to be our "last modified". + let remote_modified = ServerTimestamp::from_millis(1770000000000); + + let records = vec![ + json!({ + "id": "toolbar", + "type": "folder", + "parentid": "places", + "parentName": "", + "dateAdded": 0, + "title": "toolbar", + "children": ["folderAAAAAA"], + }), + json!({ + "id": "folderAAAAAA", + "type": "folder", + "parentid": "toolbar", + "parentName": "toolbar", + "dateAdded": 0, + "title": "A", + "children": ["bookmarkBBBB"], + }), + json!({ + "id": "bookmarkBBBB", + "type": "bookmark", + "parentid": "folderAAAAAA", + "parentName": "A", + "dateAdded": 0, + "title": "A", + "bmkUri": "http://example.com/a", + }), + ]; + + let engine = create_sync_engine(&api); + + let incoming = records + .clone() + .into_iter() + .map(|json| IncomingBso::from_test_content_ts(json, remote_modified)) + .collect(); + + engine_apply_incoming(&engine, incoming); + + // This was the first creation of the bookmark, the lastModified should be the server timestamp. + let bm = get_raw_bookmark(&reader, &SyncGuid::new("bookmarkBBBB")) + .expect("must work") + .expect("must exist"); + + assert_eq!( + bm.date_modified.as_millis_i64(), + remote_modified.as_millis() + ); + + // Now reset the engine and do it again, which should not adjust date_modified. + engine.reset(&EngineSyncAssociation::Disconnected)?; + let incoming = records + .into_iter() + .map(|json| IncomingBso::from_test_content_ts(json, remote_modified)) + .collect(); + engine_apply_incoming(&engine, incoming); + let bm = get_raw_bookmark(&reader, &SyncGuid::new("bookmarkBBBB")) + .expect("must work") + .expect("must exist"); + + assert_eq!( + bm.date_modified.as_millis_i64(), + remote_modified.as_millis() + ); + + // applying a change to the bookmark should update it. + let new_records = vec![json!({ + "id": "bookmarkBBBB", + "type": "bookmark", + "parentid": "folderAAAAAA", + "parentName": "A", + "dateAdded": 0, + "title": "A", + "bmkUri": "http://example.com/a", + })]; + let new_remote_modified = ServerTimestamp::from_millis(1780000000000); + let new_incoming = new_records + .into_iter() + .map(|json| IncomingBso::from_test_content_ts(json, new_remote_modified)) + .collect(); + engine_apply_incoming(&engine, new_incoming); + let bm = get_raw_bookmark(&reader, &SyncGuid::new("bookmarkBBBB")) + .expect("must work") + .expect("must exist"); + + assert_eq!( + bm.date_modified.as_millis_i64(), + new_remote_modified.as_millis() + ); + Ok(()) + } + #[test] fn test_dedupe_local_newer() -> anyhow::Result<()> { let api = new_mem_api();