Danial Horton Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre

some sites that are bookmarked, aren't removed from history if you have clear private data enabled, and you press save and quit, however this happens only if you happen to type the url out in the address box rather then using the bookmark.

Reproducible: Always

Steps to Reproduce:
1. Create a new profile if necessary
2. Load minefield (version doesn't matter)
3. Set it to clear private data automatically(clear history must be enabled obviously)
4. Go to a site,... say http://www.google.com and bookmark it.
5. Restart firefox and make sure the google bookmark is still there
6. Type http://www.google.com into the address box and navigate to it.
7. At this point either quit firefox with the "save and quit" option, or restart firefox with the "save and quit" option.
Actual Results:  
Upon restarting, if you open the history, depending on how you show history (mines set by site) you will find a folder present for that site which cannot be compacted/expanded and contains what appears to be no site. The only way to get rid of this at this point is to load an older Firefox (3.0.3 did the trick) and exit it, which will clear the history properly, or delete the places files(backup the bookmarks first of course).

Expected Results:  
All history should be cleared if

	Danial Horton Reporter
	Comment 1 • 16 years ago

oh, i should indicate that i use

Adblock Plus 0.7.5.5+.2008110323 [DISABLED]
BBCode 0.5.2.4 [DISABLED]
Cache Status 0.7.4 [DISABLED]
ChromEdit Plus 2.7.2 [DISABLED]
Console² 0.3.9.2 [DISABLED]
CookiePie 1.0.3 [DISABLED]
CustomizeGoogle 0.76 [DISABLED]
Download Statusbar 0.9.6.3 [DISABLED]
English (Australian) Dictionary 2.1.1 [DISABLED]
Flashblock 1.5.6 [DISABLED]
FlashGot 1.1.3.1 [DISABLED]
free-downloads.net Toolbar 1.5.47.1 [DISABLED]
Google Toolbar for Firefox 3.1.20081010W [DISABLED]
IE Tab 1.5.20080823 [DISABLED]
Image Toolbar 0.6.5 [DISABLED]
Image Zoom 0.3.1 [DISABLED]
Java Quick Starter 1.0 [DISABLED]
Minimize To Tray Enhancer 0.7.5.3 [DISABLED]
MinimizeToTray 1.0.0.20080518 [DISABLED]
Net Usage Item 1.2.210.1 [DISABLED]
Nightly Tester Tools 2.0.2 [DISABLED]
OpenSearchFox 0.1.5 [DISABLED]
Session Manager 0.6.2.5 [DISABLED]
Sourceforge Direct Download 0.5 [DISABLED]
StatusbarEx 0.2.17 [DISABLED]
Tab Mix Plus 0.3.7.3 [DISABLED]
Torrent Finder Toolbar 1.2.4 [DISABLED]
translator 1.0.4.4 [DISABLED]
User Agent Switcher 0.6.11 [DISABLED]
Veoh Browser Plug-in 1.3 [DISABLED]
Veoh Web Player Video Finder 1.4 [DISABLED]

and defaulted the theme, i've reproduced it on XP and Vista, as well as using clean profiles.

	Luke Iliffe (Harlequin99)
	Comment 2 • 16 years ago

Confirmed on a new profile using the STR in comment 0. This is using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre

This appears to be limited to viewing the history in the history sidebar only when it is set to view by site, all other views behave as expected. Cannot see any traces of the cleared history in the library.

	Steve England [:stevee]
	Comment 3 • 16 years ago

can you attach your places.sqlite for the STR ausing a clean profile?

	Danial Horton Reporter
	Comment 4 • 16 years ago

reproduce the error on a clean profile and post the places.sqlite? yep i can do that :)

	Danial Horton Reporter
	Comment 6 • 16 years ago

I'd like to further that this is a problem if a user sets the awesome bar to only show history results, the awesomebar may show items that the user would prefer were cleared.

	Luke Iliffe (Harlequin99)
	Comment 7 • 16 years ago

(In reply to comment #6)
> I'd like to further that this is a problem if a user sets the awesome bar to
> only show history results, the awesomebar may show items that the user would
> prefer were cleared.

I just cleared the value for browser.urlbar.restrict.history which should only show history items in the awesomebar and exclude non visited bookmarks. After a restart (that should clear the history) the www.google.com bookmark/history item does indeed show up in awesomebar results.

	Marco Bonardo [:mak] Assignee
	Comment 8 • 16 years ago

if you choose save & quit on next run the site is reopened and the history entry is created again... I would expect to see the website in the history sidebar, inside its host container.

	Danial Horton Reporter
	Comment 9 • 16 years ago

Marco, did you read the full STR? 

Also, if you remember, clearing private data clears session history as well now, so the saved session won't really reload.

	Steve England [:stevee]
	Comment 10 • 16 years ago

Thanks for the STR Danial. I've reworked them a little to be more explicit:

1. New profile, start firefox
2. Tools > Options > Privacy > Private Data. Tick "Always clear my private data when I close Minefield" and untick "Ask me before clearing private data".
3. Click the Private Data > Settings button and verify that Visited Pages is ticked. OK, OK out of the Options window.
4. Type www.google.com into the address bar and hit return. Google page loads. 
5. Click the star icon in the URL bar once.
6. Quit firefox with the [x] at the top right of the browser window. A "Quit Minefield" dialog should appear, just hit the Quit button. The browser closes and its history should have been cleared.
7. Start firefox again.
8. View > Sidebar > History, View > By Site

Expected
- www.google.com should not be listed because Clear Private Data should have cleared the history at shutdown

Actual
- www.google.com appears in the list (probably because it is bookmarked). The www.google.com folder looks like it is expandable, but in fact is not.

	Danial Horton Reporter
	Comment 11 • 16 years ago

Thanks, i'll reference your style of STR for future use.

	Marco Bonardo [:mak] Assignee
	Comment 12 • 16 years ago

ok what i can tell is:
- history is correctly cleared (And this is good)
- this only happens for bookmarks
- is due to visit_count not being correctly updated when we clear history

privacy impact is not high since the uri is bookmarked and we are only showing the host, hwv something that needs to be fixed.

	Danial Horton Reporter
	Comment 13 • 16 years ago

Well from my own testing on clean profile.

Doesn't occur if you click quit.
Always reproducible if you click save and quit.

As for history being correctly cleared, that would be the case, had the item for that site not appeared in the history sidebar.

Luke has also confirmed that it is indeed still in history by setting firefox to only search history from the awesomebar. Had the history been cleared, it wouldn't appear in the results.

Just because it is bookmarked doesn't make it any less of a privacy concern, i want my history cleared for good reason, regardless of whether the site is bookmarked or not.

	Marco Bonardo [:mak] Assignee
	Comment 14 • 16 years ago

it IS in the locationbar because it is a bookmark.

And comment 12 is the cause, so this does not need more confirmations, thanks!

	Marco Bonardo [:mak] Assignee
	Comment 15 • 16 years ago

PS: and after fixing this we'll also check for awesomebar to be sure everything has been fixed correctly.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 16 • 16 years ago

Probably the wrong place to ask, but why is 'Save and Quit' even an option while in PB mode?  That's the whole idea, not save anything, so why 'Save & Quit' ?

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 17 • 16 years ago

hmm, reference my comment above, I was just doing some testing, and now I no longer see any problem when closing out of PB Mode...

using this hourly build:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081106 Minefield/3.1b2pre Firefox/3.0 ID:20081106103456

	Danial Horton Reporter
	Comment 18 • 16 years ago

(In reply to comment #14)
> it IS in the locationbar because it is a bookmark.
> 
> And comment 12 is the cause, so this does not need more confirmations, thanks!

Ok, a bookmark is showing when firefox is only set to show history.
clearing the value on browser.urlbar.restrict.history prevents bookmarks from showing in the location bar, so why is a bookmark showing as a history item

argh, whatever, if it gets fixed it gets fixed.

	Marco Bonardo [:mak] Assignee
	Comment 19 • 16 years ago

Shawn, this appear due to the fact we don't sync moz_places_temp to moz_places at shutdown. I suppose that clearHistory() from the clear private data dialog, if we are setup so that data is cleared at shutdown, could be called after we have done the last sync at "quit-application", and so it would modify temp table setting correct values for frecency and visit_count, but those values are never synced back.
And this is a concern now that we have all this new privacy options (the same issue could be triggered by removePagesByTimeframe() if the user choice to clear only recent history at shutdown).
So i think we should force a synchronous sync of "at least" moz_places before the shutdown, i guess the history destructor could be a good place, often that query will do nothing and should not slow us down, or if you have any idea to do the last sync later (the only place i can think of would be xpcom-shutdown, but it could be too late to use the connection)

	Marco Bonardo [:mak] Assignee
	Comment 20 • 16 years ago

I've fixed also the query in ClearHistory that was working on the false assumption that expire_never page annotations are preventing us from removing pages (and this is no more true)

The test cannot check the db after the shutdown, so it's simply checking that data is correct before and after a sync.

We can discuss this later, hwv i think this issue should block b2.

	Shawn Wilsher :sdwilsh
	Comment 21 • 16 years ago

So, I do not think the right solution here is to duplicate our sync code in C++ here for cases where we want to sync.  The right solution here is to dump an event to the main thread when we get quit-application so that we run after quit-application ensuring that anything that does anything with us during quit-application will be synchronized by the time the application shuts down.

I also think that once we do that last synchronization, we should close the database connection so any further operations on the database will in fact fail.  We'll be able to catch bugs in our code like this much faster this way too.

	Marco Bonardo [:mak] Assignee
	Comment 22 • 16 years ago

this has still some issue:
1. connection can't be closed, sqlite3_close fails because we have open statements
2. the test is uneffective, it PASS also without the fix
3. still, with the fix, i see random test failures, but on failures i see onClearHistory called correctly BEFORE sync-finished, but the tables have not been synced.

Testing with STRs the issue is fixed, but i have some fear about the randomness.

	Marco Bonardo [:mak] Assignee
	Comment 23 • 16 years ago

mh i was thinking at this point ui responsiveness is no more needed since UI has already beene closed, so to be safe we could execute the statements synchronously, i'll try if that solves the random failures

	Marco Bonardo [:mak] Assignee
	Comment 24 • 16 years ago

no, that's not useful, i still see random failures

	Marco Bonardo [:mak] Assignee
	Comment 25 • 16 years ago

Comment on attachment 347029 [details] [diff] [review]
patch v1.1

ok, found the fault, i was observing too early for sync-finished, new patch coming.

	Marco Bonardo [:mak] Assignee
	Comment 26 • 16 years ago

this solves random failures.
Original issue solved but test still uneffective.

	Shawn Wilsher :sdwilsh
	Comment 28 • 16 years ago

Comment on attachment 347046 [details] [diff] [review]
patch v1.2

Regarding the close failure - we should add a method to nsPIPlacesDatabase like finalizeInternalStatements that will finalize our of our internal statements.  Call that before you close, and the failure should go away.

>diff --git a/toolkit/components/places/tests/sync/head_sync.js b/toolkit/components/places/tests/sync/head_sync.js

>+function DBConn() {
>+  let db = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsPIPlacesDatabase).
>+           DBConnection;
>+  if (db.connectionReady)
>+    return db;
>+
>+  // open a new connection if needed
>+  var file = dirSvc.get('ProfD', Ci.nsIFile);
>+  file.append("places.sqlite");
>+  var storageService = Cc["@mozilla.org/storage/service;1"].
>+                       getService(Ci.mozIStorageService);
>+  try {
>+    var dbConn = storageService.openDatabase(file);
>+  } catch (ex) {
>+    return null;
>+  }
>+  return dbConn;
>+}
Not really sure why this is needed.  Also, it doesn't follow convention in the rest of this file for function declarations, and doesn't contain a java-doc comment about what it does.

>diff --git a/toolkit/components/places/tests/sync/test_database_sync_after_quit_application_with_removeAllPages.js b/toolkit/components/places/tests/sync/test_database_sync_after_quit_application_with_removeAllPages.js

>+ *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
Naw - this is all you

Anyway - I know why you cannot get the test to fail.  Bug 460635 ended up making sure that the thread is shutdown before we close the connection.  The issue before was that we would end up closing before our event was processed, and bad things happened.  If you want to add a test, you could add one to storage to make sure that an async statement executes before close.  It wouldn't consistent fail though, so I'm not really sure it's worth it.

	Marco Bonardo [:mak] Assignee
	Comment 29 • 16 years ago

i would retain the current test, even if it's useless it's an additional test that could be useful to catch future problems...

about DBConn() i used that because when we simulate quit-application the connection should go away, but we have to test, so we need to reinit the connection, doing that with a getter was cleaner imho.

	Shawn Wilsher :sdwilsh
	Comment 32 • 16 years ago

Comment on attachment 347426 [details] [diff] [review]
patch v1.4

>@@ -832,39 +824,47 @@ nsNavHistoryExpire::ExpireAnnotations(mo
> 
>   // remove days annos
>   rv = expirePagesStatement->BindInt32Parameter(0, nsIAnnotationService::EXPIRE_DAYS);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expirePagesStatement->BindInt64Parameter(1, (now - EXPIRATION_POLICY_DAYS));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expirePagesStatement->Execute();
>   NS_ENSURE_SUCCESS(rv, rv);
>-  
>+  rv = expirePagesStatement->Reset();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   // remove days item annos
>   rv = expireItemsStatement->BindInt32Parameter(0, nsIAnnotationService::EXPIRE_DAYS);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expireItemsStatement->BindInt64Parameter(1, (now - EXPIRATION_POLICY_DAYS));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expireItemsStatement->Execute();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = expireItemsStatement->Reset();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // remove weeks annos
>   rv = expirePagesStatement->BindInt32Parameter(0, nsIAnnotationService::EXPIRE_WEEKS);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expirePagesStatement->BindInt64Parameter(1, (now - EXPIRATION_POLICY_WEEKS));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expirePagesStatement->Execute();
>   NS_ENSURE_SUCCESS(rv, rv);
>+  rv = expirePagesStatement->Reset();
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>   // remove weeks item annos
>   rv = expireItemsStatement->BindInt32Parameter(0, nsIAnnotationService::EXPIRE_WEEKS);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expireItemsStatement->BindInt64Parameter(1, (now - EXPIRATION_POLICY_WEEKS));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = expireItemsStatement->Execute();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = expireItemsStatement->Reset();
>   NS_ENSURE_SUCCESS(rv, rv);
You do not need to reset if the statement is not going to be used and will fall out of scope.

r=sdwilsh

	Shawn Wilsher :sdwilsh
	Comment 33 • 16 years ago

er, disregard the comment about Reset.  I mistakenly though that those were four different statements when it is only just two.

	Dietrich Ayala (:dietrich)
	Comment 34 • 16 years ago

Comment on attachment 347426 [details] [diff] [review]
patch v1.4

r=me. one question though: the mozIStorageStatement IDL says there's no need to finalize() statements, just null them out. is there a reason why you explicitly call finalize?

	Marco Bonardo [:mak] Assignee
	Comment 35 • 16 years ago

(In reply to comment #34)
> (From update of attachment 347426 [details] [diff] [review])
> r=me. one question though: the mozIStorageStatement IDL says there's no need to
> finalize() statements, just null them out. is there a reason why you explicitly
> call finalize?

Good question, i guess nulling would work as well and make the code more compact, if that's the case i'll use null.

	Marco Bonardo [:mak] Assignee
	Comment 36 • 16 years ago

this is nulling out statements, since their destructor takes care of finalizing them.
Also previous patch was missing 2 tests i forgot to add :\
Asking review for the additional bits.

	Marco Bonardo [:mak] Assignee
	Comment 37 • 16 years ago

Comment on attachment 347534 [details] [diff] [review]
patch v1.5

bad one

	Marco Bonardo [:mak] Assignee
	Comment 38 • 16 years ago

sorry, 1.5 was a bad patch where tests were duplicated by error, this is 1.4 with nulled out statements and a fix to avoid random failures in test_history_removeAllPages.js, and is good to go.

So, asking approval to land for beta 2

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 39 • 16 years ago

Comment on attachment 347535 [details] [diff] [review]
patch 1.6

a=beltzner, but it's not needed as b2 blockers have automatic approval

	Marco Bonardo [:mak] Assignee
	Comment 40 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/3ed4b74c493f

	Shawn Wilsher :sdwilsh
	Comment 41 • 16 years ago

The benefit of finalizing instead of nulling out is that we'll get errors instead of crashes like bug 464571.

	Dietrich Ayala (:dietrich)
	Comment 42 • 16 years ago

backed out, as it was holding the tree closed

http://hg.mozilla.org/mozilla-central/rev/925fe560ecce

	Marco Bonardo [:mak] Assignee
	Comment 43 • 16 years ago

backed out test for finalizeInternalStatements
http://hg.mozilla.org/mozilla-central/rev/0f40aa039d2b

other tests are not directly related to the fix and should pass

Shawn, finalizing we would get errors instead of crashes, but really we should not have any call after the connection is closed.

	Marco Bonardo [:mak] Assignee
	Comment 44 • 16 years ago

here is the culprit:

#4  0x08b7946c in nsNavHistory::CommitLazyMessages (this=0x1089400) at
/builds/moz2_slave/mozilla-central-macosx-debug/build/toolkit/components/places/src/nsNavHistory.cpp:5491
#5  0x08b79543 in nsNavHistory::LazyTimerCallback (aTimer=0x105f1cf0,

so, i suggest going back to Finalize, and before finalizing statements kill lazytimer and consume all its messages.

	Marco Bonardo [:mak] Assignee
	Comment 45 • 16 years ago

- changed from nullify statements to finalize them, to avoid crashes (will assert in case something goes wrong)

- before finalizing statements kill lazy timer and clear messages if they're still pending. Notice we could lose some "last-second" change to titles, visits or favicons, but they would not be synced hwv, when we will remove LAZY_ADD the problem will be solved

So this is practically patch 1.4 without the 2 tests already pushed and with fix for lazy messages.

Notice: 2 tests for removeAllPages are still in the tree, they are good and passing since are simply tests we were missing, not directly dependant on this fix (due to storage doing good work in enqueuing statements).

	Shawn Wilsher :sdwilsh
	Comment 46 • 16 years ago

Comment on attachment 347955 [details] [diff] [review]
patch 1.7

We'll want another method on nsPIPlacesDatabase that ends up calling nsNavHistory::SyncDB so we actually flush out our lazy queue.  Then we can sync the tables to disk, and then we can finalize our statements.

We won't lose any data this way.

	Marco Bonardo [:mak] Assignee
	Comment 47 • 16 years ago

try to save everything and be angry if you forget something.

	Shawn Wilsher :sdwilsh
	Comment 48 • 16 years ago

Comment on attachment 348113 [details] [diff] [review]
patch v1.8
[Checkin: Comment 50]

r=sdwilsh

	Dietrich Ayala (:dietrich)
	Comment 49 • 16 years ago

Comment on attachment 348113 [details] [diff] [review]
patch v1.8
[Checkin: Comment 50]

r=me

	Dietrich Ayala (:dietrich)
	Comment 50 • 16 years ago

checked-in: http://hg.mozilla.org/mozilla-central/rev/dc0e07e3d380

	Dietrich Ayala (:dietrich)
	Comment 51 • 16 years ago

unit test and leak builds finished green, closing.