Bug 1344205 - Remove unsatisfied assertion on database page removals and protect removals from orphaning. r=standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 23 Mar 2017 17:51:52 +0100
changeset 349501 fe1b0a83474bc45cc9cd2beded861b165de339bb
parent 349500 43dfa7e2fb45921e69b5b52a70ac23f37e819692
child 349502 41284bd3ce5b3c3d4ecf038197f1cbea7f63747f
push id31552
push userkwierso@gmail.com
push dateSat, 25 Mar 2017 00:03:33 +0000
treeherdermozilla-central@f9acfdca68a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1344205
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
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
@@ -3296,27 +3296,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"));