Closed Bug 1381001 Opened 5 years ago Closed 5 years ago

Fennec Bookmark Management: folders removed are still displayed after sync


(Firefox for Android Graveyard :: General, defect)

56 Branch
Not set


(firefox56 verified)

Firefox 56
Tracking Status
firefox56 --- verified


(Reporter: sflorean, Assigned: jwu)




(1 file)

- 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.

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 →
Flags: needinfo?(gkruglov)
Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

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.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/
(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:

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

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/
(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
Bump the folder's last modified timestamp when deleting it. r=Grisha
Keywords: checkin-needed
Closed: 5 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).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.