Bug 1345421 - Part 1: Clean up mechanism used to create fresh processes. r=baku, a=test-only
authorAndrew Sutherland <asutherland@asutherland.org>
Mon, 12 Jun 2017 03:17:17 -0400
changeset 413976 5527f743aec031cd9e798b9d6c7c22c19e2e356a
parent 413975 dbcc401cf103b4d46895300eeb05694799c0a2e6
child 413977 9b401b03ab50ee2d1aa8a1a5ba9c3b139d4c504f
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, test-only
bugs1345421, 1345990
milestone55.0
Bug 1345421 - Part 1: Clean up mechanism used to create fresh processes. r=baku, a=test-only Bug 1345990 introduced a "forceNewProcess" argument in BrowserTestUtils.openNewForegroundTab. By switching to this we can stop bloating the process count pref to try and produce equivalent results. To minimize test churn and because it doesn't really hurt to double-check, the code that asserts that our tabs are each in different processes and related book-keeping infrastructure have been left intact. We also set a preference to disable preallocated processes in the interest of maximizing test consistency and minimizing breakage. It's conceivable that a preallocated process might end up creating its StorageDBParent actor prior to when we want, breaking things. By ensuring the process isn't created until we want it, we avoid a lot of brittleness.
dom/tests/browser/browser_localStorage_e10s.js
--- a/dom/tests/browser/browser_localStorage_e10s.js
+++ b/dom/tests/browser/browser_localStorage_e10s.js
@@ -27,18 +27,20 @@ class KnownTabs {
   }
 }
 
 /**
  * Open our helper page in a tab in its own content process, asserting that it
  * really is in its own process.
  */
 function* openTestTabInOwnProcess(name, knownTabs) {
-  let url = HELPER_PAGE_URL + '?' + encodeURIComponent(name);
-  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url);
+  let opening = HELPER_PAGE_URL + '?' + encodeURIComponent(name);
+  let tab = yield BrowserTestUtils.openNewForegroundTab({
+    gBrowser, opening, forceNewProcess: true
+  });
   let pid = tab.linkedBrowser.frameLoader.tabParent.osPid;
   ok(!knownTabs.byName.has(name), "tab needs its own name: " + name);
   ok(!knownTabs.byPid.has(pid), "tab needs to be in its own process: " + pid);
 
   let knownTab = new KnownTab(name, tab);
   knownTabs.byPid.set(pid, knownTab);
   knownTabs.byName.set(name, knownTab);
   return knownTab;
@@ -213,59 +215,32 @@ requestLongerTimeout(4);
  * - Open a "lateWriteThenListen" tab that initially does nothing.  We will
  *   later tell it to issue a write and then listen for events to make sure it
  *   captures the later events.
  * - Open "lateOpenSeesPreload" tab after we've done everything and ensure that
  *   it preloads/precaches the data without us having touched localStorage or
  *   added an event listener.
  */
 add_task(function*() {
-  // - Boost process count so all of our tabs get new processes.
-  // Our test wants to assert things about the precache status which is only
-  // populated at process startup and never updated.  (Per analysis at
-  // https://bugzilla.mozilla.org/show_bug.cgi?id=1312022 this still makes
-  // sense.)  https://bugzilla.mozilla.org/show_bug.cgi?id=1312022 introduced
-  // a mechanism for keeping an arbitrary number of processes alive, modifying
-  // all browser chrome tests to keep alive whatever dom.ipc.processCount is set
-  // to.  The mechanism was slightly modified later to be type-based, so now
-  // it's "dom.ipc.keepProcessesAlive.web" we care about.
-  //
-  // Our options for ensuring we get a new process are to either:
-  // 1) Try and push keepalive down to 1 and kill off the processes that are
-  //    already hanging around.
-  // 2) Just bump the process count up enough so that every tab we open will
-  //    get a new process.
-  //
-  // The first option turns out to be hard to get right.  Specifically,
-  // although one can set the keepalive and process counts to 1 and open and
-  // close tabs to try and trigger process termination down to 1, since we don't
-  // know how many processes might exist, we can't reliably listen for observer
-  // notifications of their shutdown to ensure we're avoiding shutdown races.
-  // (If there are races then the processes won't actually be shut down.)  So
-  // it's easiest to just boost the limit.
-  let keepAliveCount = 0;
-  try {
-    // This will throw if the preference is not defined, leaving our value at 0.
-    // Alternately, we could use Preferences.jsm's Preferences.get() API which
-    // supports default values, but we're sticking with SpecialPowers here for
-    // consistency.
-    keepAliveCount = SpecialPowers.getIntPref("dom.ipc.keepProcessesAlive.web");
-  } catch (ex) {
-    // Then zero is correct.
-  }
-  let safeProcessCount = keepAliveCount + 6;
-  info("dom.ipc.keepProcessesAlive.web is " + keepAliveCount + ", boosting " +
-       "process count temporarily to " + safeProcessCount);
-
-  // (There's already one about:blank page open and we open 5 new tabs, so 6
-  // processes.  Actually, 7, just in case.)
   yield SpecialPowers.pushPrefEnv({
     set: [
-      ["dom.ipc.processCount", safeProcessCount],
-      ["dom.ipc.processCount.web", safeProcessCount]
+      // Stop the preallocated process manager from speculatively creating
+      // processes.  Our test explicitly asserts on whether preload happened or
+      // not for each tab's process.  This information is loaded and latched by
+      // the StorageDBParent constructor which the child process's
+      // LocalStorageManager() constructor causes to be created via a call to
+      // LocalStorageCache::StartDatabase().  Although the service is lazily
+      // created and should not have been created prior to our opening the tab,
+      // it's safest to ensure the process simply didn't exist before we ask for
+      // it.
+      //
+      // This is done in conjunction with our use of forceNewProcess when
+      // opening tabs.  There would be no point if we weren't also requesting a
+      // new process.
+      ["dom.ipc.processPrelaunch.enabled", false],
     ]
   });
 
   // Ensure that there is no localstorage data or potential false positives for
   // localstorage preloads by forcing the origin to be cleared prior to the
   // start of our test.
   clearOriginStorageEnsuringNoPreload();