IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ReloadToCrossSitePage) {
  auto response1 = std::make_unique<net::test_server::ControllableHttpResponse>(
              embedded_test_server(), "/title1.html");
  auto response2 = std::make_unique<net::test_server::ControllableHttpResponse>(
              embedded_test_server(), "/title1.html");
  StartEmbeddedServer();
  NavigationControllerImpl& nav_controller =
      static_cast<NavigationControllerImpl&>(
          shell()->web_contents()->GetController());

  // First navigation won't redirect and will just end up at
  // http://foo.com/title1.html.
  {
    // Navigate...
    GURL start_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
    TestNavigationObserver nav_observer(shell()->web_contents());
    shell()->LoadURL(start_url);
    response1->WaitForRequest();
    response1->Send(net::HTTP_OK, "text/html; charset=utf-8", "<p>Blah</p>");
    response1->Done();
    nav_observer.Wait();

    // Verify...
    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
    EXPECT_EQ(1, nav_controller.GetEntryCount());
    EXPECT_EQ(0, nav_controller.GetLastCommittedEntryIndex());
    EXPECT_EQ(start_url, nav_controller.GetLastCommittedEntry()->GetURL());
  }

  // Second navigation is a reload that will redirect to
  // http://bar.com/title2.html.
  {
    // Navigate...
    GURL redirect_url(embedded_test_server()->GetURL("bar.com", "/title2.html"));
    TestNavigationObserver reload_observer(shell()->web_contents());
    shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
    response2->WaitForRequest();
    response2->Send("HTTP/1.1 302 Moved Temporarily\n");
    response2->Send("Location: " + redirect_url.spec());
    response2->Done();
    reload_observer.Wait();

    // The successful reload should be classified as a NEW_PAGE navigation
    // with replacement, since it needs to stay at the same entry in session
    // history, but needs a new entry because of the change in SiteInstance.
    EXPECT_TRUE(reload_observer.last_navigation_succeeded());
    EXPECT_EQ(NavigationType::NAVIGATION_TYPE_NEW_PAGE,  // NAVIGATION_TYPE_EXISTING_PAGE
              reload_observer.last_navigation_type());   // with OOPIFs disabled.
    EXPECT_EQ(2, nav_controller.GetEntryCount());  // 1 with OOPIFs disabled.
    EXPECT_EQ(1, nav_controller.GetLastCommittedEntryIndex());  // 0 with OOPIFs disabled.
    EXPECT_EQ(redirect_url, nav_controller.GetLastCommittedEntry()->GetURL());
  }
}

This bug would have been caught by NavigationControllerTest.Reload_GeneratesNewPage unit test *if* the test would be simulating the commit in the right RenderFrameHost (as-is, the test never swaps the RFH during a cross-site navigation):

+  TestRenderFrameHost* GetNavigatingRenderFrameHost() {
+    return AreAllSitesIsolatedForTesting() ? contents()->GetPendingMainFrame()
+                                           : contents()->GetMainFrame();
+  }
+
...
-  main_test_rfh()->SendNavigate(entry_id, true, url2);
+  navigating_rfh->SendNavigate(entry_id, true, url2);
+

A similar unit test (NavigationControllerTest.Forward_GeneratesNewPage) might indicate that this bug also affects other history navigations (if the site the history entry commits in changes).

Actually, I must have misread the test results yesterday.  It seems that the history length is the same with and without OOPIFs - the only difference is the type of the entry after the reload:

    EXPECT_TRUE(reload_observer.last_navigation_succeeded());
    EXPECT_EQ(1, nav_controller.GetEntryCount());
    EXPECT_EQ(0, nav_controller.GetLastCommittedEntryIndex());
    EXPECT_EQ(redirect_url, nav_controller.GetLastCommittedEntry()->GetURL());
    if (AreAllSitesIsolatedForTesting()) {
      EXPECT_EQ(NavigationType::NAVIGATION_TYPE_NEW_PAGE,
                reload_observer.last_navigation_type());
    } else {
      EXPECT_EQ(NavigationType::NAVIGATION_TYPE_EXISTING_PAGE,
                reload_observer.last_navigation_type());
    }

I probably should take a longer, more careful look at the failures in
NavigationControllerTest.Forward_GeneratesNewPage and
NavigationControllerTest.Reload_GeneratesNewPage unit tests (which don't seem to reproduce in browser tests and therefore may be an artifact of how unit tests simulate navigations, rather than a product code bug).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86e3a9f6efedf945f86250ab9188f8c2ea9a300e

commit 86e3a9f6efedf945f86250ab9188f8c2ea9a300e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Dec 21 00:25:18 2018

Tweak navigation commit unit tests, to more closely match product code.

The following tweaks were made:

- Use NavigationSimulator where possible (instead of manually calling
  individual functions of TestRenderFrameHost).

- Target the correct RenderFrameHost (which might change during
  a navigation that requires a process swap).  This is achieved
  by introducing the GetNavigatingRenderFrameHost helper method.

- Fix several tests that used a different url 1) when initiating the
  navigation and 2) when committing the navigation.

- Change interstitial tests so that the target page doesn't change
  while the interstitial is shown.

- Make sure that Forward_GeneratesNewPage and Reload_GeneratesNewPage
  unit tests pass the correct value of |did_create_new_entry| (which is
  |false| in browser tests / in production).  This changes the
  expectations of Reload_GeneratesNewPage to match the production
  (reload should replace the history entry, instead of creating a new
  entry) and Forward_GeneratesNewPage (no pruning needs to take place).
  This change is supported by introducing a new browser test that
  replicates the scenario of Reload_GeneratesNewPage.

Cleaning up unit test will help proceed with enforcement of CanCommit
origin checks (which might have otherwise introduce "renderer kills"
during the tests).

Bug:  770239 ,  916783 
Change-Id: I0af7e1049376a0d0834d48f4de05fb0fe38d56df
Reviewed-on: https://chromium-review.googlesource.com/c/1387196
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618400}
[modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/public/test/navigation_simulator.cc

Because of #c3, I think it is appropriate to resolve this bug as WontFix:
- The new test landed in #c4
- The difference between NAVIGATION_TYPE_EXISTING_PAGE and NAVIGATION_TYPE_NEW_PAGE is minor (and if we think this difference is important, then we probably should track it in a separate bug).