Bug 1344205 - Remove unsatisfied assertion on database page removals and protect removals from orphaning. r=standard8 a=gchang
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 23 Mar 2017 17:51:52 +0100
changeset 379258 404583ced67a4c7c1c9c8ed538de5f9d2fc18b3b
parent 379257 1b0b9e1915d147b4e904ac07a5f16677035600ce
child 379259 b387337a67283b82eab7d1ea6bf38b811e699702
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8, gchang
bugs1344205
milestone53.0
Bug 1344205 - Remove unsatisfied assertion on database page removals and protect removals from orphaning. r=standard8 a=gchang Due to the asynchronous behavior of history, it's possible a race condition causes a page to be used when it was about to be removed. For example a bookmark to a page could be added between the point where we decide the page is an orphan, and the point where we actually remove the page. Notifications in such a case may be improperly ordered, and there is no cheap way to guarantee we won't notify a removal in such a case. Since applicable solutions would be too expensive (code and perf wise), for now just remove the assertion we can't satisfy, and protect removals from possibly creating unexpected orphans. Testing this would require a special setup where we can guarantee thread scheduling order, so for now this comes without a test. Regardless it's basically only adding safety guards. MozReview-Commit-ID: KYkaqAv8fCB
toolkit/components/places/History.jsm
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsPlacesExpiration.js
toolkit/components/places/tests/unit/test_null_interfaces.js
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -692,17 +692,20 @@ var clear = Task.async(function* (db) {
  */
 var cleanupPages = Task.async(function*(db, pages) {
   yield invalidateFrecencies(db, pages.filter(p => p.hasForeign || p.hasVisits).map(p => p.id));
 
   let pageIdsToRemove = pages.filter(p => !p.hasForeign && !p.hasVisits).map(p => p.id);
   if (pageIdsToRemove.length > 0) {
     let idsList = sqlList(pageIdsToRemove);
     // Note, we are already in a transaction, since callers create it.
-    yield db.execute(`DELETE FROM moz_places WHERE id IN ( ${ idsList } )`);
+    // Check relations regardless, to avoid creating orphans in case of
+    // async race conditions.
+    yield db.execute(`DELETE FROM moz_places WHERE id IN ( ${ idsList } )
+                      AND foreign_count = 0 AND last_visit_date ISNULL`);
     // Hosts accumulated during the places delete are updated through a trigger
     // (see nsPlacesTriggers.h).
     yield db.executeCached(`DELETE FROM moz_updatehosts_temp`);
 
     // Expire orphans.
     yield db.executeCached(`
       DELETE FROM moz_favicons WHERE NOT EXISTS
         (SELECT 1 FROM moz_places WHERE favicon_id = moz_favicons.id)`);
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -3241,27 +3241,16 @@ nsNavBookmarks::OnVisit(nsIURI* aURI, in
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnDeleteURI(nsIURI* aURI,
                             const nsACString& aGUID,
                             uint16_t aReason)
 {
-  NS_ENSURE_ARG(aURI);
-
-#ifdef DEBUG
-  nsNavHistory* history = nsNavHistory::GetHistoryService();
-  int64_t placeId;
-  nsAutoCString placeGuid;
-  MOZ_ASSERT(
-    history && NS_SUCCEEDED(history->GetIdForPage(aURI, &placeId, placeGuid)) && !placeId,
-    "OnDeleteURI was notified for a page that still exists?"
-  );
-#endif
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnClearHistory()
 {
   // TODO(bryner): we should notify on visited-time change for all URIs
--- a/toolkit/components/places/nsPlacesExpiration.js
+++ b/toolkit/components/places/nsPlacesExpiration.js
@@ -236,17 +236,17 @@ const EXPIRATION_QUERIES = {
     actions: ACTION.TIMED | ACTION.TIMED_OVERLIMIT | ACTION.SHUTDOWN_DIRTY |
              ACTION.IDLE_DIRTY | ACTION.IDLE_DAILY | ACTION.DEBUG
   },
 
   // Expire found URIs from the database.
   QUERY_EXPIRE_URIS: {
     sql: `DELETE FROM moz_places WHERE id IN (
             SELECT p_id FROM expiration_notify WHERE p_id NOTNULL
-          )`,
+          ) AND foreign_count = 0 AND last_visit_date ISNULL`,
     actions: ACTION.TIMED | ACTION.TIMED_OVERLIMIT | ACTION.SHUTDOWN_DIRTY |
              ACTION.IDLE_DIRTY | ACTION.IDLE_DAILY | ACTION.DEBUG
   },
 
   // Expire orphan URIs from the database.
   QUERY_SILENT_EXPIRE_ORPHAN_URIS: {
     sql: `DELETE FROM moz_places WHERE id IN (
             SELECT h.id
--- a/toolkit/components/places/tests/unit/test_null_interfaces.js
+++ b/toolkit/components/places/tests/unit/test_null_interfaces.js
@@ -11,17 +11,17 @@
 var testServices = [
   ["browser/nav-history-service;1",
     ["nsINavHistoryService"],
     ["queryStringToQueries", "removePagesByTimeframe", "removePagesFromHost", "getObservers"]
   ],
   ["browser/nav-bookmarks-service;1",
     ["nsINavBookmarksService", "nsINavHistoryObserver", "nsIAnnotationObserver"],
     ["createFolder", "getObservers", "onFrecencyChanged", "onTitleChanged",
-     "onPageAnnotationSet", "onPageAnnotationRemoved"]
+     "onPageAnnotationSet", "onPageAnnotationRemoved", "onDeleteURI"]
   ],
   ["browser/livemark-service;2", ["mozIAsyncLivemarks"], ["reloadLivemarks"]],
   ["browser/annotation-service;1", ["nsIAnnotationService"], ["getObservers"]],
   ["browser/favicon-service;1", ["nsIFaviconService"], []],
   ["browser/tagging-service;1", ["nsITaggingService"], []],
 ];
 do_print(testServices.join("\n"));