This test gets disabled in https://phabricator.services.mozilla.com/D132388

Assigning to Anny (because Nika says Anny is working on this bug).

This fails (with Fission) when browser.tabs.documentchannel.parent-controlled is enabled, so this bug is for fixing this.

With the browser.tabs.documentchannel.parent-controlled pref enabled, test browser_scrollPositions.js fails here (in all three cases).

Below, I'll describe what I have discovered when running this test case.

After we have scrolled down on the very first page, we check that the scroll gets saved correctly on the tab state here and the tab state looks something like this

{
   "history":{
      "entries":[
         {
            "url":"http://example.com/browser/browser/components/sessionstore/test/browser_scrollPositions_sample.html",
            "title":"browser_scrollPositions_sample.html",
            "cacheKey":0,
            "ID":26,
            "docshellUUID":"{595d9c73-a282-4ab0-b0ec-2a4cd8819f86}",
            "resultPrincipalURI":null,
            "hasUserInteraction":false,
            "triggeringPrincipal_base64":"eyIzIjp7fX0=",
            "docIdentifier":26,
            "persist":true
         }
      ],
      "requestedIndex":0,
      "index":1
   },
   "scroll":{
      "scroll":"120,217"
   }
}

Note that the scroll does not get saved in a layout history state on the SH entry, and instead gets saved at a "top" level.

In the past, I've made a few changes to various Session Store mechanisms to fix assertions in this test, and this was one of the changes. In that patch I made sure that we updated layout history state from docshell before we load a new URI as it might be the last opportunity to do so before a process swap. With my parent-initiated load patches in bug 1721217, loads that are initiated in the parent process (e.g. as a result of a call to BrowserTestUtils.addTab or BrowserTestUtils.loadURI within a browser test) will not be bounced to the content process and proceed to directly load in the parent process before the load is redirected to the final content process.

In this test, we first load http://example.com/browser/browser/components/sessionstore/test/browser_scrollPositions_sample.html, and then in the same browser we load http://example.com/browser/browser/components/sessionstore/test/browser_scrollPositions_sample2.html

We start loading browser_scrollPositions_sample.html in the parent process and then we redirect it to a content process. Later, we scroll on the page. Then we trigger the load for browser_scrollPositions_sample2.html in the parent process, and then redirect it to the same content process. The second load happens in a different docshell (because of bfcache, I think, because that test case passes with bfcache-in-parent disabled).
This is similar to what happens in other test cases where the second load happens in a different docshell (the first being an https load, and the second - http).

The issue is the same - the old docshell doesn't have a chance to sync and persist it's layout history state, because unlike cases when the load (regardless of where it was started) was bounced to the content process and started from there, it had a chance to do so here.

We need to figure out where would a good chance to update the layout history state in Session Store.
This is a sequence of events that happens, to happen us to reason about potential places for doing so:

1. In the parent process, we initiate the load of the first URI.
2. The load gets redirected to the final content process, and we load the first URI in a docshell.
3. We scroll through this first page.
4. In the parent process, we initiate the load of the second URI. This load does not get bounced to the content process
5. The second load gets redirected to the final content process, but to a different docshell.
6. We scroll through this second page.
7. We close the window
8. We destroy the docshells.

By this point, we still haven't updated the layout history state. It would be nice if we could identify a spot where we could update the layout history sometime after step 3 and step 8. I'll discuss some potential solutions and list where I am stuck

1. Just after marking the page as being in the bfcache, we tell the docshell to persist and sync its layout history state. I added the following calls

        nsDocShell::Cast(shell)->PersistLayoutHistoryState();
        nsDocShell::Cast(shell)->SynchronizeLayoutHistoryState();

after we freeze the docshell in the content process here. Unfortunately, this alone does not work because we still need to tell the parent process to trigger a session store collection for the scroll positions to be saved in Session Store. I don't have a good solution to this. I tried calling BrowsingContext's FlushSessionStore, but that didn't help. I also experimented with adding this snippet:

        if constexpr (SessionStoreUtils::NATIVE_LISTENER) {
          if (Document* document = nsDocShell::Cast(shell)->GetDocument()) {
            if (WindowGlobalChild* windowChild = document->GetWindowGlobalChild()) {
              RefPtr<SessionStoreDataCollector> collector =
                  SessionStoreDataCollector::CollectSessionStoreData(windowChild);
              printf_stderr("ANNY -- BC::DidSet isInBFCache, docshell in process, calling collector->Flush, Id=%ld\n", Id());
              collector->Flush();
            }
          }
        }

However, no luck still. I haven't finished debugging this, but I'm going to post this comment and NI Andreas anyway, before I spiral down too much.

2. I conceived this idea only after running into a problem with solution 1. Solution #2 can be the following: Just before we know that the page is going into bfcache (somewhere before we call SetIsInBfCache), we tell the docshell to persist and sync its layout history state and then we can call CanonicalBrowsingContext::MaybeScheduleSessionStoreUpdate. Potential problems I see with this - what if the updates and the process of collecting data takes too long? And is the above function the best to call, for flushing out updates to session history entries?

3. Perhaps we could synchronize the layout history state when the docshell starts getting destroyed, here, but I tried this by calling SynchronizeLayoutHistoryState and that didn't fix the issue. I debugged it only a little bit before realizing that it's still a racy solution as we are in the middle of tearing down structures so perhaps its not the best time to sync layout history state.

4. I'm sure there are more solutions???

Andreas, I'm wondering if you have any thoughts/ideas/feedback on this? Sorry for this huge comment, but I hope its helpful

On the priority - this bug does not really need to be fixed before enabling the parent controlled pref - in https://phabricator.services.mozilla.com/D132388 I disabled it when I thought I'd land the patches with the parent controlled pref enabled (this is a patch that I later abandoned).

We need to get back to this soon after bug 1746524 is done.

With the method Peter added in bug 1766131, this test seems to be easy to fix. Anny's investigation did help me understand enough to get to the point where I knew which line of code to add.

Bug 1766131 added a call to this method before the other call
to ReplacedBy, but apparently the other call site comes into play
in some tests, at least when parent controlled navigation is
enabled. This fixes browser_scrollPositions.js with parent controlled
navigation, and is a step towards fixing browser_test_shentry_wireframe.js.

It sounds like the issue happens when you navigate to one page, then
to another in the same site, via BrowserTestUtils. Because these are
initiated from the parent process, then end up getting redirected to
the content process in another docshell. We have to save the layout
history state before doing this, or we lose the scroll information.

https://hg.mozilla.org/mozilla-central/rev/bc029159cd39