386956 - Simplify nsNavHistory::RemoveDuplicateURIs

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

nsNavHistory::RemoveDuplicateURIs actually uses a SELECT, a cycle to find duplicates, and a cycle on duplicates with 2 DELETEs on each iteration...

that can be done with 2 simpleQuery DELETEs in a transaction.
Duplicates are found using the HAVING clause.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Use HAVING clause in RemoveDuplicateURIs

Comment 2

[Marco Bonardo [:mak]]

Attachment: Use HAVING clause in RemoveDuplicateURIs

fixed typos in comments

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: as checked in

Comment 4

[(not reading, please use [e-mail] instead)]

fixed, thanks Marco.

Checking in nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.142; previous revision: 1.141
done

Comment 5

[(not reading, please use [e-mail] instead)]

<tracy> dietrich, regarding bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=386956 , what woudl you recommend for verification?
<dietrich> sspitzerMsgMe: ^^ any idea how tracy could verify bug 386956?
<dietrich> i think that's used by import...

Yes, this code is used in import (from Fx 2, history.dat to Fx 3, places.sqlite).

See http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsMorkHistoryImporter.cpp#248

Note, the existing code (before Marco's fix) also removed duplicate uris.  his patch is a code simplification.

I have Jay Patel's fx 2 history.dat with duplicate URIs that I used for testing.  you could migrate from fx 2 -> fx 3 with that, and then use the SQLite Database Browser to verify that there were no duplicate uris after the migration.  (Assuming Jay is ok with me sharing that with you privately.)

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Can I get a copy of the history.dat file for verification?

Comment 7

[(not reading, please use [e-mail] instead)]

> Can I get a copy of the history.dat file for verification?

I'm glad you asked.  I was creating one for you, and in the process realized there is a problem with this patch and it needs to be backed out.

For a uri that appears twice, the currently checked in patch will remove both all the visits for both in moz_historyvisits, and then remove both instances from moz_places.

so I need to back out this patch, probably wontfix this bug, and log a new one about instead of deleting moz_historyvisits, making them visits for the moz_place entry we keep.

I'll attach a history.dat for testing.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: back out the patch, causes dataloss

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 274115 [details] [diff] [review]
back out the patch, causes dataloss

oops, I'm wrong.  Sorry Marco.

Comment 10

[(not reading, please use [e-mail] instead)]

poor testing on my part.  Marco's fix is fine.

"SELECT id FROM moz_places GROUP BY url HAVING(COUNT(url) > 1)" works as expected.

But I don't think we want to remove visits, because that will leave us with a visit count that does not match the number of entries in the moz_historyvisits table.  but I'll spin that bug off.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: history.dat file with duplicate urls

Comment 12

[(not reading, please use [e-mail] instead)]

al, you can generate this yourself by doing:

http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=984023'

and

http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=984023%27

in a fx2 profile and then migrating to the trunk.

to ensure that we don't have duplicate urls, you can inspect the moz_places table in the SQLite Database Browser, or run this query:

SELECT id FROM moz_places GROUP BY url HAVING(COUNT(url) > 1)

note, in fx 2, you would see two urls in the history sidebar where in fx 3, you will only see one.

Comment 13

[(not reading, please use [e-mail] instead)]

> two urls

to clarify, I mean two instances of urls like:

"http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=..."

there are a few other urls in the history.dat I attached.

Comment 14

[(not reading, please use [e-mail] instead)]

> But I don't think we want to remove visits, because that will leave us with a
> visit count that does not match the number of entries in the moz_historyvisits
> table.  but I'll spin that bug off.

spun off to bug #389789

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/200708100404 Minefield/3.0a8pre.

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv