Backed out changeset 6ad49f7d560a (bug 1756995) for causing mochitest failures on browser_firefoxview_tab.js CLOSED TREE

2022-05-25 14:27:30 -04:00 · 2022-05-25 14:27:30 -04:00 · 9773ee73d1
--- a/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js
+++ b/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js
@ -107,10 +107,8 @@ add_task(async function test_sessions_get_recently_closed_tabs() {

  await extension.startup();

-  let sessionUpdatePromise = BrowserTestUtils.waitForSessionStoreUpdate(tab);
  // Test with a closed tab.
  BrowserTestUtils.removeTab(tab);
-  await sessionUpdatePromise;

  extension.sendMessage("check-sessions");
  let recentlyClosed = await extension.awaitMessage("recentlyClosed");
--- a/browser/components/sessionstore/test/browser_463206.js
+++ b/browser/components/sessionstore/test/browser_463206.js
@ -33,62 +33,66 @@ addNonCoopTask(

 async function test_restore_text_data_subframes(aURL) {
  // Add a new tab.
-  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, aURL);
+  let tab = BrowserTestUtils.addTab(gBrowser, aURL);
+  await BrowserTestUtils.browserLoaded(tab.linkedBrowser);

-  await setPropertyOfFormField(
-    tab.linkedBrowser,
-    "#out1",
-    "value",
-    Date.now().toString(16)
-  );
+  // "Type in" some random values.
+  await SpecialPowers.spawn(tab.linkedBrowser, [], async function() {
+    function typeText(aTextField, aValue) {
+      aTextField.value = aValue;

-  await setPropertyOfFormField(
-    tab.linkedBrowser,
-    "input[name='1|#out2']",
-    "value",
-    Math.random()
-  );
+      let event = aTextField.ownerDocument.createEvent("UIEvents");
+      event.initUIEvent("input", true, true, aTextField.ownerGlobal, 0);
+      aTextField.dispatchEvent(event);
+    }
+    typeText(content.document.getElementById("out1"), Date.now().toString(16));
+    typeText(content.document.getElementsByName("1|#out2")[0], Math.random());
+    await SpecialPowers.spawn(content.frames[0], [], async function() {
+      await SpecialPowers.spawn(content.frames[1], [], async function() {
+        function typeText2(aTextField, aValue) {
+          aTextField.value = aValue;

-  await setPropertyOfFormField(
-    tab.linkedBrowser.browsingContext.children[0].children[1],
-    "#in1",
-    "value",
-    new Date()
-  );
+          let event = aTextField.ownerDocument.createEvent("UIEvents");
+          event.initUIEvent("input", true, true, aTextField.ownerGlobal, 0);
+          aTextField.dispatchEvent(event);
+        }
+        typeText2(content.document.getElementById("in1"), new Date());
+      });
+    });
+  });

  // Duplicate the tab.
  let tab2 = gBrowser.duplicateTab(tab);
-  let browser2 = tab2.linkedBrowser;
  await promiseTabRestored(tab2);

-  isnot(
-    await getPropertyOfFormField(browser2, "#out1", "value"),
-    await getPropertyOfFormField(
-      browser2.browsingContext.children[1],
-      "#out1",
-      "value"
-    ),
-    "text isn't reused for frames"
-  );
-
-  isnot(
-    await getPropertyOfFormField(browser2, "input[name='1|#out2']", "value"),
-    "",
-    "text containing | and # is correctly restored"
-  );
-
-  is(
-    await getPropertyOfFormField(
-      browser2.browsingContext.children[1],
-      "#out2",
-      "value"
-    ),
-    "",
-    "id prefixes can't be faked"
-  );
-
  // Query a few values from the top and its child frames.
  await SpecialPowers.spawn(tab2.linkedBrowser, [], async function() {
+    let out1Val = await SpecialPowers.spawn(
+      content.frames[1],
+      [],
+      async function() {
+        return content.document.getElementById("out1").value;
+      }
+    );
+    Assert.notEqual(
+      content.document.getElementById("out1").value,
+      out1Val,
+      "text isn't reused for frames"
+    );
+    Assert.notEqual(
+      content.document.getElementsByName("1|#out2")[0].value,
+      "",
+      "text containing | and # is correctly restored"
+    );
+    let out2Val = await SpecialPowers.spawn(
+      content.frames[1],
+      [],
+      async function() {
+        return content.document.getElementById("out2").value;
+      }
+    );
+    Assert.equal(out2Val, "", "id prefixes can't be faked");
+
    // Bug 588077
    // XXX(farre): disabling this, because it started passing more heavily on Windows.
    /*
@ -103,17 +107,19 @@ async function test_restore_text_data_subframes(aURL) {
    );
    todo_is(in1ValFrame0_1, "", "id prefixes aren't mixed up");
    */
+
+    let in1ValFrame1_0 = await SpecialPowers.spawn(
+      content.frames[1],
+      [],
+      async function() {
+        return SpecialPowers.spawn(content.frames[0], [], async function() {
+          return content.document.getElementById("in1").value;
+        });
+      }
+    );
+    Assert.equal(in1ValFrame1_0, "", "id prefixes aren't mixed up");
  });

-  is(
-    await getPropertyOfFormField(
-      browser2.browsingContext.children[1].children[0],
-      "#in1",
-      "value"
-    ),
-    "",
-    "id prefixes aren't mixed up"
-  );
  // Cleanup.
  gBrowser.removeTab(tab2);
  gBrowser.removeTab(tab);
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@ -5822,8 +5822,8 @@ nsDocShell::OnStateChange(nsIWebProgress* aProgress, nsIRequest* aRequest,
          if (WindowContext* windowContext =
                  mBrowsingContext->GetCurrentWindowContext()) {
            SessionStoreChild::From(windowContext->GetWindowGlobalChild())
-                ->ResetSessionStore(mBrowsingContext,
-                                    mBrowsingContext->GetSessionStoreEpoch());
+                ->SendResetSessionStore(
+                    mBrowsingContext, mBrowsingContext->GetSessionStoreEpoch());
          }
        }
      }
@ -6564,14 +6564,11 @@ nsresult nsDocShell::EndPageLoad(nsIWebProgress* aProgress,
  if constexpr (SessionStoreUtils::NATIVE_LISTENER) {
    if (WindowContext* windowContext =
            mBrowsingContext->GetCurrentWindowContext()) {
-      using Change = SessionStoreChangeListener::Change;
-
-      // We've finished loading the page and now we want to collect all the
-      // session store state that the page is initialized with.
-      SessionStoreChangeListener::CollectSessionStoreData(
-          windowContext,
-          EnumSet<Change>(Change::Input, Change::Scroll, Change::SessionHistory,
-                          Change::WireFrame));
+      // TODO(farre): File bug: From a user perspective this would probably be
+      // just fine to run off the change listener timer. Turns out that a flush
+      // is needed. Several tests depend on this behaviour. Could potentially be
+      // an optimization for later. See Bug 1756995.
+      SessionStoreChangeListener::FlushAllSessionStoreData(windowContext);
    }
  }

--- a/toolkit/components/sessionstore/BrowserSessionStore.cpp
+++ b/toolkit/components/sessionstore/BrowserSessionStore.cpp
@ -230,8 +230,6 @@ void UpdateSessionStoreField(CanonicalBrowsingContext* aBrowsingContext,
    } else {
      currentEntry = (aBrowsingContext->Top()->*GetWeakRef)().get();
    }
-  } else {
-    currentEntry = (aBrowsingContext->Top()->*GetWeakRef)().get();
  }

  *aEntry = currentEntry.forget().take();
--- a/toolkit/components/sessionstore/SessionStoreChangeListener.cpp
+++ b/toolkit/components/sessionstore/SessionStoreChangeListener.cpp
@ -116,12 +116,16 @@ SessionStoreChangeListener::HandleEvent(dom::Event* aEvent) {
  nsAutoString eventType;
  aEvent->GetType(eventType);

+  Change change = Change::None;
+
  if (eventType == kInput) {
-    RecordChange(windowContext, Change::Input);
+    change = Change::Input;
  } else if (eventType == kScroll) {
-    RecordChange(windowContext, Change::Scroll);
+    change = Change::Scroll;
  }

+  RecordChange(windowContext, EnumSet(change));
+
  return NS_OK;
 }

@ -187,26 +191,12 @@ void SessionStoreChangeListener::FlushSessionStore() {
    mTimer = nullptr;
  }

-  bool collectSessionHistory = false;
-  bool collectWireFrame = false;
-
  for (auto& iter : mSessionStoreChanges) {
    WindowContext* windowContext = iter.GetKey();
    if (!windowContext) {
      continue;
    }

-    BrowsingContext* browsingContext = windowContext->GetBrowsingContext();
-
-    // This is a bit unfortunate, but is needed when a window context with a
-    // recorded change has become non-current before its data has been
-    // collected. This can happen either due to navigation or destruction, and
-    // in the previous case we don't want to collect, but in the latter we do.
-    // This could be cleaned up if we change. See bug 1770773.
-    if (!windowContext->IsCurrent() && !browsingContext->IsDiscarded()) {
-      continue;
-    }
-
    RefPtr<Document> document = windowContext->GetDocument();
    if (!document) {
      continue;
@ -225,21 +215,15 @@ void SessionStoreChangeListener::FlushSessionStore() {
      maybeScroll = Some(presShell->GetVisualViewportOffset());
    }

-    collectWireFrame = collectWireFrame || changes.contains(Change::WireFrame);
-
-    collectSessionHistory =
-        collectSessionHistory || changes.contains(Change::SessionHistory);
-
-    mSessionStoreChild->IncrementalSessionStoreUpdate(
-        browsingContext, maybeFormData, maybeScroll, mEpoch);
-  }
-
-  if (collectWireFrame) {
-    collectSessionHistory = CollectWireframe() || collectSessionHistory;
+    mSessionStoreChild->SendIncrementalSessionStoreUpdate(
+        windowContext->GetBrowsingContext(), maybeFormData, maybeScroll,
+        mEpoch);
  }

  mSessionStoreChanges.Clear();
-  mSessionStoreChild->UpdateSessionStore(collectSessionHistory);
+
+  mSessionStoreChild->UpdateSessionStore(mCollectSessionHistory);
+  mCollectSessionHistory = false;
 }

 /* static */
@ -263,17 +247,28 @@ SessionStoreChangeListener* SessionStoreChangeListener::CollectSessionStoreData(
  return sessionStoreChangeListener;
 }

+/* static */
+void SessionStoreChangeListener::FlushAllSessionStoreData(
+    WindowContext* aWindowContext) {
+  EnumSet<Change> allChanges(Change::Input, Change::Scroll);
+  SessionStoreChangeListener* listener =
+      CollectSessionStoreData(aWindowContext, allChanges);
+  if (listener) {
+    listener->FlushSessionStore();
+  }
+}
+
 void SessionStoreChangeListener::SetActor(
    SessionStoreChild* aSessionStoreChild) {
  mSessionStoreChild = aSessionStoreChild;
 }

-bool SessionStoreChangeListener::CollectWireframe() {
+void SessionStoreChangeListener::CollectWireframe() {
  if (auto* docShell = nsDocShell::Cast(mBrowsingContext->GetDocShell())) {
-    return docShell->CollectWireframe();
+    if (docShell->CollectWireframe()) {
+      mCollectSessionHistory = true;
+    }
  }
-
-  return false;
 }

 void SessionStoreChangeListener::RecordChange(WindowContext* aWindowContext,
--- a/toolkit/components/sessionstore/SessionStoreChangeListener.h
+++ b/toolkit/components/sessionstore/SessionStoreChangeListener.h
@ -56,18 +56,20 @@ class SessionStoreChangeListener final : public nsINamed,

  void FlushSessionStore();

-  enum class Change { Input, Scroll, SessionHistory, WireFrame };
+  enum class Change { None, Input, Scroll };

  static SessionStoreChangeListener* CollectSessionStoreData(
      WindowContext* aWindowContext, const EnumSet<Change>& aChanges);

+  static void FlushAllSessionStoreData(WindowContext* aWindowContext);
+
  void SetActor(SessionStoreChild* aSessionStoreChild);

  void SetEpoch(uint32_t aEpoch) { mEpoch = aEpoch; }

  uint32_t GetEpoch() const { return mEpoch; }

-  bool CollectWireframe();
+  void CollectWireframe();

 private:
  void RecordChange(WindowContext* aWindowContext, EnumSet<Change> aChanges);
@ -96,6 +98,8 @@ class SessionStoreChangeListener final : public nsINamed,
  nsCOMPtr<nsITimer> mTimer;
  RefPtr<SessionStoreChild> mSessionStoreChild;
  SessionStoreChangeTable mSessionStoreChanges;
+
+  bool mCollectSessionHistory = false;
 };

 }  // namespace mozilla::dom
--- a/toolkit/components/sessionstore/SessionStoreChild.cpp
+++ b/toolkit/components/sessionstore/SessionStoreChild.cpp
@ -170,7 +170,7 @@ void SessionStoreChild::UpdateEventTargets() {
 void SessionStoreChild::UpdateSessionStore(bool aSessionHistoryUpdate) {
  if (!mSessionStoreListener) {
    // This is the case when we're shutting down, and expect a final update.
-    SessionStoreUpdate(Nothing(), Nothing(), aSessionHistoryUpdate, 0);
+    Unused << SendSessionStoreUpdate(Nothing(), Nothing(), false, 0);
    return;
  }

@ -186,7 +186,7 @@ void SessionStoreChild::UpdateSessionStore(bool aSessionHistoryUpdate) {
    privatedMode.emplace(store->GetPrivateModeEnabled());
  }

-  SessionStoreUpdate(
+  Unused << SendSessionStoreUpdate(
      docShellCaps, privatedMode,
      store->GetAndClearSHistoryChanged() || aSessionHistoryUpdate,
      mSessionStoreListener->GetEpoch());
@ -207,6 +207,8 @@ void SessionStoreChild::UpdateSHistoryChanges() {
 mozilla::ipc::IPCResult SessionStoreChild::RecvFlushTabState(
    FlushTabStateResolver&& aResolver) {
  if (mSessionStoreChangeListener) {
+    mSessionStoreChangeListener->CollectWireframe();
+
    mSessionStoreChangeListener->FlushSessionStore();
  }
  aResolver(true);
@ -214,46 +216,6 @@ mozilla::ipc::IPCResult SessionStoreChild::RecvFlushTabState(
  return IPC_OK();
 }

-void SessionStoreChild::SessionStoreUpdate(
-    const Maybe<nsCString>& aDocShellCaps, const Maybe<bool>& aPrivatedMode,
-    const bool aNeedCollectSHistory, const uint32_t& aEpoch) {
-  if (XRE_IsContentProcess()) {
-    Unused << SendSessionStoreUpdate(aDocShellCaps, aPrivatedMode,
-                                     aNeedCollectSHistory, aEpoch);
-  } else if (SessionStoreParent* sessionStoreParent =
-                 static_cast<SessionStoreParent*>(
-                     InProcessChild::ParentActorFor(this))) {
-    sessionStoreParent->SessionStoreUpdate(aDocShellCaps, aPrivatedMode,
-                                           aNeedCollectSHistory, aEpoch);
-  }
-}
-
-void SessionStoreChild::IncrementalSessionStoreUpdate(
-    const MaybeDiscarded<BrowsingContext>& aBrowsingContext,
-    const Maybe<FormData>& aFormData, const Maybe<nsPoint>& aScrollPosition,
-    uint32_t aEpoch) {
-  if (XRE_IsContentProcess()) {
-    Unused << SendIncrementalSessionStoreUpdate(aBrowsingContext, aFormData,
-                                                aScrollPosition, aEpoch);
-  } else if (SessionStoreParent* sessionStoreParent =
-                 static_cast<SessionStoreParent*>(
-                     InProcessChild::ParentActorFor(this))) {
-    sessionStoreParent->IncrementalSessionStoreUpdate(
-        aBrowsingContext, aFormData, aScrollPosition, aEpoch);
-  }
-}
-
-void SessionStoreChild::ResetSessionStore(
-    const MaybeDiscarded<BrowsingContext>& aBrowsingContext, uint32_t aEpoch) {
-  if (XRE_IsContentProcess()) {
-    Unused << SendResetSessionStore(aBrowsingContext, aEpoch);
-  } else if (SessionStoreParent* sessionStoreParent =
-                 static_cast<SessionStoreParent*>(
-                     InProcessChild::ParentActorFor(this))) {
-    sessionStoreParent->ResetSessionStore(aBrowsingContext, aEpoch);
-  }
-}
-
 NS_IMPL_CYCLE_COLLECTION(SessionStoreChild, mSessionStoreListener,
                         mSessionStoreChangeListener)
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(SessionStoreChild, AddRef)
--- a/toolkit/components/sessionstore/SessionStoreChild.h
+++ b/toolkit/components/sessionstore/SessionStoreChild.h
@ -35,19 +35,6 @@ class SessionStoreChild final : public PSessionStoreChild {
  void FlushSessionStore();
  void UpdateSHistoryChanges();

-  void SessionStoreUpdate(const Maybe<nsCString>& aDocShellCaps,
-                          const Maybe<bool>& aPrivatedMode,
-                          const bool aNeedCollectSHistory,
-                          const uint32_t& aEpoch);
-
-  void IncrementalSessionStoreUpdate(
-      const MaybeDiscarded<BrowsingContext>& aBrowsingContext,
-      const Maybe<FormData>& aFormData, const Maybe<nsPoint>& aScrollPosition,
-      uint32_t aEpoch);
-
-  void ResetSessionStore(
-      const MaybeDiscarded<BrowsingContext>& aBrowsingContext, uint32_t aEpoch);
-
  SessionStoreChangeListener* GetSessionStoreChangeListener() const {
    return mSessionStoreChangeListener;
  }
--- a/toolkit/components/sessionstore/SessionStoreParent.cpp
+++ b/toolkit/components/sessionstore/SessionStoreParent.cpp
@ -33,10 +33,10 @@ SessionStoreParent::SessionStoreParent(
    BrowserSessionStore* aSessionStore)
    : mBrowsingContext(aBrowsingContext), mSessionStore(aSessionStore) {}

-static void DoSessionStoreUpdate(CanonicalBrowsingContext* aBrowsingContext,
-                                 const Maybe<nsCString>& aDocShellCaps,
-                                 const Maybe<bool>& aPrivatedMode,
-                                 bool aNeedCollectSHistory, uint32_t aEpoch) {
+static void SessionStoreUpdate(CanonicalBrowsingContext* aBrowsingContext,
+                               const Maybe<nsCString>& aDocShellCaps,
+                               const Maybe<bool>& aPrivatedMode,
+                               bool aNeedCollectSHistory, uint32_t aEpoch) {
  UpdateSessionStoreData data;
  if (aDocShellCaps.isSome()) {
    auto& disallow = data.mDisallow.Construct();
@ -162,8 +162,8 @@ mozilla::ipc::IPCResult SessionStoreParent::RecvSessionStoreUpdate(
    return IPC_OK();
  }

-  DoSessionStoreUpdate(mBrowsingContext, aDocShellCaps, aPrivatedMode,
-                       aNeedCollectSHistory, aEpoch);
+  SessionStoreUpdate(mBrowsingContext, aDocShellCaps, aPrivatedMode,
+                     aNeedCollectSHistory, aEpoch);
  return IPC_OK();
 }

@ -189,26 +189,6 @@ mozilla::ipc::IPCResult SessionStoreParent::RecvResetSessionStore(
  return IPC_OK();
 }

-void SessionStoreParent::SessionStoreUpdate(
-    const Maybe<nsCString>& aDocShellCaps, const Maybe<bool>& aPrivatedMode,
-    const bool aNeedCollectSHistory, const uint32_t& aEpoch) {
-  Unused << RecvSessionStoreUpdate(aDocShellCaps, aPrivatedMode,
-                                   aNeedCollectSHistory, aEpoch);
-}
-
-void SessionStoreParent::IncrementalSessionStoreUpdate(
-    const MaybeDiscarded<BrowsingContext>& aBrowsingContext,
-    const Maybe<FormData>& aFormData, const Maybe<nsPoint>& aScrollPosition,
-    uint32_t aEpoch) {
-  Unused << RecvIncrementalSessionStoreUpdate(aBrowsingContext, aFormData,
-                                              aScrollPosition, aEpoch);
-}
-
-void SessionStoreParent::ResetSessionStore(
-    const MaybeDiscarded<BrowsingContext>& aBrowsingContext, uint32_t aEpoch) {
-  Unused << RecvResetSessionStore(aBrowsingContext, aEpoch);
-}
-
 NS_IMPL_CYCLE_COLLECTION(SessionStoreParent, mBrowsingContext, mSessionStore)
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(SessionStoreParent, AddRef)
 NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(SessionStoreParent, Release)
--- a/toolkit/components/sessionstore/SessionStoreParent.h
+++ b/toolkit/components/sessionstore/SessionStoreParent.h
@ -44,21 +44,6 @@ class SessionStoreParent final : public PSessionStoreParent {
  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(SessionStoreParent)
  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(SessionStoreParent)

- protected:
-  friend class SessionStoreChild;
-  void SessionStoreUpdate(const Maybe<nsCString>& aDocShellCaps,
-                          const Maybe<bool>& aPrivatedMode,
-                          const bool aNeedCollectSHistory,
-                          const uint32_t& aEpoch);
-
-  void IncrementalSessionStoreUpdate(
-      const MaybeDiscarded<BrowsingContext>& aBrowsingContext,
-      const Maybe<FormData>& aFormData, const Maybe<nsPoint>& aScrollPosition,
-      uint32_t aEpoch);
-
-  void ResetSessionStore(
-      const MaybeDiscarded<BrowsingContext>& aBrowsingContext, uint32_t aEpoch);
-
 private:
  ~SessionStoreParent() = default;