409945 - Charset annotations created on import should be itemAnnotations (was: Clear private data doesn't force smart bookmarks to rebuild)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Damian Shaw [Quan]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

I just went to clear my private data as I'm letting someone else use my computer for the day (so bookmarks and like I want retained but everything else can go).

Given there was no obvious way to clear 'places' or 'smart bookmarks', I assumed clearing browser history would do this, because it essentially is browser history. To my somewhat dismay I found it wasn't cleared and furthermore there was no obvious way to clear them without just deleting them...

I expected this bug to already be reported as it'd be pretty essential to get something like this in for Firefox 3 final otherwise people would get rather upty about it not actually letting you obviously clear private data, but did several searches and couldn't find anything. I sort of want to report this as Major, but I'll leave it as normal for now.

Reproducible: Always

Steps to Reproduce:
1. Clear Private Data (any possible combination of options)
2. Click on 'Places' or 'Smart Bookmarks'
3. Notice data is still there.
Actual Results:  
Data wasn't cleared

Expected Results:  
Data should of been cleared.

Comment 1

[Jo Hermans]

Would this be fixed by bug 370644 & bug 394133 ? Or would that only affect the 'Most Visited' count ?

Comment 2

[Damian Shaw [Quan]]

I don't know, are things like:

"Recently Visited Pages"
or
"Recently Bookmarked"

affected by the whole "visit count" thing, because if not it, things like that need to be considered. I don't know much of Firefox's internal workings so I can't say much, just trying to bring the issue up.

Comment 3

[Damian Shaw [Quan]]

Nominating this to be fixed for Firefox 3.

Just double checked it on Firefox Beta 2 on a different OS (Windows XP Pro instead of Windows x64). 

Same situation, Smart Bookmarks isn't affected by Clear Private Data at all, given Smart Bookmarks is really a clever form of browsing history and not actually 'Bookmarks' as such, then this makes Clear Private Data function useless for some people.

Comment 4

[Damian Shaw [Quan]]

It's been pointed out to me that smart bookmarks, on the Mozilla developer IRC channel, that not all smart bookmarks need to be history based. So ideally I guess what would be wanted was when someone selected: 

"Browsing History" on "Clear Private Data"

That any history based smart bookmarks were cleared, things involving things like "most" and "recent". Most people seemed to thing such things were directly generated from the History, but with a cleared a History these things still appear to be populated. Anyway, just trying to provide as much info as possible for someone who might try to fix the bug.

Comment 5

[Daniel Serodio]

IIRC, there is a "privacy" keyword here on Bugzilla, can you add it to this bug?

Comment 6

[Damian Shaw [Quan]]

Thanks done, new to this Bugzilla stuff.

Comment 7

[Dietrich Ayala (:dietrich)]

Smart bookmarks are not "saved", but are driven by the data available in history and bookmarks. Two possible bugs here:

1. history was not actually cleared
2. live-update of query nodes is broken (ie: changes to history did not trigger regeneration of the smart bookmarks' contents)

Comment 8

[Damian Shaw [Quan]]

Well I've just done a little test, I went to the web page:
"http://www.squarefree.com/burningedge/The Burning Edge" 

I cleared my private data (everything except cookies and passwords). Went to my Firefox profile folder and opened places.sqlite and searched for "burningedge" and low and behold I found the exact url: "http://www.squarefree.com/burningedge/The Burning Edge".

In fact I found it twice amongst loads of other urls I've visited in the last week, even though I must have cleared private data 3 or 4 times to keep testing things out the last week. 

places.sqlite was 9104kb, which also seemed odd given I'd been clearing my data so regularly.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #8)
> I cleared my private data (everything except cookies and passwords). Went to my
> Firefox profile folder and opened places.sqlite and searched for "burningedge"
> and low and behold I found the exact url:
> "http://www.squarefree.com/burningedge/The Burning Edge".

Was that url bookmarked / starred?

> places.sqlite was 9104kb, which also seemed odd given I'd been clearing my data so regularly.

places.sqlite size does not change, there is another open bug report on that, but that's not a problem

Comment 11

[Damian Shaw [Quan]]

(In reply to comment #10)
> (In reply to comment #8)
> > I cleared my private data (everything except cookies and passwords). Went to my
> > Firefox profile folder and opened places.sqlite and searched for "burningedge"
> > and low and behold I found the exact url:
> > "http://www.squarefree.com/burningedge/The Burning Edge".
> 
> Was that url bookmarked / starred?

Yes, but URLs not bookmarked / starred also appeared in places.sqlite

Comment 12

[Mike Connor [:mconnor]]

Resummarizing, fixing severity based on this being something that should work.  Marco, can you reproduce this?

Comment 13

[Marco Bonardo [:mak]]

after a clear private data the only addresses i still see in places.sqlite (moz_places table) are:
- bookmarks
- starred items
- default folders (roots)
- livemarks results

smart bookmarks query will continue to get results from those urls, but still that should not be a privacy concern since those urls are bookmarked (so it's quite obvious to everybody that they are visited)

Damiam, since you already have opened the db with an sqlite explorer, can you execute this query and check what items are out of that categories? (you can use sqlite manager if you want https://addons.mozilla.org/en-US/firefox/addon/5817)

SELECT h.id,h.url FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
WHERE
b.id IS NULL AND
(a.id IS NULL OR a.expiration != 4)

test this in safe mode and be sure to not visit any website after clearing private data. Thank you

Comment 14

[Marco Bonardo [:mak]]

the only thing i've been able to reproduce is this:

STR:
- visit a page and star it
- clear private data
- remove the bookmark (but don't visit the site)
- the page still appear into most visited even if it's not visited and it's not a bookmark
- if you clear private data again it will go away (Since it will expire all dangling items)

this is because most visited uses count column in moz_places, and the page had not been expired yet. 

Comment 15

[Damian Shaw [Quan]]

Right, I've never used an SQLite Explorer before, I originally opened places.sqlite with notepad, but your instructions were easy to follow and when I did I was rather confused because indeed I wasn't able to reproduce the bug. 

Unsatisfied I've been testing the behavior of different options of Clear Private Data for the last 30 mins or so and have been able to reproduce the bug some times and not at others. I think I've found the problem and this is a bit long winded but here are the steps I did that fully show the problem:

1. Clicked on 'Smart Bookmarks'
2. Mouse over 'Most Visited'
3. Observed 'Anandtech' as 1st result
4. Deleted 'Anandtech' from bookmarks, double checked and searched it wasn't duplicated
5. Selected 'Clear Private Data'
6. Selected ONLY 'Browsing History'
7. Clicked 'Clear Private Data NOW'
8. Clicked on 'Smart Bookmarks'
9. Mouse over 'Most Visited'
10. Observed 'Anandtech' as 1st result

I then tried again but restarted Firefox before checking 'Most Visited'

1. Clicked on 'Smart Bookmarks'
2. Mouse over 'Most Visited'
3. Observed 'Ctrl+Alt+Del' as 3rd result
4. Deleted 'Ctrl+Alt+Del' from bookmarks, double checked and searched it wasn't duplicated
5. Selected 'Clear Private Data'
6. Selected ONLY 'Browsing History'
7. Clicked 'Clear Private Data NOW'
8. Restarted Firefox
9. Clicked on 'Smart Bookmarks'
10. Mouse over 'Most Visited'
11. Observed 'Ctrl+Alt+Del' as 3rd result

A little puzzled I still wasn't able to replicate the correct behavior as I did earlier, I tried selecting all options for Clear Private Data:

1. Clicked on 'Smart Bookmarks'
2. Mouse over 'Most Visited'
3. Observed 'Convivea Forums' as 4th result
4. Deleted 'Convivea Forums' from bookmarks, double checked and searched it wasn't duplicated
5. Selected 'Clear Private Data'
6. Selected ALL options for Clear Private Data
7. Clicked 'Clear Private Data NOW'
8. Restarted Firefox
9. Clicked on 'Smart Bookmarks'
10. Mouse over 'Most Visited'
11. Observed 'Convivea Forums' was no longer there!
12. Observed 'Anandtech' and 'Ctrl+Alt+Del' was still the 1st and 3rd result respectively!!

This produces 2 behavioral problems:

1) Logically 'Browsing History' should clear 'History based smart bookmarks'

2) Results that have been cleared from Bookmarks but still observed in 'Most Visited' appear to still be 'trapped' there even after fully clearing every options on 'Clear Private Data' (I'm guessing it was clearing the cache that made it work in the end).

Comment 16

[Marco Bonardo [:mak]]

(In reply to comment #15)
> 1) Logically 'Browsing History' should clear 'History based smart bookmarks'

yes, but most visited is a history & bookmark based (since bookmarked/starred items will dongle there also after a clear history)

could you install sqlite manager extension and, when you see that a site is still showed after a clear history, open it and execute in sqlite manager the SQL query:

SELECT h.id, h.url FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
WHERE
b.id IS NULL AND
(a.id IS NULL OR a.expiration != 4)

and annotate if it's returning some item?

Comment 17

[Marco Bonardo [:mak]]

with "open it" i mean sqlite manager, not the site!

Comment 18

[Marco Bonardo [:mak]]

one last thing: what version are you using for these tests? still beta2? if yes, could you retry also with current nightly build?
thank you

Comment 19

[Damian Shaw [Quan]]

Yeah sorry, I did that already and wrote in to the Bugzilla comment box, but
needed to restart Firefox so lost it all and forgot to retype it up.

I cleared 'browsing history', executed the SQL query and got 0 rows returned.

But I would like to point out that 'Anandtech' and 'Crtl+Alt+Del' are still in
my 'Most Visited' section even though I've deleted them from my bookmarks,
cleared private data multiple times and restarted Firefox multiple times.


Yep, I'm still on Beta 2, I'll try with the nightly build in a bit thanks.

Comment 20

[Marco Bonardo [:mak]]

thank you, if the query is returning 0 after clear private data, those sites are bookmarked somewhere, or have some un-expirable annotation on them.

please when you still see an item there and the previous query returns 0 execute this:

SELECT h.id, h.url, b.parent, a.anno_attribute_id FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
WHERE
b.id IS NOT NULL OR
a.id IS NOT NULL

recognize the bogus url into results, and put here values reported for that url (parent and anno_attribute_id)

Comment 21

[Damian Shaw [Quan]]

They are absolutely NOT bookmarked somewhere, I was very very careful in checking that.

I executed the SQL command, the problematic URLs report:

id: 425
url: http://anandtech.com/default.aspx
parent: NULL
anno_attribute_id: 1

AND

id: 445
url: http://www.ctrlaltdel-online.com/
parent: NULL
anno_attribute_id: 1

I've not yet tried the latest nightly build, will try and reproduce this on that later.

Comment 22

[Marco Bonardo [:mak]]

found the problem,it is not bookmarked but that has a bookmarkProperties/description annotation

it needs some investigation but the problem is surely there
thank you for your effort in finding that 

Comment 23

[Marco Bonardo [:mak]]

Damian, how old is your profile? have you tested previous alphas with that? Could you give me a "somehow" correct date of creation for the profile you're using?

that annotation shouldn't be there, i suppose that has been inserted there by some old alpha version code.

Will however post a patch to cleanup profiles from that dangling annos before next beta.

Comment 24

[Marco Bonardo [:mak]]

i can probably track down this to Bug 379211, checked-in 2007-05-10 in alpha5

Comment 25

[Damian Shaw [Quan]]

I never installed an alpha version of Firefox on this particular computer. 1 moment, I'll download the latest trunk build, do a clean install and see if I can replicate.

Comment 26

[Marco Bonardo [:mak]]

have you migrated your profile from a previous version on another computer?

Comment 27

[Marco Bonardo [:mak]]

sorry, i still need you execute this query when an items does not disappear, and report name column of the bogus item...

SELECT h.id, h.url, t.name FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
LEFT JOIN moz_anno_attributes t on t.id = a.anno_attribute_id
WHERE
b.id IS NOT NULL OR
a.id IS NOT NULL

Comment 28

[Damian Shaw [Quan]]

Unable to reproduce on:  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020404 Minefield/3.0b3pre ID:2008020404 with a clean profile. Yay :-).

I think was able to reproduce it on 3.0b4pre with a clean profile but imported bookmarks. Want me to try all that stuff on a version with an old profile or imported bookmarks to see if it still affects the nightly builds?

Comment 29

[Marco Bonardo [:mak]]

yes please, but first try with current build and with imported bookmarks since i think that import could cause that

Comment 30

[Damian Shaw [Quan]]

Sorry, testing is going to have to wait until tomorrow morning, I've some how completely killed Firefox. Every time I install and launch I just get the error message: 

"Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system"

Well there is no other Firefox process running and I've tried restarting my system, re-installing Firefox, installing a different version of Firefox, manually deleting Firefox etc... So until I figure this out, Firefox won't work on this computer at all! Sorry.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Make sure there are no running firefox.exe processes and then delete the parent.lock file from your profile directory.

Comment 32

[Damian Shaw [Quan]]

Right, I made a .html backup of my bookmarks through the normal 'export' process. Installed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020404 Minefield/3.0b3pre ID:2008020404, then imported bookmarks.

I clicked on 'Anandtech' and 'Ctrl + Alt + Del', refreshed them a few times to get it in to 'Most Visited'.

1) Deleted Anandtech
2) Cleared Browser History
3) Still appeared in 'Most Visited'

1) Deleted 'Ctrl + Alt + Del'
2) Cleared selected ALL options on Clear Private Data
3) Restarted Firefox
4) Still appeared in 'Most Visited'

I executed the SQL command:

SELECT h.id,h.url FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
WHERE
b.id IS NULL AND
(a.id IS NULL OR a.expiration != 4)


And got 0 results returned, I then executed the SQL command:

SELECT h.id, h.url, b.parent, a.anno_attribute_id FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
WHERE
b.id IS NOT NULL OR
a.id IS NOT NULL

The troublesome URLs gave this data:


id: 207
url: http://anandtech.com/default.aspx
parent: NULL
anno_attribute_id: 8

AND

id: 211
url: http://www.ctrlaltdel-online.com/
parent: NULL
anno_attribute_id: 8


I then ran the SQL command:

SELECT h.id, h.url, t.name FROM moz_places h
LEFT JOIN moz_bookmarks b ON h.id = b.fk
LEFT JOIN moz_annos a ON h.id = a.place_id
LEFT JOIN moz_anno_attributes t on t.id = a.anno_attribute_id
WHERE
b.id IS NOT NULL OR
a.id IS NOT NULL


The troublesome URLs gave this data:

id: 425
url: http://anandtech.com/default.aspx
name: URIProperties/characterSet

AND

id: 445
url: http://www.ctrlaltdel-online.com/
name: URIProperties/characterSet


P.S thanks for the help, turns out I needed to delete the whole Mozilla folder in %appdata% to get things working again.

Comment 33

[Danial Horton]

you really didn't have to, all you had to do was delete the Places.sqlite file

Comment 34

[Damian Shaw [Quan]]

I really really did, my entire profile folder was deleted never mind about places.sqlite, Firefox still refused to load.

Comment 35

[Marco Bonardo [:mak]]

Thank you Damian, i'm going to fix that.

Comment 36

[Marco Bonardo [:mak]]

Attachment: wip

Comment 37

[Danial Horton]

recently visited sites is kind of redundant, thats what the history bar is for.

Comment 38

[Danial Horton]

oh, im not on my machine atm, but theres an issue with clearing Typed in url's if you happen to delete the smart bookmarks folder (and anything in it)

Comment 39

[Marco Bonardo [:mak]]

(In reply to comment #38)
> oh, im not on my machine atm, but theres an issue with clearing Typed in url's
> if you happen to delete the smart bookmarks folder (and anything in it)

smart bookmarks folder is made up of queries, so deleting that will not change anything elsewhere. However that is unrelated to this bug where the problem is that an item does not disappear from "most visited" query

Comment 40

[Danial Horton]

doesn't explain why it works fine with it not deleted, and then it stuffs up once it is.

Comment 41

[Marco Bonardo [:mak]]

Attachment: wip

we should migrate annos into items annos instead of deleting them, to avoid dataloss

Comment 42

[Marco Bonardo [:mak]]

Attachment: patch

this is working, i still need to do some import and migration test.

for performance of queries i'd wait for Bug 415201 (that adds some useful index)

Comment 43

[Marco Bonardo [:mak]]

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

Here are steps for my tests:

First
- import a bookmarks.html that contain charset definition for most bookmarks
- check that all charset annos are gone into table moz_items_annos

Second
- take a places.sqlite that has charset annotation for bookmarks into moz_annos
- after the first shutdown or clear private data check that all charset annos have been migrated to moz_items_annos

Third
- take a bookmark that has a charset anno, unbookmark it and bring to most visited query
- clear private data
- check that it is no more present in most visited

All tests passed.

Notice that i've tried to be conservative, so i move charset annotations only for bookmarks, while i don't touch them for history items that are not bookmarked (since they are not created on import, so the annotation there could be valid)

Comment 44

[Dietrich Ayala (:dietrich)]

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

>Index: toolkit/components/places/src/nsNavHistoryExpire.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v
>retrieving revision 1.37
>diff -u -8 -p -r1.37 nsNavHistoryExpire.cpp
>--- toolkit/components/places/src/nsNavHistoryExpire.cpp	25 Jan 2008 17:11:14 -0000	1.37
>+++ toolkit/components/places/src/nsNavHistoryExpire.cpp	8 Feb 2008 00:25:35 -0000
>@@ -872,16 +872,62 @@ nsNavHistoryExpire::ExpireAnnotationsPar
>       "LEFT OUTER JOIN moz_places p ON a.place_id = p.id "
>       "LEFT OUTER JOIN moz_historyvisits v ON a.place_id = v.place_id "
>       "WHERE p.id IS NULL "
>       "OR (v.id IS NULL AND a.expiration != ") +
>       nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +
>       NS_LITERAL_CSTRING("))"));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // XXX REMOVE ME BEFORE FINAL
>+  // There was a period in which we inserted bogus charset annos during bookmark
>+  // import, we must move them into items annos, since those pages will
>+  // never get deleted from moz_places and that is a valid privacy concern.
>+  nsCAutoString charsetAnno;
>+  charsetAnno.AssignLiteral("URIProperties/characterSet");
>+

nsCAutoString charsetAnno("URIProperties/characterSet") should work here.

everything else looks fine, r=me. a couple of things:

- please wait to land this until the anno index fix gets checked in
- make a note on bug 317472
- make a note on bug 391419

Comment 45

[Marco Bonardo [:mak]]

coming with fixed review comments, then i'll annotate there

yeah, we should not check-in this before Bug 415201

Comment 46

[Marco Bonardo [:mak]]

Attachment: for check-in

Comment 47

[Marco Bonardo [:mak]]

Bug 415201 is fixed, checkin-needed here!

Comment 48

[Dietrich Ayala (:dietrich)]

backed out due to failing unit tests:

../../../../_tests/xpcshell-simple/test_browser_places/unit/test_bookmarks_html.js: FAIL
../../../../_tests/xpcshell-simple/test_browser_places/unit/test_bookmarks_html.js.log:
>>>>>>>
*** test pending
*** exiting
*** CHECK FAILED: false == true
JS frame :: /builds/slave/trunk_osx/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 99
JS frame :: /builds/slave/trunk_osx/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 114
JS frame :: /builds/slave/trunk_osx/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_true :: line 118
JS frame :: ../../../../_tests/xpcshell-simple/test_browser_places/unit/test_bookmarks_html.js :: testCanonicalBookmarks :: line 250
JS frame :: ../../../../_tests/xpcshell-simple/test_browser_places/unit/test_bookmarks_html.js :: run_test :: line 116
JS frame :: /builds/slave/trunk_osx/mozilla/tools/test-harness/xpcshell-simple/tail.js :: _execute_test :: line 41
JS frame :: /builds/slave/trunk_osx/mozilla/tools/test-harness/xpcshell-simple/execute_test.js :: <TOP_LEVEL> :: line 38
2147500036
*** FAIL ***

<<<<<<<

Comment 49

[Marco Bonardo [:mak]]

oops i usually execute places tests, but this was in the other test unit, sorry!!!
i need to update the test since it's wrong now:

  var pageURI = iosvc.newURI(testBookmark1.uri, "", null);
  do_check_true(annosvc.pageHasAnnotation(pageURI, LAST_CHARSET_ANNO));
  do_check_eq("ISO-8859-1", annosvc.getPageAnnotation(pageURI,
                                                      LAST_CHARSET_ANNO));

should be item not page, coming soon with a fix

Comment 50

[Marco Bonardo [:mak]]

Attachment: test fixed

i've fixed the test that was checking the wrong anno

Comment 51

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.40; previous revision: 1.39
done

Comment 52

[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