407373 - if ExpireItems() fails, don't fire the timer again.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

if ExpireItems() fails, don't keep going

after running over night, in my console, I was seeing:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file c:/builds
/trunk/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp, line 483
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file c:/builds
/trunk/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp, line 335
WARNING: ExpireItems failed.: file c:/builds/trunk/mozilla/toolkit/components/pl
aces/src/nsNavHistoryExpire.cpp, line 292

even when no longer idle, every 3.5 seconds, I'd get those messages.

Dietrich's fix for bug #407018 should fix the error, as the problem is caused by using selectStatement instead of selectMinStatement.

But I think we should consider either changing the logic in ExpireItems (which sets keepGoing), or fix DoPartialExpiration.

Dietrich, what do you think about this:

nsresult
nsNavHistoryExpire::DoPartialExpiration()
{
  // expire history items
  PRBool keepGoing;
  nsresult rv = ExpireItems(EXPIRATION_COUNT_PER_RUN, &keepGoing);
  if (NS_FAILED(rv))
    NS_WARNING("ExpireItems failed.");
  else if (keepGoing)
    StartTimer(SUBSEQUENT_EXPIRATION_TIMEOUT);
  return NS_OK;
}

Comment 1

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

Attachment: patch

Comment 2

[Dietrich Ayala (:dietrich)]

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

Thanks seth, i had a similar fix in my local tree for bug 407018. This should go in regardless of whether i figure out the allocations problem. r=me.

Comment 3

[Mike Schroepfer]

So you want this for b2?

Comment 4

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

> So you want this for b2?

If we get bug #407018 for b2, then it isn't so much of an issue (as we should not be failing.)

But I'd still consider taking it, because:

Once you get into the state where you have no more records older than the max-age cap, we'll hit this bug when we start to look for visits older than the min-age cap and over the sites cap.  (Keep in mind thought that once we have dietrich fix for bug #407018, we will not return failure.)

without his fix for bug #407018, after visiting a page, 3.5 seconds later we fire the timer to expire 6 visits.  when that timer fires, we will execute a query to find visits older than the min-age cap.  (good news is this is not a slow query anymore, thanks to marco!)   we'll fail, and then continue to fire this query every 3.5 seconds.

note, this fix prevents this from happening if there are any other unexpected failures in ::ExpiredItems().

additionally, the fix is low risk.

Comment 5

[Mike Schroepfer]

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

Ok lets do it then...

Comment 6

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

fixed, thanks for the approval.

Checking in nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  ns
NavHistoryExpire.cpp
new revision: 1.29; previous revision: 1.28
done

Comment 7

[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