Closed Bug 1254797 Opened 4 years ago Closed 4 years ago

Manually added sites to Top Sites are problematic (wrong indexing, can't remove/unpin, etc)

Categories

(Firefox for Android :: Data Providers, defect, major)

All
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
firefox47 + fixed
firefox48 + fixed
fennec 47+ ---

People

(Reporter: Grisha, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

To replicate:
1) on a fresh fennec install, remove one of the default sites (e.g. amazon) to free up room, and add a couple of sites manually via "Add a site".
2) try removing and unpinning added sites
3) repeat steps 1 and 2 in random order for a minute or two

You should notice that sometimes manually added sites won't remove at all. Sometimes a manually added site will disappear, but not the one you were trying to remove. Unpinning either doesn't work at all, or behaves as described below.

Additionally, I'm not sure if Pinning/Unpinning is behaving correctly. If I pin a "default" top site, and then unpin it, it I just see its "pin state" toggle. However, if I add a site manually, pin it, and then unpin it, this would remove the site entirely (same as if I pressed "Remove" instead).
(In reply to :Grisha Kruglov from comment #0)
> To replicate:
> 1) on a fresh fennec install, remove one of the default sites (e.g. amazon)
> to free up room, and add a couple of sites manually via "Add a site".
> 2) try removing and unpinning added sites
> 3) repeat steps 1 and 2 in random order for a minute or two
> 
> You should notice that sometimes manually added sites won't remove at all.
> Sometimes a manually added site will disappear, but not the one you were
> trying to remove. Unpinning either doesn't work at all, or behaves as
> described below.

Uggh, I just realised we probably have to insert blank tiles in the middle of our main topsites cursor. That's fallout from Bug 760956.

A simpler way to reproduce that aspect of the bug is:
1) Start with a clean profile
2) Pin the 5th suggested site
3) Remove any of the sites 1-4
-> the pinned site starts wandering upwards into position 4 (and goes further if you remove more sites)

[4) start adding history by visiting sites, and the pinned site starts wandering towards its correct position...]

We could solve this maybe by stuffing N "blank" tiles into our temporary TOPSITES table in BrowserProvider after stuffing the suggested sites, where N = max(pinned_sites.position) - count(topsites_table_after_inserting_top_and_suggested)?

> Additionally, I'm not sure if Pinning/Unpinning is behaving correctly. If I
> pin a "default" top site, and then unpin it, it I just see its "pin state"
> toggle. However, if I add a site manually, pin it, and then unpin it, this
> would remove the site entirely (same as if I pressed "Remove" instead).

This is intentional: we don't remove a "suggested" site unless the user explicitly uses the remove menu item. The topsites consists of (A) pinned sites, inserted in their appropriate positions, followed by the gaps being filled with (B) top history (ordered by frecency), and (C) suggested sites if the grid isn't full. Pinned sites (A) can be added manually, but generally a pinned site will have come from either (B) or (C), so unpinning it means it reappears as a top-history or suggested-site - it only disappears if it was manually added (or if the history ordering has changed since the original pinning, in which case some other site might take its spot). In fact if you pin a site and unpin it, and there have been no new history visits / etc, the ordering in topsites shouldn't change, only the pinned status of the site being pinned/unpinned.
Assignee: nobody → ahunt
(In reply to Andrzej Hunt :ahunt from comment #1)
> (In reply to :Grisha Kruglov from comment #0)
> > To replicate:
> > 1) on a fresh fennec install, remove one of the default sites (e.g. amazon)
> > to free up room, and add a couple of sites manually via "Add a site".
> > 2) try removing and unpinning added sites
> > 3) repeat steps 1 and 2 in random order for a minute or two
> > 
> > You should notice that sometimes manually added sites won't remove at all.
> > Sometimes a manually added site will disappear, but not the one you were
> > trying to remove. Unpinning either doesn't work at all, or behaves as
> > described below.
> 
> Uggh, I just realised we probably have to insert blank tiles in the middle
> of our main topsites cursor. That's fallout from Bug 760956.
> 
> A simpler way to reproduce that aspect of the bug is:
> 1) Start with a clean profile
> 2) Pin the 5th suggested site
> 3) Remove any of the sites 1-4
> -> the pinned site starts wandering upwards into position 4 (and goes
> further if you remove more sites)
> 
> [4) start adding history by visiting sites, and the pinned site starts
> wandering towards its correct position...]
> 
> We could solve this maybe by stuffing N "blank" tiles into our temporary
> TOPSITES table in BrowserProvider after stuffing the suggested sites, where
> N = max(pinned_sites.position) -
> count(topsites_table_after_inserting_top_and_suggested)?

Just to explain a bit more: once we get into this weird state where the physical location on screen doesn't match the pinned-position (as stored in the DB), things get really weird, because (I think) we grab the physical position, and then try to delete whichever pinned-site has that as its pinned-position in the DB.

So in addition to the changes above, we should probably make our unpinning more robust and pass the DBs pinned-position into our deletion method if at all possible (instead of passing in the physical position). Alternatively we might be able to use the bookmark ID (since pinned sites are in the bookmarks table).
[Tracking Requested - why for this release]: This bug was probably introduced in 47, where Bug 760956 landed.

This seems to be a hopefully rare edge case, which should only happen when there isn't enough history to fill the topsites grid, and only if suggested sites / history are manually deleted after one or more sites have been pinned.
Seems like a common scenario we should be fixing in 47, tracked.
tracking-fennec: --- → 47+
This variable was renamed to be positive instead of negative but the value/usage wasn't adapted.

Review commit: https://reviewboard.mozilla.org/r/40233/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40233/
This fixes an edge case that is most likely to happen to new users if they pin a site followed
by removing one or more suggested sites. This results in the topsites table containing
less sites than needed, leading to some pinned sites being displayed in a higher
than expected position. This also broke unpinning since our code assumes
that a topsites physical position corresponds to its DB position (which prior
to this patch was not the case).

Review commit: https://reviewboard.mozilla.org/r/40235/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40235/
Attachment #8730922 - Flags: review?(rnewman)
Pinned sites should be deleted directly, however I'm not confident enough
in my knowledge of sync to be certain that we won't end up with deleted
pinned sites in our table.

(We use normal bookmark deletion for removing pinned sites.)

Review commit: https://reviewboard.mozilla.org/r/40237/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40237/
Attachment #8730923 - Flags: review?(rnewman)
Status: NEW → ASSIGNED
Component: General → Data Providers
Comment on attachment 8730922 [details]
MozReview Request: Bug 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman

https://reviewboard.mozilla.org/r/40235/#review37107

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:808
(Diff revision 1)
> +            } else {
> +                hasPreparedBlankTiles = true;
> +            }
> +
> +            blanksBuilder.append(" SELECT" +
> +                                 " ? AS " + Bookmarks._ID + "," +

Let's be reasonable people here.

```
blanksBuilder.append(" SELECT -1 AS " + Bookmarks._ID + ", '' AS " …);
```

Skip the args nonsense altogether.
Attachment #8730922 - Flags: review?(rnewman) → review+
Comment on attachment 8730921 [details]
MozReview Request: Bug 1254797 - Pre: fix variable name being negation of value r=me

https://reviewboard.mozilla.org/r/40233/#review37163
Attachment #8730921 - Flags: review+
Comment on attachment 8730923 [details]
MozReview Request: Bug 1254797 - Post: Ensure we ignore deleted sites in pinned query r=rnewman

https://reviewboard.mozilla.org/r/40237/#review37165

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:718
(Diff revision 1)
>              suggestedGridLimit = Integer.parseInt(gridLimitParam, 10);
>          }
>  
> -        final String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " + Bookmarks.PARENT + " == " + Bookmarks.FIXED_PINNED_LIST_ID;
> +        final String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " +
> +                                             Bookmarks.PARENT + " == " + Bookmarks.FIXED_PINNED_LIST_ID +
> +                                             " AND " + Bookmarks.IS_DELETED + " IS NOT -1";

We use 1 for deleted, 0 for not deleted. Where did -1 come from?
Attachment #8730923 - Flags: review?(rnewman)
Comment on attachment 8730922 [details]
MozReview Request: Bug 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40235/diff/1-2/
Comment on attachment 8730923 [details]
MozReview Request: Bug 1254797 - Post: Ensure we ignore deleted sites in pinned query r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40237/diff/1-2/
Attachment #8730923 - Flags: review?(rnewman)
Comment on attachment 8730924 [details]
MozReview Request: Bug 1254797 - Post: assert that pinned position matches physical position in top sites r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40239/diff/1-2/
Comment on attachment 8730923 [details]
MozReview Request: Bug 1254797 - Post: Ensure we ignore deleted sites in pinned query r=rnewman

https://reviewboard.mozilla.org/r/40237/#review37747
Attachment #8730923 - Flags: review?(rnewman) → review+
Comment on attachment 8730924 [details]
MozReview Request: Bug 1254797 - Post: assert that pinned position matches physical position in top sites r?mcomella

https://reviewboard.mozilla.org/r/40239/#review37749

Interested to see if this bites us in the wild! Keep an eye on Nightly crash reports.
Attachment #8730924 - Flags: review+
(In reply to Richard Newman [:rnewman] from comment #16)
> Comment on attachment 8730924 [details]
> MozReview Request: Bug 1254797 - Post: assert that pinned position matches
> physical position in top sites r?mcomella
> 
> https://reviewboard.mozilla.org/r/40239/#review37749
> 
> Interested to see if this bites us in the wild! Keep an eye on Nightly crash
> reports.

Hmm, on second thoughts I'm worried we're quite likely to hit it (on nightly or aurora): thepinned list could be corrupted as a result of this bug, i.e. there might be multiple pinned sites with the same pinned id. The first of these should now be positioned correctly, the duplicate would have an incorrect position, and would trigger this assert if they try to edit that site.

I'll hold off on landing this part for now, I'm wondering if we'd have to sanitise pinned sites once on nightly/aurora (either bump duplicate ids to ensure uniqueness, or just discard duplicates). That could probably be done as another DB migration, or we could move the assert earlier (into TopSitesGridAdapter?), run a sanitisation the first time the positions don't match, and only fire the assert if the positions still don't match? (We'd probably want that only on nightly/aurora?)
Keywords: leave-open
https://hg.mozilla.org/integration/fx-team/rev/c1f7b417f9e26427dc17e870d0a741bf4d231764
Bug 1254797 - Pre: fix variable name being negation of value r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/54080662c8491231102e0e81144b8fa6ddcee3d7
Bug 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/da1a66f1c4b74c4bd778e91b9589d73763dddfe3
Bug 1254797 - Post: Ensure we ignore deleted sites in pinned query r=rnewman
Attachment #8730924 - Flags: review?(michael.l.comella)
Comment on attachment 8730921 [details]
MozReview Request: Bug 1254797 - Pre: fix variable name being negation of value r=me

Approval Request Comment
[Feature/regressing bug #]: Bug 760956
[User impact if declined]: It may be impossible to unpin sites on profiles with few history items and/or incorrect items will be unpinned.
[Describe test coverage new/current, TreeHerder]: manual testing, patches have landed in nightly without issues.
[Risks and why]: Medium risk: these commits result in additional (blank) items being added to our temporary topsites table, ie. we are performing additional work when preapring topsites.
[String/UUID change made/needed]: none.
Attachment #8730921 - Flags: approval-mozilla-aurora?
Comment on attachment 8730922 [details]
MozReview Request: Bug 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman

Approval Request Comment
[Feature/regressing bug #]: Bug 760956
[User impact if declined]: It may be impossible to unpin sites on profiles with few history items and/or incorrect items will be unpinned.
[Describe test coverage new/current, TreeHerder]: manual testing, patches have landed in nightly without issues.
[Risks and why]: Medium risk: these commits result in additional (blank) items being added to our temporary topsites table, ie. we are performing additional work when preapring topsites.
[String/UUID change made/needed]: none.
Attachment #8730922 - Flags: approval-mozilla-aurora?
Comment on attachment 8730923 [details]
MozReview Request: Bug 1254797 - Post: Ensure we ignore deleted sites in pinned query r=rnewman

Approval Request Comment
[Feature/regressing bug #]: Bug 760956
[User impact if declined]: It may be impossible to unpin sites on profiles with few history items and/or incorrect items will be unpinned.
[Describe test coverage new/current, TreeHerder]: manual testing, patches have landed in nightly without issues.
[Risks and why]: Medium risk: these commits result in additional (blank) items being added to our temporary topsites table, ie. we are performing additional work when preapring topsites.
[String/UUID change made/needed]: none.
Attachment #8730923 - Flags: approval-mozilla-aurora?
Flags: qe-verify+
Comment on attachment 8730921 [details]
MozReview Request: Bug 1254797 - Pre: fix variable name being negation of value r=me

regression fix that was tested locally, Aurora47+
Attachment #8730921 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8730922 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8730923 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Given that some of the patches here have already been uplifted to aurora, I think we should close this bug and break off the remaining work into a follow-up bug.
Flags: needinfo?(ahunt)
Blocks: 1264646
(In reply to :Margaret Leibovic from comment #25)
> Given that some of the patches here have already been uplifted to aurora, I
> think we should close this bug and break off the remaining work into a
> follow-up bug.

Filed as Bug 1264646. Closing this bug since we've fixed the underlying cause (the new bug just deals with cleanup).
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(ahunt)
Resolution: --- → FIXED
Looks like this was fixed in 47+ a month ago.
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.