731267 - Ignore "All Bookmarks" folder and its badly rooted parent

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Tracy Walker [:tracy]]

Select Bookmarks from the Fennec awesomebar. Notice inside there is a folder that has no title.  Inside that folder is an "All Bookmarks" folder.  Inside that folder is just text "All Bookmarks"

I think this came from desktop.  Should this be populated or not synced in the first place?

Comment 1

[Richard Newman [:rnewman]]

I wonder if this is the same as Bug 730350….

Comment 2

[Tracy Walker [:tracy]]

Possibly, but the untitled folder(with All Bookmarks" inside) that I am seeing is at the same level as the Mobile folder, not inside it.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Tracy Walker [:tracy] from comment #2)
> Possibly, but the untitled folder(with All Bookmarks" inside) that I am
> seeing is at the same level as the Mobile folder, not inside it.

We rearranged the database schema, and adjusted how bookmarks are presented, since that bug.

Comment 4

[Tracy Walker [:tracy]]

Ok, dupes; probably that one of this since what I'm reporting is current behavior.

Comment 6

[Tony Chung [:tchung]]

Attachment: screenshot

I think this should at least be a P2.  Looking at this view gives a lousy user experience of not knowing what's in this folder.

Comment 7

[Richard Newman [:rnewman]]

Just to clarify:

Tony, Tracy: do you have STR from a "fresh" (has never synced with Fennec) desktop profile and a fresh (starting from not installed at all) Fennec profile?

Comment 8

[Tony Chung [:tchung]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Just to clarify:
> 
> Tony, Tracy: do you have STR from a "fresh" (has never synced with Fennec)
> desktop profile and a fresh (starting from not installed at all) Fennec
> profile?

Yeah thats what i did for my screenshot.  

STR: 
1) install clean nightly on fennec  (03-05-2012 build)
2) create a clean firefox desktop profile, and bookmark a few sites.  Bookmarked to Bookmarks Menu, Unsorted Bookmarks, create and name a folder within Bookmarks Menu. Even add tags and such.
3) setup a new sync account on desktop, and sync your bookmarked changes.
4) back to fennec, launch Sync > use Jpake, and watch everything sync over
5) Verify empty bookmark folder

Comment 9

[Tony Chung [:tchung]]

Attachment: screenshot of empty folder

Here's what it looks like after you click the titleless folder.   It takes me to an "All Bookmarks" view.  Then if you open that folder, you get a blank entry.

Comment 10

[Allison Naaktgeboren :ally]

triage: moving to p2, not a sync issue, but bad ux caused by the data from fennec

Comment 11

[Richard Newman [:rnewman]]

(In reply to :Ally Naaktgeboren from comment #10)
> triage: moving to p2, not a sync issue, but bad ux caused by the data from
> fennec

Not sure if that's true; haven't determined a root cause yet. Unless Tony can repro without involving Sync, I'm inclined to point the finger at me...

Comment 12

[Matt Brubeck (:mbrubeck)]

Marking this as blocking Fennec Native release; we want to try to eliminate any cases of incorrect UX related to Sync even if they are mostly harmless/cosmetic, because we want users to feel that sync is 100% trustworthy.

Comment 13

[Richard Newman [:rnewman]]

See <https://bugzilla.mozilla.org/show_bug.cgi?id=731273#c27>.

We should stop these records from being uploaded, but we have to handle the ones that are there.

Comment 14

[Richard Newman [:rnewman]]

We'll either do this in Sync or (as gcp and I discussed today) in the Fennec bookmarks UI, by only showing the known special roots. Margaret?

Comment 15

[Richard Newman [:rnewman]]

I spent a little more time considering this.

If Sync deletes any part of the additional tree structure, we run the risk of dumping subsequent records into Unsorted Bookmarks.

For example, if we delete on-the-fly, you might get "All Bookmarks" show up as a folder in Unsorted Bookmarks, because we deleted its parent before we added it.

If we clean up at the end of a sync, you'll get transient records in the DB (we'll add, then delete), and if the sync is interrupted or new records arrive later, they'll get dumped into Unsorted Bookmarks just as if we deleted on-the-fly.

The only safe thing we can do is store the records in the DB, in order to preserve the tree structure to "catch" subsequent records.

We also don't particularly want to flag these records as deleted; if we do that, then any subsequent upload will attempt to delete them on other machines.

The correct course of action, then, is for Fennec to ignore roots that it doesn't care about.

Over the wall!

Comment 16

[:Margaret Leibovic]

Attachment: patch

This patch special-cases the FIXED_ROOT_ID and returns only the folders we care about in that case. This also let me remove the code we were using to ignore the root folder and the tags folder in our results.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 605980 [details] [diff] [review]
patch

Review of attachment 605980 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/LocalBrowserDB.java
@@ +314,5 @@
> +
> +        if (folderId == Bookmarks.FIXED_ROOT_ID) {
> +            // Because of sync, we can end up with some additional records under
> +            // the root node that we don't want to see. Since sync doesn't 
> +            // want to run into problems deleing these, we can just ignore them

deleting

@@ +321,5 @@
> +                         DEFAULT_BOOKMARK_COLUMNS,
> +                         Bookmarks.GUID + " = ? OR " +
> +                         Bookmarks.GUID + " = ? OR " +
> +                         Bookmarks.GUID + " = ? OR " +
> +                         Bookmarks.GUID + " = ?",

nit: I'd still keep a Bookmarks.PARENT = folderId in the query just to make sure we're still doing the right thing even with the specific set of folders.

Comment 18

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d19579ec69b

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/2d19579ec69b

Comment 20

[:Margaret Leibovic]

Comment on attachment 605980 [details] [diff] [review]
patch

[Approval Request Comment]
Mobile-only. Blocking fennec-1.0.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 605980 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 13.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c58ae6ebbd

Comment 23

[Paul Feher]

Verified/fixed on:

Aurora Fennec 13.0a2 (2012-03-26)
Nightly Fennec 14.0a1 (2012-03-26)
Device: Samsung Nexus S
OS: Android 2.3.6