410302 - when configured to clear private private data on shutdown, we do more work than we need to

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

expiring more than we need to on shutdown 

I think this happened as a result of a review comment of mine for bug #402297

I suggested the following change:

  PRBool sanitizeOnShutdown, sanitizeHistory;
  prefs->GetBoolPref(PREF_SANITIZE_ON_SHUTDOWN, &sanitizeOnShutdown);
  prefs->GetBoolPref(PREF_SANITIZE_ITEM_HISTORY, &sanitizeHistory);
  PRInt32 maxRecords =
    (sanitizeHistory && sanitizeOnShutdown) ? -1 :EXPIRATION_CAP_PLACES;

   // vacuum up dangling items
  rv = ExpireHistoryParanoid(connection, maxRecords);

We call this code on quit.  We should always pass in EXPIRATION_CAP_PLACES for max records, and remove the pref code.

I suggested this code for the scenarion where the user clears history on quit. 

but in that case, we will call ExpireHistoryParanoid() directly, which passes in -1, and does it after we call ExpireItems(0, &keepGoing);

so right now, if you are configured to clear private data on quit, we do twice the work we need to because we call ExpireHistoryParanoid() twice with -1.

I'll attach a patch.

Comment 1

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

Attachment: patch

Comment 2

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

in the case of shutting down and clearing private data, we'll still call
ExpireHistoryParanoid() twice, but the first time will be on
nsNavHistoryExpire::OnQuit() with EXPIRATION_CAP_PLACES for max records.

then, well call it again from nsNavHistoryExpire::ClearHistory(), but after
calling ExpireItems() but this time with -1 for max records.

Comment 3

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

morphing summary.

Comment 4

[Dietrich Ayala (:dietrich)]

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

Could we instead just not call ExpireHistoryParanoid the first time if the prefs are set? (since we know it'll get called again later)

That way it'd be getting called only once.

Comment 5

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

> Could we instead just not call ExpireHistoryParanoid the first time if the
> prefs are set? (since we know it'll get called again later)

the first time is from nsNavHistoryExpire::OnQuit(), and we always want to call that in order to do partial expiration.  (Think about the common case of *not* clearing private data on shutdown.  only onQuit() gets called.)

the second time is nsNavHistoryExpire::ClearHistory(), and we call it here after calling ExpireItems() to clean up the database after calling ExpireItems().

I thought about just doing this from onQuit(), but we still need the call in ClearHistory() as it has to be after the call to ExpireItems() and ClearHistory() is called when you manually clear all history when clearing private data.

what we could do is, in onQuit, keep the existing code, and if we determine that maxRecords is -1, we do nothing in onQuit() and instead, wait to do the work in ClearHistory().  This would avoid the partial expiration in onQuit() in this scenario.

if you think that's a better fix, I can whip that up.

Comment 6

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

> if you think that's a better fix, I can whip that up.

I think that is the right fix, patch coming.

Comment 7

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

Attachment: patch, v2, per dietrich's suggestion

if our prefs indicate that we are going to be clearing history, don't bother with the partial expiration onQuit()

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 295126 [details] [diff] [review]
patch, v2, per dietrich's suggestion

r=me, thanks!

Comment 9

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

Comment on attachment 295126 [details] [diff] [review]
patch, v2, per dietrich's suggestion

seeking approval

Comment 10

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

fixed.

Checking in nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.33; previous revision: 1.32
done

Comment 11

[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