Bug 1345421 - Part 2: Ensure mOriginsHavingData is up-to-date via flushing. r=baku, a=test-only
authorAndrew Sutherland <asutherland@asutherland.org>
Mon, 12 Jun 2017 04:52:21 -0400
changeset 413977 9b401b03ab50ee2d1aa8a1a5ba9c3b139d4c504f
parent 413976 5527f743aec031cd9e798b9d6c7c22c19e2e356a
child 413978 64d650c2c2ac836530c9204ccdf372d487d912b2
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
milestone55.0
Bug 1345421 - Part 2: Ensure mOriginsHavingData is up-to-date via flushing. r=baku, a=test-only The intermittent failure appears to have been due to mOriginsHavingData only being updated when the db thread flushes. The db thread has a hard-coded 5 second flush interval. It's likely that e10s startup was previously so slow that we were assured of having a flush happen by the time our fresh process created its parent actor. We correct this by reliably ensuring a flush before spinning up the process to check preload state. We also ensure a flush at the start of the test for our check that there was no preload in the initial cases. We were actually more vulnerable in that case, I believe, but as a browser chrome test, there were no other tests that would have used content localStorage. We additionally ensure that the content process has received and populated mOriginsHavingData by having the tab opening process wait for about:blank to load in the process before actually opening our origin. Prior to this change we were depending on orderings that aren't guaranteed.
dom/tests/browser/browser_localStorage_e10s.js
--- a/dom/tests/browser/browser_localStorage_e10s.js
+++ b/dom/tests/browser/browser_localStorage_e10s.js
@@ -24,45 +24,98 @@ class KnownTabs {
   cleanup() {
     this.byPid = null;
     this.byName = null;
   }
 }
 
 /**
  * Open our helper page in a tab in its own content process, asserting that it
- * really is in its own process.
+ * really is in its own process.  We initially load and wait for about:blank to
+ * load, and only then loadURI to our actual page.  This is to ensure that
+ * LocalStorageManager has had an opportunity to be created and populate
+ * mOriginsHavingData.
+ *
+ * (nsGlobalWindow will reliably create LocalStorageManager as a side-effect of
+ * the unconditional call to nsGlobalWindow::PreloadLocalStorage.  This will
+ * reliably create the StorageDBChild instance, and its corresponding
+ * StorageDBParent will send the set of origins when it is constructed.)
  */
 function* openTestTabInOwnProcess(name, knownTabs) {
-  let opening = HELPER_PAGE_URL + '?' + encodeURIComponent(name);
+  let realUrl = HELPER_PAGE_URL + '?' + encodeURIComponent(name);
+  // Load and wait for about:blank.
   let tab = yield BrowserTestUtils.openNewForegroundTab({
-    gBrowser, opening, forceNewProcess: true
+    gBrowser, opening: 'about:blank', 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);
+
+  // Now trigger the actual load of our page.
+  tab.linkedBrowser.loadURI(realUrl);
+  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
+  is(tab.linkedBrowser.frameLoader.tabParent.osPid, pid, "still same pid");
   return knownTab;
 }
 
 /**
  * Close all the tabs we opened.
  */
 function* cleanupTabs(knownTabs) {
   for (let knownTab of knownTabs.byName.values()) {
     yield BrowserTestUtils.removeTab(knownTab.tab);
     knownTab.cleanup();
   }
   knownTabs.cleanup();
 }
 
 /**
+ * Wait for a LocalStorage flush to occur.  This notification can occur as a
+ * result of any of:
+ * - The normal, hardcoded 5-second flush timer.
+ * - InsertDBOp seeing a preload op for an origin with outstanding changes.
+ * - Us generating a "domstorage-test-flush-force" observer notification.
+ */
+function waitForLocalStorageFlush() {
+  return new Promise(function(resolve) {
+    let observer = {
+      observe: function() {
+        SpecialPowers.removeObserver(observer, "domstorage-test-flushed");
+        resolve();
+      }
+    };
+    SpecialPowers.addObserver(observer, "domstorage-test-flushed");
+  });
+}
+
+/**
+ * Trigger and wait for a flush.  This is only necessary for forcing
+ * mOriginsHavingData to be updated.  Normal operations exposed to content know
+ * to automatically flush when necessary for correctness.
+ *
+ * The notification we're waiting for to verify flushing is fundamentally
+ * ambiguous (see waitForLocalStorageFlush), so we actually trigger the flush
+ * twice and wait twice.  In the event there was a race, there will be 3 flush
+ * notifications, but correctness is guaranteed after the second notification.
+ */
+function triggerAndWaitForLocalStorageFlush() {
+  SpecialPowers.notifyObservers(null, "domstorage-test-flush-force");
+  // This first wait is ambiguous...
+  return waitForLocalStorageFlush().then(function() {
+    // So issue a second flush and wait for that.
+    SpecialPowers.notifyObservers(null, "domstorage-test-flush-force");
+    return waitForLocalStorageFlush();
+  })
+}
+
+/**
  * Clear the origin's storage so that "OriginsHavingData" will return false for
  * our origin.  Note that this is only the case for AsyncClear() which is
  * explicitly issued against a cache, or AsyncClearAll() which we can trigger
  * by wiping all storage.  However, the more targeted domain clearings that
  * we can trigger via observer, AsyncClearMatchingOrigin and
  * AsyncClearMatchingOriginAttributes will not clear the hashtable entry for
  * the origin.
  *
@@ -74,19 +127,20 @@ function clearOriginStorageEnsuringNoPre
   let principal =
     Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(
       HELPER_PAGE_ORIGIN);
   // We want to use createStorage to force the cache to be created so we can
   // issue the clear.  It's possible for getStorage to return false but for the
   // origin preload hash to still have our origin in it.
   let storage = Services.domStorageManager.createStorage(null, principal, "");
   storage.clear();
-  // We don't need to wait for anything.  The clear call will have queued the
-  // clear operation on the database thread, and the child process requests
-  // for origins will likewise be answered via the database thread.
+
+  // We also need to trigger a flush os that mOriginsHavingData gets updated.
+  // The inherent flush race is fine here because
+  return triggerAndWaitForLocalStorageFlush();
 }
 
 function* verifyTabPreload(knownTab, expectStorageExists) {
   let storageExists = yield ContentTask.spawn(
     knownTab.tab.linkedBrowser,
     HELPER_PAGE_ORIGIN,
     function(origin) {
       let principal =
@@ -231,23 +285,29 @@ add_task(function*() {
       // 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],
+      // Enable LocalStorage's testing API so we can explicitly trigger a flush
+      // when needed.
+      ["dom.storage.testing", true],
     ]
   });
 
   // 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();
+  yield clearOriginStorageEnsuringNoPreload();
+
+  // Make sure mOriginsHavingData gets updated.
+  yield triggerAndWaitForLocalStorageFlush();
 
   // - Open tabs.  Don't configure any of them yet.
   const knownTabs = new KnownTabs();
   const writerTab = yield* openTestTabInOwnProcess("writer", knownTabs);
   const listenerTab = yield* openTestTabInOwnProcess("listener", knownTabs);
   const readerTab = yield* openTestTabInOwnProcess("reader", knownTabs);
   const lateWriteThenListenTab = yield* openTestTabInOwnProcess(
     "lateWriteThenListen", knownTabs);
@@ -326,16 +386,24 @@ add_task(function*() {
 
   yield* verifyTabStorageState(writerTab, lastWriteState);
   yield* verifyTabStorageEvents(listenerTab, lastWriteMutations);
   yield* verifyTabStorageState(listenerTab, lastWriteState);
   yield* verifyTabStorageState(readerTab, lastWriteState);
   yield* verifyTabStorageEvents(lateWriteThenListenTab, lastWriteMutations);
   yield* verifyTabStorageState(lateWriteThenListenTab, lastWriteState);
 
+  // - Force a LocalStorage DB flush so mOriginsHavingData is updated.
+  // mOriginsHavingData is only updated when the storage thread runs its
+  // accumulated operations during the flush.  If we don't initiate and ensure
+  // that a flush has occurred before moving on to the next step,
+  // mOriginsHavingData may not include our origin when it's sent down to the
+  // child process.
+  yield triggerAndWaitForLocalStorageFlush();
+
   // - Open a fresh tab and make sure it sees the precache/preload
   const lateOpenSeesPreload =
     yield* openTestTabInOwnProcess("lateOpenSeesPreload", knownTabs);
   yield* verifyTabPreload(lateOpenSeesPreload, true);
 
   // - Clean up.
   yield* cleanupTabs(knownTabs);