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

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

Trunk
Firefox 3 beta3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 294931 [details] [diff] [review]
patch
Attachment #294931 - Flags: review?(dietrich)
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.
Assignee: nobody → sspitzer
morphing summary.
Status: NEW → ASSIGNED
Summary: expiring more than we need to on shutdown if clearing private data → when configured to clear private private data on shutdown, we do more work than we need to
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.
> 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.
> if you think that's a better fix, I can whip that up.

I think that is the right fix, patch coming.
Created attachment 295126 [details] [diff] [review]
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()
Attachment #294931 - Attachment is obsolete: true
Attachment #295126 - Flags: review?(dietrich)
Attachment #294931 - Flags: review?(dietrich)
Comment on attachment 295126 [details] [diff] [review]
patch, v2, per dietrich's suggestion

r=me, thanks!
Attachment #295126 - Flags: review?(dietrich) → review+
Target Milestone: --- → Firefox 3 M11
Comment on attachment 295126 [details] [diff] [review]
patch, v2, per dietrich's suggestion

seeking approval
Attachment #295126 - Flags: approval1.9?

Updated

10 years ago
Attachment #295126 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.