From bc5544c0367e6e88051b7ae4c72ef9e09fbd0c4e Mon Sep 17 00:00:00 2001 From: "dietrich@mozilla.com" Date: Fri, 24 Aug 2007 10:40:55 -0700 Subject: [PATCH] Bug 393472 EXPIRE_NEVER annos can be orphaned when moz_places records are deleted (r=sspitzer) Bug 387573 ExpireHistoryParanoid does not cleanup all unused values (for mak77@supereva.it, r=dietrich) --- .../places/src/nsNavHistoryExpire.cpp | 96 +++++++++++++------ .../places/tests/unit/test_expiration.js | 32 ++++--- 2 files changed, 86 insertions(+), 42 deletions(-) diff --git a/toolkit/components/places/src/nsNavHistoryExpire.cpp b/toolkit/components/places/src/nsNavHistoryExpire.cpp index 594d4d398bba..75ce80fc9659 100644 --- a/toolkit/components/places/src/nsNavHistoryExpire.cpp +++ b/toolkit/components/places/src/nsNavHistoryExpire.cpp @@ -153,11 +153,13 @@ void nsNavHistoryExpire::OnDeleteURI() { mozIStorageConnection* connection = mHistory->GetStorageConnection(); - if (! connection) { + if (!connection) { NS_NOTREACHED("No connection"); return; } - ExpireAnnotations(connection); + nsresult rv = ExpireAnnotations(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireAnnotations failed."); } // nsNavHistoryExpire::OnQuit @@ -168,7 +170,7 @@ void nsNavHistoryExpire::OnQuit() { mozIStorageConnection* connection = mHistory->GetStorageConnection(); - if (! connection) { + if (!connection) { NS_NOTREACHED("No connection"); return; } @@ -178,12 +180,20 @@ nsNavHistoryExpire::OnQuit() mTimer->Cancel(); // Handle degenerate runs: - ExpireForDegenerateRuns(); + nsresult rv = ExpireForDegenerateRuns(); + if (NS_FAILED(rv)) + NS_WARNING("ExpireForDegenerateRuns failed."); // vacuum up dangling items - ExpireHistoryParanoid(connection); - ExpireFaviconsParanoid(connection); - ExpireAnnotationsParanoid(connection); + rv = ExpireHistoryParanoid(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireHistoryParanoid failed."); + rv = ExpireFaviconsParanoid(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireFaviconsParanoid failed."); + rv = ExpireAnnotationsParanoid(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireAnnotationsParanoid failed."); } @@ -201,11 +211,21 @@ nsNavHistoryExpire::ClearHistory() mozIStorageConnection* connection = mHistory->GetStorageConnection(); NS_ENSURE_TRUE(connection, NS_ERROR_OUT_OF_MEMORY); - ExpireItems(0, &keepGoing); + nsresult rv = ExpireItems(0, &keepGoing); + if (NS_FAILED(rv)) + NS_WARNING("ExpireItems failed."); - ExpireHistoryParanoid(connection); - ExpireFaviconsParanoid(connection); - ExpireAnnotationsParanoid(connection); + rv = ExpireHistoryParanoid(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireHistoryParanoid failed."); + + rv = ExpireFaviconsParanoid(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireFaviconsParanoid failed."); + + rv = ExpireAnnotationsParanoid(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireAnnotationsParanoid failed."); ENUMERATE_WEAKARRAY(mHistory->mObservers, nsINavHistoryObserver, OnClearHistory()) @@ -236,7 +256,9 @@ nsNavHistoryExpire::DoPartialExpiration() // expire history items PRBool keepGoing; - ExpireItems(EXPIRATION_COUNT_PER_RUN, &keepGoing); + nsresult rv = ExpireItems(EXPIRATION_COUNT_PER_RUN, &keepGoing); + if (NS_FAILED(rv)) + NS_WARNING("ExpireItems failed."); if (keepGoing && mSequentialRuns < MAX_SEQUENTIAL_RUNS) StartTimer(SUBSEQUENT_EXIPRATION_TIMEOUT); @@ -320,11 +342,18 @@ nsNavHistoryExpire::ExpireItems(PRUint32 aNumToExpire, PRBool* aKeepGoing) } // don't worry about errors here, it doesn't affect our ability to continue - EraseFavicons(connection, expiredVisits); - EraseAnnotations(connection, expiredVisits); + rv = EraseFavicons(connection, expiredVisits); + if (NS_FAILED(rv)) + NS_WARNING("EraseFavicons failed."); + + rv = EraseAnnotations(connection, expiredVisits); + if (NS_FAILED(rv)) + NS_WARNING("EraseAnnotations failed."); // expire annotations by policy - ExpireAnnotations(connection); + rv = ExpireAnnotations(connection); + if (NS_FAILED(rv)) + NS_WARNING("ExpireAnnotations failed."); rv = transaction.Commit(); NS_ENSURE_SUCCESS(rv, rv); @@ -424,7 +453,8 @@ nsNavHistoryExpire::EraseVisits(mozIStorageConnection* aConnection, // nsNavHistoryExpire::EraseHistory // // This erases records in moz_places when there are no more visits. -// We need to be careful not to delete bookmarks and place:URIs. +// We need to be careful not to delete bookmarks, place:URIs and +// URIs with EXPIRE_NEVER annotations. // // This will modify the input by setting the erased flag on each of the // array elements according to whether the history item was erased or not. @@ -455,7 +485,12 @@ nsNavHistoryExpire::EraseHistory(mozIStorageConnection* aConnection, deletedPlaceIds + NS_LITERAL_CSTRING(") AND id IN (SELECT h.id FROM moz_places h " "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id " - "WHERE v.id IS NULL)")); + "WHERE v.id IS NULL) " + "AND id NOT IN (SELECT h.id FROM moz_places h " + "JOIN moz_annos a ON h.id = a.place_id " + "WHERE a.expiration = ") + + nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) + + NS_LITERAL_CSTRING(")")); } @@ -614,19 +649,18 @@ nsNavHistoryExpire::ExpireHistoryParanoid(mozIStorageConnection* aConnection) { // delete history entries with no visits that are not bookmarked // also never delete any "place:" URIs (see function header comment) - nsCOMPtr deleteStatement; - nsresult rv = aConnection->CreateStatement(NS_LITERAL_CSTRING( - "DELETE FROM moz_places WHERE id IN (SELECT h.id FROM moz_places h " + nsresult rv = aConnection->ExecuteSimpleSQL( + NS_LITERAL_CSTRING("DELETE FROM moz_places " + "WHERE id IN (SELECT h.id FROM moz_places h " "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id " "LEFT OUTER JOIN moz_bookmarks b ON h.id = b.fk " + "LEFT OUTER JOIN moz_annos a ON h.id = a.place_id " "WHERE v.id IS NULL " - "AND b.type = ?1 AND b.fk IS NULL " - "AND SUBSTR(h.url,0,6) <> 'place:')"), - getter_AddRefs(deleteStatement)); - NS_ENSURE_SUCCESS(rv, rv); - rv = deleteStatement->BindInt32Parameter(0, nsINavBookmarksService::TYPE_BOOKMARK); - NS_ENSURE_SUCCESS(rv, rv); - rv = deleteStatement->Execute(); + "AND b.id IS NULL " + "AND a.expiration = ") + + nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) + + NS_LITERAL_CSTRING(" AND a.id IS NULL " + "AND SUBSTR(h.url,0,6) <> 'place:')")); NS_ENSURE_SUCCESS(rv, rv); return NS_OK; } @@ -639,11 +673,13 @@ nsNavHistoryExpire::ExpireHistoryParanoid(mozIStorageConnection* aConnection) nsresult nsNavHistoryExpire::ExpireFaviconsParanoid(mozIStorageConnection* aConnection) { - return aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + nsresult rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( "DELETE FROM moz_favicons WHERE id IN " "(SELECT f.id FROM moz_favicons f " "LEFT OUTER JOIN moz_places h ON f.id = h.favicon_id " "WHERE h.favicon_id IS NULL)")); + NS_ENSURE_SUCCESS(rv, rv); + return rv; } @@ -718,7 +754,9 @@ nsNavHistoryExpire::ExpireForDegenerateRuns() // This run looks suspicious, try to expire up to the number of items // we may have missed this session. PRBool keepGoing; - ExpireItems(mAddCount - mExpiredItems, &keepGoing); + nsresult rv = ExpireItems(mAddCount - mExpiredItems, &keepGoing); + if (NS_FAILED(rv)) + NS_WARNING("ExpireItems failed."); return PR_TRUE; } diff --git a/toolkit/components/places/tests/unit/test_expiration.js b/toolkit/components/places/tests/unit/test_expiration.js index 5f3523c3c687..8fc7a5a4c88d 100644 --- a/toolkit/components/places/tests/unit/test_expiration.js +++ b/toolkit/components/places/tests/unit/test_expiration.js @@ -97,6 +97,15 @@ var observer = { }; histsvc.addObserver(observer, false); +// get direct db connection for date-based anno tests +var dirService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties); +var dbFile = dirService.get("ProfD", Ci.nsIFile); +dbFile.append("places.sqlite"); + +var dbService = Cc["@mozilla.org/storage/service;1"].getService(Ci.mozIStorageService); +var dbConnection = dbService.openDatabase(dbFile); + + // main function run_test() { var testURI = uri("http://mozilla.com"); @@ -125,16 +134,21 @@ function run_test() { test that nsIBrowserHistory.removeAllPages does remove expirable annotations but doesn't remove bookmarks or EXPIRE_NEVER annotations. */ - histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0); - annosvc.setPageAnnotation(testURI, testAnnoName + "Hist", testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY); - annosvc.setPageAnnotation(testURI, testAnnoName + "Never", testAnnoVal, 0, annosvc.EXPIRE_NEVER); + var removeAllTestURI = uri("http://removeallpages.com"); + histsvc.addVisit(removeAllTestURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0); + annosvc.setPageAnnotation(removeAllTestURI, testAnnoName + "Hist", testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY); + annosvc.setPageAnnotation(removeAllTestURI, testAnnoName + "Never", testAnnoVal, 0, annosvc.EXPIRE_NEVER); bhist.removeAllPages(); try { annosvc.getPageAnnotation(testAnnoName + "Hist"); do_throw("nsIBrowserHistory.removePagesFromHost() didn't remove an EXPIRE_WITH_HISTORY annotation"); } catch(ex) {} - do_check_eq(annosvc.getPageAnnotation(testURI, testAnnoName + "Never"), testAnnoVal); - annosvc.removePageAnnotation(testURI, testAnnoName + "Never"); + try { + do_check_eq(annosvc.getPageAnnotation(removeAllTestURI, testAnnoName + "Never"), testAnnoVal); + annosvc.removePageAnnotation(removeAllTestURI, testAnnoName + "Never"); + } catch(ex) { + do_throw("nsIBrowserHistory.removeAllPages deleted EXPIRE_NEVER annos!"); + } /* test age-based history and anno expiration via the browser.history_expire_days pref. @@ -154,14 +168,6 @@ function run_test() { do_check_eq(testURI.spec, observer.expiredURI); do_check_eq(annosvc.getPageAnnotationNames(testURI, {}).length, 0); - // get direct db connection for date-based anno tests - var dirService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties); - var dbFile = dirService.get("ProfD", Ci.nsIFile); - dbFile.append("places.sqlite"); - - var dbService = Cc["@mozilla.org/storage/service;1"].getService(Ci.mozIStorageService); - var dbConnection = dbService.openDatabase(dbFile); - /* test anno expiration (expire never) */