Bug 1908824 - bookmark sync should not always consider bookmarks as m…#7230
Open
mhammond wants to merge 1 commit intomozilla:mainfrom
Open
Bug 1908824 - bookmark sync should not always consider bookmarks as m…#7230mhammond wants to merge 1 commit intomozilla:mainfrom
mhammond wants to merge 1 commit intomozilla:mainfrom
Conversation
…odified "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.
mhammond
force-pushed
the
push-kmszwlxnuynx
branch
from
0cf585a to
cdcd600
Compare
mhammond
changed the title
skhamis
approved these changes
Feb 21, 2026
Contributor
skhamis
left a comment
skhamis
left a comment
There was a problem hiding this comment.
Nice! This is quite elegant, also no migrations needed as both tables are `TEMP. LGTM!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…odified "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 asnow()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 the same bug number fixes the same problem there.
Pull Request checklist
[ci full]to the PR title.