Bug 1344205 - Remove unsatisfied assertion on database page removals and protect removals from orphaning. r=standard8

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

--HG--
extra : rebase_source : 79402fb533c7e075c6e2ba44ebc1ec1b8f1084ee

toolkit/components/places/History.jsm

@@ -697,7 +697,10 @@ var cleanupPages = Task.async(function*(db, pages) {
   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`);

toolkit/components/places/nsNavBookmarks.cpp

@@ -3301,17 +3301,6 @@ 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;
 }
 

toolkit/components/places/nsPlacesExpiration.js

@@ -241,7 +241,7 @@ const EXPIRATION_QUERIES = {
   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
   },

toolkit/components/places/tests/unit/test_null_interfaces.js

@@ -16,7 +16,7 @@ var testServices = [
   ["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"]],