496542 - Duplicate entries in History sidebar and Library

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[John P Baker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre

If you use 'Most Visited' to go to a history entry the sidebar shows more than one entry for that page (probably one for each visit made previously)

Reproducible: Always

Steps to Reproduce:
1. View Sidebar / History, set 'By Last Visited', close sidebar
2. Go to http://planet.mozilla.org/ from location bar
3. Go to http://www.bristol.ac.uk/ from location bar
4. repeat 2 and 3 a few times possibly with an occasional extra site
5. From "Most Visited" select "Planet Mozilla"
6. View Sidebar / History
Actual Results:  
"Planet Mozilla" appears more than once at the top of the list

Expected Results:  
"Planet Mozilla" to appear once

Comment 1

[Marco Bonardo [:mak]]

confirmed, really originally i was recalling by last visited was not grouping pages, but it is. sounds like something is broken in updating the result in one of the 2 cases.

Comment 2

[John P Baker]

If you leave a couple of minutes between steps 5 and 6 (or AFAICT viewing the history sidebar again after more than a couple of minutes have passed) the results are correct - which might imply that the temp tables are involved.

Comment 3

[John P Baker]

I have just encountered a slightly different version of this after restarting the browser to a saved session; I am having problems reducing this to STR but it looks as if a non-cache page in a tab in a saved session will show multiple entries at that entries location in the sidebar (ie the bug is not only for the first entry)

Comment 4

[Ildar Nurislamov]

Not only in Sidebar but in "Show All History" view too.
So, on "Today" list you can see page occurrence multiplied by ~5, on Last 7 day multplied ~10 .. and so on. 
Sometimes bug happens, sometimes not.

Comment 6

[Drew Willcoxon :adw]

Note that this bug can be triggered by clicking the home button multiple times too, as in bug 498335.  The method of navigating to pages (i.e., using Most Visited as indicated in comment 0 or not) most likely doesn't matter.

Comment 7

[Alice0775 White]

Regression range:

Woks Fine:
http://hg.mozilla.org/mozilla-central/rev/a1fc27041e53
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090514 Minefield/3.6a1pre ID:20090514043824

Broken:
http://hg.mozilla.org/mozilla-central/rev/b766e9dc2335
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090515 Minefield/3.6a1pre ID:20090515044249

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a1fc27041e53&tochange=b766e9dc2335

Comment 8

[Alice0775 White]

Regression range in 1.9.1:

Works fine:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e49c05fc9122
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090519 Shiretoko/3.5b5pre ID:20090519043912

Broken:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/acd2d4638228
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre ID:20090520041838

Pushlog:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=e49c05fc9122&tochange=acd2d4638228

Comment 9

[John P Baker]

This could implicate bug 488966; One odd thing about the patch in that bug is that the following phrase in the old code

-            "SELECT id, url, title, rev_host, visit_count, visit_date, "
-              "favicon_url, session, empty "
-            "FROM ( "
-              "SELECT h.id AS id, h.url AS url, h.title AS title, "
-                "h.rev_host AS rev_host, h.visit_count AS visit_count, "
-                "MAX(v.visit_date) AS visit_date, f.url AS favicon_url, "
-                "v.session AS session, null AS empty "
-              "FROM moz_places_temp h "
-              "JOIN moz_historyvisits v ON h.id = v.place_id "
-              "LEFT JOIN moz_favicons f ON h.favicon_id = f.id "
-              "WHERE h.hidden <> 1 AND v.visit_type NOT IN ") +
-                nsPrintfCString("(0,%d) ",
-                                nsINavHistoryService::TRANSITION_EMBED) +
-                NS_LITERAL_CSTRING("{ADDITIONAL_CONDITIONS} "
-                "AND h.id NOT IN (SELECT id FROM moz_places_temp) "
-              "GROUP BY h.id "
-            ") "

is, AFAICT, a no-op as by definition h.id is an 'id FROM moz_places_temp'. This is no longer the case in the new code.

+        "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
+        "h.last_visit_date, f.url, v.session, null "
+        "FROM moz_places_temp h "
+        "JOIN moz_historyvisits v ON h.id = v.place_id "
+        "LEFT JOIN moz_favicons f ON h.favicon_id = f.id "
+        // WHERE 1 is a no-op since additonal conditions will start with AND.
+        "WHERE 1 "
+          "{QUERY_OPTIONS_VISITS} {QUERY_OPTIONS_PLACES} "
+          "{ADDITIONAL_CONDITIONS} "

Comment 10

[Marco Bonardo [:mak]]

yes, it is for sure a problem with table partitioning and the query. And i second you on the fact the old query was executing a no-op.

Comment 11

[Marco Bonardo [:mak]]

ugh, the GROUP BY is not applied to global UNIONed result, but only to a single query :\ this applies to all selectAsURI queries.
Since actually those queries are the most used all around, if possible this should be fixed for next RC if it will exists.

I'm asking blocking status to make drivers aware, it's not an hard blocker, but all of our main history queries are based upon this query, users are most likely to notice a bunch of entries for the same page till temp tables are synced to disk tables. So on every new added visit for 2 minutes.

Also, this needs a unit test, of course.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

This also affects "Forget this Site"; if you right-click on a website and ask Firefox to forget it, the experience is a little concerning. It in face removes the entry you're looking at, then that entry comes back for a few seconds, then it goes away again.

This might require an RC2.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This should fix all of the above, works fine from my tests, but everyone able to see the issue and to compile could try this for added safety.

Comment 14

[Marco Bonardo [:mak]]

PS: from a perf point of view this will lose something, but on a large db where the actual query takes 2s (notice old query was taking almost 4s), this will lose about 50-100ms, not a so big concern.
No reasons to cry on a perf loss if the query is wrong, though.

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It's easy to go fast if you don't care about the results!

Can we get a try server build going ASAP for wider testing?

Comment 16

[Marco Bonardo [:mak]]

try server building changeset ee807dcde041

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 383476 [details] [diff] [review]
patch v1.0

>+++ b/toolkit/components/places/tests/sync/test_database_sync_with_specialHistoryQueries.js
>+      // Set the preference for the timer to a large value so we don't sync.
>+      prefs.setIntPref(kSyncPrefName, 1);
This is not a large value

r=sdwilsh

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: v1.1, for check-in

comment fixed

Comment 19

[Dietrich Ayala (:dietrich)]

trunk: http://hg.mozilla.org/mozilla-central/rev/c4e56b6047b9

Comment 20

[Dietrich Ayala (:dietrich)]

per shaver's email to release-drivers, marking this blocking.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with builds on all platforms like:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090616 Minefield/3.6a1pre ID:20090616111535

Comment 22

[Dietrich Ayala (:dietrich)]

1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/30c84885c29a

Comment 23

[Nochum Sossonko [:Natch]]

restoring v. and tm...

A test landed with this as well, so in-testsuite+

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 with builds on all platforms like:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 ID:20090616212246

One remaining question to "Forget about this site". When I run this action for a sub domain only this subdomain will be removed. But running it on the domain itself, everything should be removed with the domain name in it. Is that correct?