Bug 1554167 - Remove pending shutdown sanitization immediately after shutdown sanitization finishes. r=mak,baku a=jcristau
authorJohann Hofmann <jhofmann@mozilla.com>
Thu, 30 May 2019 15:39:39 +0000
changeset 536632 736e8d1c1c44873cbd829ef9b97f890111700b51
parent 536631 e87602ae8d9f91280a079e72d47cedd9a394abcb
child 536633 fa85a070d83364097722aeb109b110eba62b51f8
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, baku, jcristau
bugs1554167
milestone68.0
Bug 1554167 - Remove pending shutdown sanitization immediately after shutdown sanitization finishes. r=mak,baku a=jcristau In https://hg.mozilla.org/mozilla-central/rev/25397a6f8c4f#l1.35 we added an early return to the SanitizeOnShutdown function to avoid cleaning principals by permission if the user had set their preferences to clear all storage on shutdown anyway. This unfortunately ended the function execution before it would call `removePendingSanitization("shutdown");` later on and thus remove the pending shutdown sanitization (which, in fact, had completed successfully earlier). The result is that the shutdown sanitization would be left dangling and run again on next startup, where, for reasons I don't fully understand, it would race and conflict with loading the home page, if that home page was from web content. The solution is to remove the pending shutdown sanitization immediately after the sanitization is done. As far as I can see there was never really a point in having it happen after session principal cleanup finished, since in case of a crash it would not run the principal cleanup again next startup, just the shutdown cleanup. For good measure I also moved the new tab container sanitization to happen earlier in this function, to prevent it from dangling as well. Differential Revision: https://phabricator.services.mozilla.com/D33087
browser/modules/Sanitizer.jsm
--- a/browser/modules/Sanitizer.jsm
+++ b/browser/modules/Sanitizer.jsm
@@ -740,21 +740,38 @@ class PrincipalsCollector {
     progress.step = "total-principals:" + principals.length;
     return principals;
   }
 }
 
 async function sanitizeOnShutdown(progress) {
   log("Sanitizing on shutdown");
 
+  let needsSyncSavePrefs = false;
   if (Sanitizer.shouldSanitizeOnShutdown) {
     // Need to sanitize upon shutdown
     progress.advancement = "shutdown-cleaner";
     let itemsToClear = getItemsToClearFromPrefBranch(Sanitizer.PREF_SHUTDOWN_BRANCH);
     await Sanitizer.sanitize(itemsToClear, { progress });
+
+    // We didn't crash during shutdown sanitization, so annotate it to avoid
+    // sanitizing again on startup.
+    removePendingSanitization("shutdown");
+    needsSyncSavePrefs = true;
+  }
+
+  if (Sanitizer.shouldSanitizeNewTabContainer) {
+    progress.advancement = "newtab-segregation";
+    sanitizeNewTabSegregation();
+    removePendingSanitization("newtab-container");
+    needsSyncSavePrefs = true;
+  }
+
+  if (needsSyncSavePrefs) {
+    Services.prefs.savePrefFile(null);
   }
 
   let principalsCollector = new PrincipalsCollector();
 
   // Clear out QuotaManager storage for principals that have been marked as
   // session only.  The cookie service has special logic that avoids writing
   // such cookies to disk, but QuotaManager always touches disk, so we need to
   // wipe the data on shutdown (or startup if we failed to wipe it at
@@ -805,29 +822,16 @@ async function sanitizeOnShutdown(progre
     log("Custom session cookie permission detected for: " + permission.principal.URI.spec);
 
     // We use just the URI here, because permissions ignore OriginAttributes.
     let principals = await principalsCollector.getAllPrincipals(progress);
     let selectedPrincipals = extractMatchingPrincipals(principals, permission.principal.URI);
     await maybeSanitizeSessionPrincipals(progress, selectedPrincipals);
   }
 
-  if (Sanitizer.shouldSanitizeNewTabContainer) {
-    progress.advancement = "newtab-segregation";
-    sanitizeNewTabSegregation();
-    removePendingSanitization("newtab-container");
-  }
-
-  if (Sanitizer.shouldSanitizeOnShutdown) {
-    // We didn't crash during shutdown sanitization, so annotate it to avoid
-    // sanitizing again on startup.
-    removePendingSanitization("shutdown");
-    Services.prefs.savePrefFile(null);
-  }
-
   progress.advancement = "done";
 }
 
 // Extracts the principals matching matchUri as root domain.
 function extractMatchingPrincipals(principals, matchUri) {
   return principals.filter(principal => {
     return Services.eTLD.hasRootDomain(matchUri.host, principal.URI.host);
   });