Bug 1350637 - Part 10: Update LocalStorage e10s tests for change to PBackground. r=janv
authorAndrew Sutherland <asutherland@asutherland.org>
Mon, 07 Aug 2017 04:14:17 -0400
changeset 373419 64f5a7a51c21
parent 373418 bbc3dc385fac
child 373420 042120b49ceb
push id93520
push userjvarga@mozilla.com
push dateTue, 08 Aug 2017 21:35:35 +0000
treeherdermozilla-inbound@042120b49ceb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanv
bugs1350637
milestone57.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 1350637 - Part 10: Update LocalStorage e10s tests for change to PBackground. r=janv The e10s tests were written assuming a world where ContentTask.spawn and LocalStorage were both PContent. Also, that Quantum DOM labeling wasn't something to worry about. These assumptions no longer held, resulting in the test intermittently failing if all changes hadn't propagated via PBackground to the tab under test by the time the ContentTask.spawn state retrieval call made it to the tab's main thread. This has been corrected by using "storage" events where already in use and polling where not in use. Plase see the added comments for more details.
dom/tests/browser/browser_localStorage_e10s.js
dom/tests/browser/page_localstorage_e10s.html
--- a/dom/tests/browser/browser_localStorage_e10s.js
+++ b/dom/tests/browser/browser_localStorage_e10s.js
@@ -150,49 +150,57 @@ async function verifyTabPreload(knownTab
     });
   is(storageExists, expectStorageExists, "Storage existence === preload");
 }
 
 /**
  * Instruct the given tab to execute the given series of mutations.  For
  * simplicity, the mutations representation matches the expected events rep.
  */
-async function mutateTabStorage(knownTab, mutations) {
+async function mutateTabStorage(knownTab, mutations, sentinelValue) {
   await ContentTask.spawn(
     knownTab.tab.linkedBrowser,
-    { mutations },
+    { mutations, sentinelValue },
     function(args) {
-      return content.wrappedJSObject.mutateStorage(args.mutations);
+      return content.wrappedJSObject.mutateStorage(args);
     });
 }
 
 /**
  * Instruct the given tab to add a "storage" event listener and record all
  * received events.  verifyTabStorageEvents is the corresponding method to
  * check and assert the recorded events.
  */
-async function recordTabStorageEvents(knownTab) {
+async function recordTabStorageEvents(knownTab, sentinelValue) {
   await ContentTask.spawn(
     knownTab.tab.linkedBrowser,
-    {},
-    function() {
-      return content.wrappedJSObject.listenForStorageEvents();
+    sentinelValue,
+    function(sentinelValue) {
+      return content.wrappedJSObject.listenForStorageEvents(sentinelValue);
     });
 }
 
 /**
  * Retrieve the current localStorage contents perceived by the tab and assert
  * that they match the provided expected state.
+ *
+ * If maybeSentinel is non-null, it's assumed to be a string that identifies the
+ * value we should be waiting for the sentinel key to take on.  This is
+ * necessary because we cannot make any assumptions about when state will be
+ * propagated to the given process.  See the comments in
+ * page_localstorage_e10s.js for more context.  In general, a sentinel value is
+ * required for correctness unless the process in question is the one where the
+ * writes were performed or verifyTabStorageEvents was used.
  */
-async function verifyTabStorageState(knownTab, expectedState) {
+async function verifyTabStorageState(knownTab, expectedState, maybeSentinel) {
   let actualState = await ContentTask.spawn(
     knownTab.tab.linkedBrowser,
-    {},
-    function() {
-      return content.wrappedJSObject.getStorageState();
+    maybeSentinel,
+    function(maybeSentinel) {
+      return content.wrappedJSObject.getStorageState(maybeSentinel);
     });
 
   for (let [expectedKey, expectedValue] of Object.entries(expectedState)) {
     ok(actualState.hasOwnProperty(expectedKey), "key present: " + expectedKey);
     is(actualState[expectedKey], expectedValue, "value correct");
   }
   for (let actualKey of Object.keys(actualState)) {
     if (!expectedState.hasOwnProperty(actualKey)) {
@@ -200,16 +208,19 @@ async function verifyTabStorageState(kno
     }
   }
 }
 
 /**
  * Retrieve and clear the storage events recorded by the tab and assert that
  * they match the provided expected events.  For simplicity, the expected events
  * representation is the same as that used by mutateTabStorage.
+ *
+ * Note that by convention for test readability we are passed a 3rd argument of
+ * the sentinel value, but we don't actually care what it is.
  */
 async function verifyTabStorageEvents(knownTab, expectedEvents) {
   let actualEvents = await ContentTask.spawn(
     knownTab.tab.linkedBrowser,
     {},
     function() {
       return content.wrappedJSObject.returnAndClearStorageEvents();
     });
@@ -313,19 +324,22 @@ add_task(async function() {
     "lateWriteThenListen", knownTabs);
 
   // Sanity check that preloading did not occur in the tabs.
   await verifyTabPreload(writerTab, false);
   await verifyTabPreload(listenerTab, false);
   await verifyTabPreload(readerTab, false);
 
   // - Configure the tabs.
-  await recordTabStorageEvents(listenerTab);
+  const initialSentinel = 'initial';
+  const noSentinelCheck = null;
+  await recordTabStorageEvents(listenerTab, initialSentinel);
 
   // - Issue the initial batch of writes and verify.
+  info("initial writes");
   const initialWriteMutations = [
     //[key (null=clear), newValue (null=delete), oldValue (verification)]
     ["getsCleared", "1", null],
     ["alsoGetsCleared", "2", null],
     [null, null, null],
     ["stays", "3", null],
     ["clobbered", "pre", null],
     ["getsDeletedLater", "4", null],
@@ -336,75 +350,122 @@ add_task(async function() {
     ["clobbered", "post", "pre"]
   ];
   const initialWriteState = {
     stays: "3",
     clobbered: "post",
     alsoStays: "6"
   };
 
-  await mutateTabStorage(writerTab, initialWriteMutations);
+  await mutateTabStorage(writerTab, initialWriteMutations, initialSentinel);
 
-  await verifyTabStorageState(writerTab, initialWriteState);
-  await verifyTabStorageEvents(listenerTab, initialWriteMutations);
-  await verifyTabStorageState(listenerTab, initialWriteState);
-  await verifyTabStorageState(readerTab, initialWriteState);
+  // We expect the writer tab to have the correct state because it just did the
+  // writes.  We do not perform a sentinel-check because the writes should be
+  // locally available and consistent.
+  await verifyTabStorageState(writerTab, initialWriteState, noSentinelCheck);
+  // We expect the listener tab to have heard all events despite preload not
+  // having occurred and despite not issuing any reads or writes itself.  We
+  // intentionally check the events before the state because we're most
+  // interested in adding the listener having had a side-effect of subscribing
+  // to changes for the process.
+  //
+  // We ensure it had a chance to hear all of the events because we told
+  // recordTabStorageEvents to listen for the given sentinel.  The state check
+  // then does not need to do a sentinel check.
+  await verifyTabStorageEvents(
+    listenerTab, initialWriteMutations, initialSentinel);
+  await verifyTabStorageState(
+    listenerTab, initialWriteState, noSentinelCheck);
+  // We expect the reader tab to retrieve the current localStorage state from
+  // the database.  Because of the above checks, we are confident that the
+  // writes have hit PBackground and therefore that the (synchronous) state
+  // retrieval contains all the data we need.  No sentinel-check is required.
+  await verifyTabStorageState(readerTab, initialWriteState, noSentinelCheck);
 
   // - Issue second set of writes from lateWriteThenListen
+  // This tests that our new tab that begins by issuing only writes is building
+  // on top of the existing state (although we don't verify that until after the
+  // next set of mutations).  We also verify that the initial "writerTab" that
+  // was our first tab and started with only writes sees the writes, even though
+  // it did not add an event listener.
+
+  info("late writes");
+  const lateWriteSentinel = 'lateWrite';
   const lateWriteMutations = [
     ["lateStays", "10", null],
     ["lateClobbered", "latePre", null],
     ["lateDeleted", "11", null],
     ["lateClobbered", "lastPost", "latePre"],
     ["lateDeleted", null, "11"]
   ];
   const lateWriteState = Object.assign({}, initialWriteState, {
     lateStays: "10",
     lateClobbered: "lastPost"
   });
 
-  await mutateTabStorage(lateWriteThenListenTab, lateWriteMutations);
-  await recordTabStorageEvents(lateWriteThenListenTab);
+  await recordTabStorageEvents(listenerTab, lateWriteSentinel);
+
+  await mutateTabStorage(
+    lateWriteThenListenTab, lateWriteMutations, lateWriteSentinel);
 
-  await verifyTabStorageState(writerTab, lateWriteState);
-  await verifyTabStorageEvents(listenerTab, lateWriteMutations);
-  await verifyTabStorageState(listenerTab, lateWriteState);
-  await verifyTabStorageState(readerTab, lateWriteState);
+  // Verify the writer tab saw the writes.  It has to wait for the sentinel to
+  // appear before checking.
+  await verifyTabStorageState(writerTab, lateWriteState, lateWriteSentinel);
+  // Wait for the sentinel event before checking the events and then the state.
+  await verifyTabStorageEvents(
+    listenerTab, lateWriteMutations, lateWriteSentinel);
+  await verifyTabStorageState(listenerTab, lateWriteState, noSentinelCheck);
+  // We need to wait for the sentinel to show up for the reader.
+  await verifyTabStorageState(readerTab, lateWriteState, lateWriteSentinel);
 
   // - Issue last set of writes from writerTab.
+  info("last set of writes");
+  const lastWriteSentinel = 'lastWrite';
   const lastWriteMutations = [
     ["lastStays", "20", null],
     ["lastDeleted", "21", null],
     ["lastClobbered", "lastPre", null],
     ["lastClobbered", "lastPost", "lastPre"],
     ["lastDeleted", null, "21"]
   ];
   const lastWriteState = Object.assign({}, lateWriteState, {
     lastStays: "20",
     lastClobbered: "lastPost"
   });
 
-  await mutateTabStorage(writerTab, lastWriteMutations);
+  await recordTabStorageEvents(listenerTab, lastWriteSentinel);
+  await recordTabStorageEvents(lateWriteThenListenTab, lastWriteSentinel);
+
+  await mutateTabStorage(writerTab, lastWriteMutations, lastWriteSentinel);
 
-  await verifyTabStorageState(writerTab, lastWriteState);
-  await verifyTabStorageEvents(listenerTab, lastWriteMutations);
-  await verifyTabStorageState(listenerTab, lastWriteState);
-  await verifyTabStorageState(readerTab, lastWriteState);
-  await verifyTabStorageEvents(lateWriteThenListenTab, lastWriteMutations);
-  await verifyTabStorageState(lateWriteThenListenTab, lastWriteState);
+  // The writer performed the writes, no need to wait for the sentinel.
+  await verifyTabStorageState(writerTab, lastWriteState, noSentinelCheck);
+  // Wait for the sentinel event to be received, then check.
+  await verifyTabStorageEvents(
+    listenerTab, lastWriteMutations, lastWriteSentinel);
+  await verifyTabStorageState(listenerTab, lastWriteState, noSentinelCheck);
+  // We need to wait for the sentinel to show up for the reader.
+  await verifyTabStorageState(readerTab, lastWriteState, lastWriteSentinel);
+  // Wait for the sentinel event to be received, then check.
+  await verifyTabStorageEvents(
+    lateWriteThenListenTab, lastWriteMutations, lastWriteSentinel);
+  await verifyTabStorageState(
+    lateWriteThenListenTab, lastWriteState, noSentinelCheck);
 
   // - 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.
+  info("flush to make preload check work");
   await triggerAndWaitForLocalStorageFlush();
 
   // - Open a fresh tab and make sure it sees the precache/preload
+  info("late open preload check");
   const lateOpenSeesPreload =
     await openTestTabInOwnProcess("lateOpenSeesPreload", knownTabs);
   await verifyTabPreload(lateOpenSeesPreload, true);
 
   // - Clean up.
   await cleanupTabs(knownTabs);
 
   clearOriginStorageEnsuringNoPreload();
--- a/dom/tests/browser/page_localstorage_e10s.html
+++ b/dom/tests/browser/page_localstorage_e10s.html
@@ -1,56 +1,137 @@
 <!doctype html>
 <html>
 <head>
   <meta charset="utf-8">
 <script>
 /**
  * Helper page used by browser_localStorage_e10s.js.
+ *
+ * We expose methods to be invoked by ContentTask.spawn() calls.
+ * ContentTask.spawn() uses the message manager and is PContent-based.  When
+ * LocalStorage was PContent-managed, ordering was inherently ensured so we
+ * could assume each page had already received all relevant events.  Now some
+ * explicit type of coordination is required.
+ *
+ * This gets complicated because:
+ * - LocalStorage is an ugly API that gives us almost unlimited implementation
+ *   flexibility in the face of multiple processes.  It's also an API that sites
+ *   may misuse which may encourage us to leverage that flexibility in the
+ *   future to improve performance at the expense of propagation latency, and
+ *   possibly involving content-observable coalescing of events.
+ * - The Quantum DOM effort and its event labeling and separate task queues and
+ *   green threading and current LocalStorage implementation mean that using
+ *   other PBackground-based APIs such as BroadcastChannel may not provide
+ *   reliable ordering guarantees.  Specifically, it's hard to guarantee that
+ *   a BroadcastChannel postMessage() issued after a series of LocalStorage
+ *   writes won't be received by the target window before the writes are
+ *   perceived.  At least not without constraining the implementations of both
+ *   APIs.
+ * - Some of our tests explicitly want to verify LocalStorage behavior without
+ *   having a "storage" listener, so we can't add a storage listener if the test
+ *   didn't already want one.
+ *
+ * We use 2 approaches for coordination:
+ * 1. If we're already listening for events, we listen for the sentinel value to
+ *    be written.  This is efficient and appropriate in this case.
+ * 2. If we're not listening for events, we use setTimeout(0) to poll the
+ *    localStorage key and value until it changes to our expected value.
+ *    setTimeout(0) eventually clamps to setTimeout(4), so in the event we are
+ *    experiencing delays, we have reasonable, non-CPU-consuming back-off in
+ *    place that leaves the CPU free to time out and fail our test if something
+ *    broke.  This is ugly but makes us less brittle.
+ *
+ * Both of these involve mutateStorage writing the sentinel value at the end of
+ * the batch.  All of our result-returning methods accordingly filter out the
+ * sentinel key/value pair.
  **/
 var pageName = document.location.search.substring(1);
 window.addEventListener(
   "load",
   () => { document.getElementById("pageNameH").textContent = pageName; });
 
-var recordedEvents = null;
-function storageListener(event) {
-  recordedEvents.push([event.key, event.newValue, event.oldValue]);
+// Key that conveys the end of a write batch.  Filtered out from state and
+// events.
+const SENTINEL_KEY = 'WRITE_BATCH_SENTINEL';
+
+var storageEventsPromise = null;
+function listenForStorageEvents(sentinelValue) {
+  const recordedEvents = [];
+  storageEventsPromise = new Promise(function(resolve, reject) {
+    window.addEventListener(
+      "storage",
+      function thisHandler(event) {
+        if (event.key === SENTINEL_KEY) {
+          // There should be no way for this to have the wrong value, but reject
+          // if it is wrong.
+          if (event.newValue === sentinelValue) {
+            window.removeEventListener("storage", thisHandler);
+            resolve(recordedEvents);
+          } else {
+            reject(event.newValue);
+          }
+        } else {
+          recordedEvents.push([event.key, event.newValue, event.oldValue]);
+        }
+      });
+  });
 }
 
-function listenForStorageEvents() {
-  recordedEvents = [];
-  window.addEventListener("storage", storageListener);
-}
-
-function mutateStorage(mutations) {
+function mutateStorage({ mutations, sentinelValue }) {
   mutations.forEach(function([key, value]) {
     if (key !== null) {
       if (value === null) {
         localStorage.removeItem(key);
       } else {
         localStorage.setItem(key, value);
       }
     } else {
       localStorage.clear();
     }
   });
+  localStorage.setItem(SENTINEL_KEY, sentinelValue);
 }
 
-function getStorageState() {
+// Returns a promise that is resolve when the sentinel key has taken on the
+// sentinel value.  Oddly structured to make sure promises don't let us
+// accidentally side-step the timeout clamping logic.
+function waitForSentinelValue(sentinelValue) {
+  return new Promise(function(resolve) {
+    function checkFunc() {
+      if (localStorage.getItem(SENTINEL_KEY) === sentinelValue) {
+        resolve();
+      } else {
+        // I believe linters will only yell at us if we use a non-zero constant.
+        // Other forms of back-off were considered, including attempting to
+        // issue a round-trip through PBackground, but that still potentially
+        // runs afoul of labeling while also making us dependent on unrelated
+        // APIs.
+        setTimeout(checkFunc, 0);
+      }
+    }
+    checkFunc();
+  });
+}
+
+async function getStorageState(maybeSentinel) {
+  if (maybeSentinel) {
+    await waitForSentinelValue(maybeSentinel);
+  }
+
   let numKeys = localStorage.length;
   let state = {};
   for (var iKey = 0; iKey < numKeys; iKey++) {
     let key = localStorage.key(iKey);
-    state[key] = localStorage.getItem(key);
+    if (key !== SENTINEL_KEY) {
+      state[key] = localStorage.getItem(key);
+    }
   }
   return state;
 }
 
 function returnAndClearStorageEvents() {
-  let loggedEvents = recordedEvents;
-  recordedEvents = [];
-  return loggedEvents;
+  return storageEventsPromise;
 }
 </script>
 </head>
 <body><h2 id="pageNameH"></h2></body>
 </html>