Closed
Bug 1254797
Opened 8 years ago
Closed 8 years ago
Manually added sites to Top Sites are problematic (wrong indexing, can't remove/unpin, etc)
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Tracking
(firefox47+ fixed, firefox48+ fixed, fennec47+)
People
(Reporter: Grisha, Assigned: ahunt)
References
Details
Attachments
(4 files)
58 bytes,
text/x-review-board-request
|
rnewman
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
rnewman
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
rnewman
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
rnewman
:
review+
|
Details |
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).
Assignee | ||
Comment 1•8 years ago
|
||
(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
Assignee | ||
Comment 2•8 years ago
|
||
(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).
Assignee | ||
Comment 3•8 years ago
|
||
[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.
status-firefox47:
--- → ?
status-firefox48:
--- → affected
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
Seems like a common scenario we should be fixing in 47, tracked.
Updated•8 years ago
|
tracking-fennec: --- → 47+
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40239/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40239/
Attachment #8730924 -
Flags: review?(michael.l.comella)
Updated•8 years ago
|
Status: NEW → ASSIGNED
Component: General → Data Providers
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Assignee | ||
Comment 17•8 years ago
|
||
(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?)
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 18•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8730924 -
Flags: review?(michael.l.comella)
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c1f7b417f9e2 https://hg.mozilla.org/mozilla-central/rev/54080662c849 https://hg.mozilla.org/mozilla-central/rev/da1a66f1c4b7
Assignee | ||
Comment 20•8 years ago
|
||
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?
Assignee | ||
Comment 21•8 years ago
|
||
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?
Assignee | ||
Comment 22•8 years ago
|
||
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+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/87e35794cf88 https://hg.mozilla.org/releases/mozilla-aurora/rev/d6ec426e0c96 https://hg.mozilla.org/releases/mozilla-aurora/rev/876a1f819d83
Comment 25•8 years ago
|
||
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)
Assignee | ||
Comment 26•8 years ago
|
||
(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: 8 years ago
Flags: needinfo?(ahunt)
Resolution: --- → FIXED
Comment 27•8 years ago
|
||
Looks like this was fixed in 47+ a month ago.
Comment 28•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Updated•6 years ago
|
Flags: qe-verify+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•