Bug 1756995 - Optimize docshell load end session store collection. r=peterv

Differential Revision: https://phabricator.services.mozilla.com/D146207
2022-06-15 11:15:35 +00:00
parent 6687e2c9f8
commit c1d0c7e28e
10 changed files with 199 additions and 111 deletions
--- 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,8 +107,10 @@ 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,66 +33,62 @@ addNonCoopTask(

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

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

-      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,
+    "input[name='1|#out2']",
+    "value",
+    Math.random()
+  );

-          let event = aTextField.ownerDocument.createEvent("UIEvents");
-          event.initUIEvent("input", true, true, aTextField.ownerGlobal, 0);
-          aTextField.dispatchEvent(event);
-        }
-        typeText2(content.document.getElementById("in1"), new Date());
-      });
-    });
-  });
+  await setPropertyOfFormField(
+    tab.linkedBrowser.browsingContext.children[0].children[1],
+    "#in1",
+    "value",
+    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.
    /*
@@ -107,19 +103,17 @@ 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
@@ -5796,8 +5796,8 @@ nsDocShell::OnStateChange(nsIWebProgress* aProgress, nsIRequest* aRequest,
          if (WindowContext* windowContext =
                  mBrowsingContext->GetCurrentWindowContext()) {
            SessionStoreChild::From(windowContext->GetWindowGlobalChild())
-                ->SendResetSessionStore(
-                    mBrowsingContext, mBrowsingContext->GetSessionStoreEpoch());
+                ->ResetSessionStore(mBrowsingContext,
+                                    mBrowsingContext->GetSessionStoreEpoch());
          }
        }
      }
@@ -6538,11 +6538,14 @@ nsresult nsDocShell::EndPageLoad(nsIWebProgress* aProgress,
  if constexpr (SessionStoreUtils::NATIVE_LISTENER) {
    if (WindowContext* windowContext =
            mBrowsingContext->GetCurrentWindowContext()) {
-      // 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);
+      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));
    }
  }

--- a/toolkit/components/sessionstore/BrowserSessionStore.cpp
+++ b/toolkit/components/sessionstore/BrowserSessionStore.cpp
@@ -230,6 +230,8 @@ 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,16 +116,12 @@ SessionStoreChangeListener::HandleEvent(dom::Event* aEvent) {
  nsAutoString eventType;
  aEvent->GetType(eventType);

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

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

@@ -191,12 +187,26 @@ 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;
@@ -215,15 +225,21 @@ void SessionStoreChangeListener::FlushSessionStore() {
      maybeScroll = Some(presShell->GetVisualViewportOffset());
    }

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

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

 /* static */
@@ -247,28 +263,17 @@ 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;
 }

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

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

  void FlushSessionStore();

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

  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; }

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

 private:
  void RecordChange(WindowContext* aWindowContext, EnumSet<Change> aChanges);
@@ -98,8 +96,6 @@ 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.
-    Unused << SendSessionStoreUpdate(Nothing(), Nothing(), false, 0);
+    SessionStoreUpdate(Nothing(), Nothing(), aSessionHistoryUpdate, 0);
    return;
  }

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

-  Unused << SendSessionStoreUpdate(
+  SessionStoreUpdate(
      docShellCaps, privatedMode,
      store->GetAndClearSHistoryChanged() || aSessionHistoryUpdate,
      mSessionStoreListener->GetEpoch());
@@ -207,8 +207,6 @@ void SessionStoreChild::UpdateSHistoryChanges() {
 mozilla::ipc::IPCResult SessionStoreChild::RecvFlushTabState(
    FlushTabStateResolver&& aResolver) {
  if (mSessionStoreChangeListener) {
-    mSessionStoreChangeListener->CollectWireframe();
-
    mSessionStoreChangeListener->FlushSessionStore();
  }
  aResolver(true);
@@ -216,6 +214,46 @@ 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,6 +35,19 @@ 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 SessionStoreUpdate(CanonicalBrowsingContext* aBrowsingContext,
-                               const Maybe<nsCString>& aDocShellCaps,
-                               const Maybe<bool>& aPrivatedMode,
-                               bool aNeedCollectSHistory, uint32_t aEpoch) {
+static void DoSessionStoreUpdate(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();
  }

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

@@ -189,6 +189,26 @@ 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,6 +44,21 @@ 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;