1709208 - SessionStore does not restore scroll position after the tab crashes and is then restored from history

Status: NEW

(Core :: Session Restore, defect, P3)

Description

[Anny Gakhokidze [:annyG]]

While coming up with a test case for bug 1703326, I noticed that the scroll is not restored in the following scenario:

• Navigate to a remote page, scroll, crash the tab.
• Try to set the scroll to some value, verify that we can't update SessionStore when the tab is crashed, and that the scroll is still the same as it was before crashing.
• Restore the tab from history, and verify that the original scroll is restored (this fails, tabState.scroll?.scroll is undefined).
• Try to set the scroll to something else and check that it gets updated to a new value (it does not, tabState.scroll?.scroll is undefined).

Logging of data here shows that we do have the correct original scroll value, but we fail to restore it properly.

This is the test case:

const PRE_CRASH_SCROLL = "1,0";
const POST_CRASH_SCROLL = "2,0";
const POST_REVIVE_SCROLL = "3,0";

const URL = "https://example.com";

function frameScript(scroll) {
  let mm = docShell.messageManager;
  let data = {
    windowstatechange: {
      path: [],
      hasChildren: false,
      scroll: { scroll },
    },
  };
  mm.sendSyncMessage("SessionStore:update", { epoch: 0, data });
}

async function updateScroll(browser, scroll) {
  await ContentTask.spawn(browser, scroll, frameScript);
  await TabStateFlusher.flush(browser);
}

async function checkScroll(tab, scroll, msg) {
  let tabState = JSON.parse(ss.getTabState(tab));
  is(tabState.scroll?.scroll, scroll, msg);
}

function clickButton(browser, id) {
  info("Clicking " + id);
  return SpecialPowers.spawn(browser, [id], buttonId => {
    let button = content.document.getElementById(buttonId);
    button.click();
  });
}

add_task(async function test_update_crashed_tab_after_session_restore() {
  let tab = BrowserTestUtils.addTab(gBrowser, URL);
  let browser = tab.linkedBrowser;
  gBrowser.selectedTab = tab;
  await promiseBrowserLoaded(browser);

  // The initial load of a new document results in an empty "windowstatechange"
  // update, so we have to first flush, then send our manual update, and then
  // flush again.
  await TabStateFlusher.flush(browser);
  await updateScroll(browser, PRE_CRASH_SCROLL);
  await checkScroll(tab, PRE_CRASH_SCROLL, "post-crash value is correct");

  await BrowserTestUtils.crashFrame(browser);

  await updateScroll(browser, POST_CRASH_SCROLL);
  // check that we cant update session store when the browser is crashed
  await checkScroll(tab, PRE_CRASH_SCROLL, "post-crash value is correct, did not update");

  // Use SessionStore to revive the tab
  let tabRestoredPromise = promiseTabRestored(tab);
  await clickButton(browser, "restoreTab");
  await tabRestoredPromise;
  await TabStateFlusher.flush(tab.linkedBrowser);
  await checkScroll(tab, PRE_CRASH_SCROLL, "post-crash value is correct, restored correctly");

  ok(
    !tab.hasAttribute("crashed"),
    "Tab shouldn't be marked as crashed anymore."
  );


  // The initial load of a new document results in an empty "windowstatechange"
  // update, so we have to first flush, then send our manual update, and then
  // flush again.
  await TabStateFlusher.flush(tab.linkedBrowser);
  await updateScroll(tab.linkedBrowser, POST_REVIVE_SCROLL);
  await checkScroll(tab, POST_REVIVE_SCROLL, "post-revive value is correct");

  gBrowser.removeTab(tab);
});

Comment 1

[u608768]

Might be worth checking if bug 1706445 fixes this (specifically D116145).

Comment 2

[u608768]

Might be fixed by bug 1716368.

Comment 3

[Anny Gakhokidze [:annyG]]

Still incorrect, the error is FAIL post-crash value is correct, restored correctly - Got undefined, expected "1,0"

in checkScroll is(tabState.scroll?.scroll, scroll, msg);
which gets called here await checkScroll(tab, PRE_CRASH_SCROLL, "post-crash value is correct, restored correctly");

Comment 4

[Chris Peterson [:cpeterson]]

Kashav, do you mind if we postpone this bug until after Fission MVP? If a user's tab just crashed, losing their scroll position is probably not their top concern. :)

Comment 5

[u608768]

Yes, that should be fine, and IIRC, this doesn't even reproduce outside of the test.