Fennec Bookmark Management: folders removed are still displayed after sync

VERIFIED FIXED in Firefox 56

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: sflorean, Assigned: jwu)

Tracking

(Blocks 1 bug)

56 Branch
Firefox 56
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

Attachments

(1 attachment)

Environment: 
Device: 
- Asus Zenpad 8(Android 6.0.1);
- Motorola Nexus 6 (Android 7.0)
Build: Nightly 56.0a1 (2017-07-13);
Prerequisites: log in with the same account on both devices and have at least one folder TEST (8 folders has my account).

Steps to reproduce:
1. On device 1 long tap on "Test" and tap on "Remove";
3. Perform a sync on device 1;
4. Perform a sync on device 2 and go to Bookmarks Panel.

Expected result:
The folder "test" was removed and is not displayed on device 2.

Actual result:
The folder is still displayed.

Notes:
Reproducible 3/4.
I can reproduce this issue all the time. The root cause I found is that when deleting a folder, the lastModified timestamp isn't bumped and sync didn't know to update the corresponding bookmark records.


Hi Grisha,

Please correct me if I was wrong, I would file a patch later that looks like fixing this bug, at least in my local environment.
Assignee: nobody → topwu.tw
Flags: needinfo?(gkruglov)
Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

https://reviewboard.mozilla.org/r/158714/#review164908

I think this should do the trick, and it's unfortunate that we missed it on the first pass. (sidenote: this type of tracking will be superceded by Bug 1364644 soon). But, please add unit tests that cover this bug, and similar deletion-related scenarios! r- so that I look at the tests once you push them up.
Attachment #8887801 - Flags: review?(gkruglov) → review-
Flags: needinfo?(gkruglov)
Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

https://reviewboard.mozilla.org/r/158714/#review165232

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java:164
(Diff revision 2)
> +                                                                   new String[] { BrowserContract.Bookmarks.MOBILE_FOLDER_GUID });
> +        final long folderLastModifiedAfterDeletion = getLastModified(withDeleted(folderUri));
> +
> +        // Check both parent & folder's last modified timestamps are increased.
> +        assertTrue(rootLastModifiedAfterDeletion > rootLastModifiedBeforeDeletion);
> +        assertTrue(folderLastModifiedAfterDeletion > folderLastModifiedBeforeDeletion);

Can you also add a test which checks this behaviour in presence of a deeper nesting structure and siblings?

Something like this:

root
-> folder1
-> folder2 -- timestamp must change
--> folder3
--> folder4 -- delete this one

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java:284
(Diff revision 2)
> +                                                    null);
> +        assertNotNull(cursor);
> +
> +        long lastModified = INVALID_TIMESTAMP;
> +        try {
> +            assertTrue(cursor.moveToFirst());

You probably also want to `assertEquals(1, cursor.getCount())`, to ensure the caller is doing the right thing.
Attachment #8887801 - Flags: review?(gkruglov) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8e12f9e0619f
Bump the folder's last modified timestamp when deleting it. r=Grisha
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e12f9e0619f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Verified as fixed on build 56.0a1 (2017-07-26);
Device: LG G4 (Android 6.0.1), Huawei MediaPad M2 (Android 5.1.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.