Closed Bug 409945 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta4

People

(Reporter: zurtex, Assigned: mak)

References

Details

(Keywords: privacy)

Attachments

(1 file, 4 obsolete files)

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.
Would this be fixed by bug 370644 & bug 394133 ? Or would that only affect the 'Most Visited' count ?
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.
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.
Flags: blocking-firefox3?
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.
Summary: Clear private data should affect places → Clear private data should affect history based smart bookmarks
IIRC, there is a "privacy" keyword here on Bugzilla, can you add it to this bug?
Thanks done, new to this Bugzilla stuff.
Keywords: privacy
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: General → Places
Ever confirmed: true
OS: Windows Server 2003 → All
Hardware: PC → All
Version: unspecified → Trunk
QA Contact: general → places
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)
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.
Duplicate of this bug: 414348
(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

(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
Resummarizing, fixing severity based on this being something that should work.  Marco, can you reproduce this?
Severity: enhancement → normal
Summary: Clear private data should affect history based smart bookmarks → Clear private data doesn't force smart bookmarks to rebuild
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
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. 
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).
(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?
with "open it" i mean sqlite manager, not the site!
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
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.
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)
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.
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 
Status: NEW → ASSIGNED
Assignee: nobody → mak77
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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.
i can probably track down this to Bug 379211, checked-in 2007-05-10 in alpha5
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.
have you migrated your profile from a previous version on another computer?
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
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?
yes please, but first try with current build and with imported bookmarks since i think that import could cause that
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.
Make sure there are no running firefox.exe processes and then delete the parent.lock file from your profile directory.
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.
you really didn't have to, all you had to do was delete the Places.sqlite file
I really really did, my entire profile folder was deleted never mind about places.sqlite, Firefox still refused to load.
Thank you Damian, i'm going to fix that.
Summary: Clear private data doesn't force smart bookmarks to rebuild → Charset annotations created on import should not be EXPIRE_NEVER (was: Clear private data doesn't force smart bookmarks to rebuild)
Summary: Charset annotations created on import should not be EXPIRE_NEVER (was: Clear private data doesn't force smart bookmarks to rebuild) → Charset annotations created on import should be itemAnnotations (was: Clear private data doesn't force smart bookmarks to rebuild)
Attached patch wip (obsolete) — Splinter Review
recently visited sites is kind of redundant, thats what the history bar is for.
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)
(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

doesn't explain why it works fine with it not deleted, and then it stuffs up once it is.
Attached patch wip (obsolete) — Splinter Review
we should migrate annos into items annos instead of deleting them, to avoid dataloss
Attachment #301695 - Attachment is obsolete: true
Flags: blocking-firefox3? → blocking-firefox3+
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #301872 - Attachment is obsolete: true
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)
Attachment #302029 - Flags: review?(dietrich)
Whiteboard: [has patch][needs review 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
Attachment #302029 - Flags: review?(dietrich) → review+
Keywords: checkin-needed
Whiteboard: [has patch][needs review dietrich] → [has patch]
Priority: -- → P2
coming with fixed review comments, then i'll annotate there

yeah, we should not check-in this before Bug 415201
Depends on: 415201
Keywords: checkin-needed
Attached patch for check-in (obsolete) — Splinter Review
Attachment #302029 - Attachment is obsolete: true
Blocks: 391419
Target Milestone: --- → Firefox 3 beta4
Whiteboard: [has patch] → [has patch][has review]
Bug 415201 is fixed, checkin-needed here!
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
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 ***

<<<<<<<
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attached patch test fixedSplinter Review
i've fixed the test that was checking the wrong anno
Attachment #302635 - Attachment is obsolete: true
Attachment #303412 - Flags: review+
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Depends on: 422548
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.