463471 - temp tables are not correctly synced to disk when the user clear private data on shutdown

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Danial Horton]

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

Comment 1

[Danial Horton]

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.

Comment 2

[Luke Iliffe (Harlequin99)]

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.

Comment 3

[Steve England [:stevee]]

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

Comment 4

[Danial Horton]

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

Comment 5

[Danial Horton]

Attachment: places.sqlite file

Comment 6

[Danial Horton]

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.

Comment 7

[Luke Iliffe (Harlequin99)]

(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.

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Danial Horton]

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.

Comment 10

[Steve England [:stevee]]

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.

Comment 11

[Danial Horton]

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

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Danial Horton]

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.

Comment 14

[Marco Bonardo [:mak]]

it IS in the locationbar because it is a bookmark.

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

Comment 15

[Marco Bonardo [:mak]]

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

Comment 16

[Jim Jeffery not reading bug-mail 1/2/11]

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' ?

Comment 17

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 18

[Danial Horton]

(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.

Comment 19

[Marco Bonardo [:mak]]

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)

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch

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.

Comment 21

[Shawn Wilsher :sdwilsh]

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.

Comment 22

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 23

[Marco Bonardo [:mak]]

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

Comment 24

[Marco Bonardo [:mak]]

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

Comment 25

[Marco Bonardo [:mak]]

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.

Comment 26

[Marco Bonardo [:mak]]

Attachment: patch v1.2

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

Comment 28

[Shawn Wilsher :sdwilsh]

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 <[e-mail]> (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.

Comment 29

[Marco Bonardo [:mak]]

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.

Comment 30

[Marco Bonardo [:mak]]

Attachment: patch v1.3

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v1.4

Comment 32

[Shawn Wilsher :sdwilsh]

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

Comment 33

[Shawn Wilsher :sdwilsh]

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

Comment 34

[Dietrich Ayala (:dietrich)]

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?

Comment 35

[Marco Bonardo [:mak]]

(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.

Comment 36

[Marco Bonardo [:mak]]

Attachment: patch v1.5

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.

Comment 37

[Marco Bonardo [:mak]]

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

bad one

Comment 38

[Marco Bonardo [:mak]]

Attachment: patch 1.6

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

Comment 39

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

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

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

Comment 40

[Marco Bonardo [:mak]]

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

Comment 41

[Shawn Wilsher :sdwilsh]

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

Comment 42

[Dietrich Ayala (:dietrich)]

backed out, as it was holding the tree closed

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

Comment 43

[Marco Bonardo [:mak]]

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.

Comment 44

[Marco Bonardo [:mak]]

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.

Comment 45

[Marco Bonardo [:mak]]

Attachment: patch 1.7

- 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).

Comment 46

[Shawn Wilsher :sdwilsh]

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.

Comment 47

[Marco Bonardo [:mak]]

Attachment: patch v1.8 [Checkin: Comment 50]

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

Comment 48

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 49

[Dietrich Ayala (:dietrich)]

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

r=me

Comment 50

[Dietrich Ayala (:dietrich)]

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

Comment 51

[Dietrich Ayala (:dietrich)]

unit test and leak builds finished green, closing.