Session restore: Do not preserve PageStates that cannot be decoded.

If a session with non-decodable PageState is restored, ensure we use a
non-shared main-frame FrameNavigationEntry with a valid but mostly
empty PageState, so that Blink does not later fail to restore the
history item.

Bug: 1196330, 1412401
Change-Id: Ia4729e8f8946f7ae3f0ad9e0b94482b73b63fef3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4246812
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106018}

chrome/browser/sessions/session_restore_browsertest.cc

@@ -970,6 +970,58 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest, RestoreForeignSession) {
   ASSERT_FALSE(entry->GetIsOverridingUserAgent());
 }
 
+// Test that a session can be restored even if the PageState were corrupted.
+// This test covers a GetPageState call that used to fail a DCHECK when the
+// PageState failed to decode during the command building step of session
+// restore, per https://crbug.com/1412401. See also https://crbug.com/1196330,
+// where release builds would crash in the renderer process during restore.
+IN_PROC_BROWSER_TEST_F(SessionRestoreTest, RestoreInvalidPageState) {
+  Profile* profile = browser()->profile();
+
+  // Set up restore data with one tab and one navigation.
+  std::vector<const sessions::SessionWindow*> session;
+  sessions::SessionWindow window;
+  {
+    auto tab1 = std::make_unique<sessions::SessionTab>();
+    tab1->tab_visual_index = 0;
+    tab1->current_navigation_index = 0;
+    tab1->pinned = true;
+    tab1->navigations.push_back(
+        ContentTestHelper::CreateNavigation(GetUrl1().spec(), "one"));
+
+    // Corrupt the PageState for the SerializedNavigationEntry.
+    tab1->navigations.at(0).set_encoded_page_state("garbage");
+
+    window.tabs.push_back(std::move(tab1));
+  }
+
+  // Restore the session in a new window.
+  session.push_back(static_cast<const sessions::SessionWindow*>(&window));
+  std::vector<Browser*> browsers = SessionRestore::RestoreForeignSessionWindows(
+      profile, session.begin(), session.end());
+  ASSERT_EQ(1u, browsers.size());
+  Browser* new_browser = browsers[0];
+  ASSERT_TRUE(new_browser);
+  WaitForTabsToLoad(new_browser);
+  EXPECT_NE(new_browser, browser());
+  EXPECT_EQ(new_browser->profile(), browser()->profile());
+  ASSERT_EQ(2u, active_browser_list_->size());
+  ASSERT_EQ(1, new_browser->tab_strip_model()->count());
+
+  // The URL should still successfully commit, even though the rest of the
+  // previous PageState was lost.
+  content::WebContents* web_contents_1 =
+      new_browser->tab_strip_model()->GetWebContentsAt(0);
+  EXPECT_EQ(GetUrl1(), web_contents_1->GetLastCommittedURL());
+  EXPECT_EQ(GetUrl1(),
+            web_contents_1->GetPrimaryMainFrame()->GetLastCommittedURL());
+
+  content::NavigationEntry* entry =
+      web_contents_1->GetController().GetLastCommittedEntry();
+  ASSERT_TRUE(entry);
+  EXPECT_EQ(GetUrl1(), entry->GetURL());
+}
+
 IN_PROC_BROWSER_TEST_F(SessionRestoreTest, Basic) {
   ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GetUrl1()));
   ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GetUrl2()));

components/sessions/content/content_serialized_navigation_builder.cc

@@ -96,13 +96,20 @@ ContentSerializedNavigationBuilder::ToNavigationEntry(
   DCHECK(browser_context);
   DCHECK(restore_context);
 
-  // The initial values of the NavigationEntry are only temporary - they
-  // will get cloberred by one of the SetPageState calls below.
+  // The initial values of the NavigationEntry are usually temporary - they will
+  // normally get clobbered by one of the SetPageState calls below.
   //
   // This means that things like |navigation->referrer_url| are ignored
   // in favor of using the data stored in |navigation->encoded_page_state|.
-  GURL temporary_url;
-  content::Referrer temporary_referrer;
+  //
+  // We still use the URL and referrer from |navigation| here for the temporary
+  // values, though, in case the PageState fails to decode and clobber them.
+  // This allows us to load the original URL and referrer even if other state is
+  // lost.
+  GURL temporary_url = navigation->virtual_url_;
+  content::Referrer temporary_referrer(
+      navigation->referrer_url(),
+      content::Referrer::ConvertToPolicy(navigation->referrer_policy()));
   absl::optional<url::Origin> temporary_initiator_origin;
   absl::optional<GURL> temporary_initiator_base_url;
 
@@ -139,14 +146,12 @@ ContentSerializedNavigationBuilder::ToNavigationEntry(
     // PageState set above + drop the SetReferrer call below.  This will
     // slightly change the legacy behavior, but will make PageState and
     // Referrer consistent.
-    content::Referrer referrer(
-        navigation->referrer_url(),
-        content::Referrer::ConvertToPolicy(navigation->referrer_policy()));
-    entry->SetReferrer(referrer);
+    entry->SetReferrer(temporary_referrer);
   } else {
     // Note that PageState covers some of the values inside |navigation| (e.g.
     // URL, Referrer).  Calling SetPageState will clobber these values in
-    // content::NavigationEntry (and FrameNavigationEntry(s) below).
+    // content::NavigationEntry (and FrameNavigationEntry(s) below), as long as
+    // the PageState successfully decodes.
     entry->SetPageState(blink::PageState::CreateFromEncodedData(
                             navigation->encoded_page_state_),
                         restore_context);

content/browser/renderer_host/navigation_controller_impl_browsertest.cc

@@ -11677,6 +11677,88 @@ IN_PROC_BROWSER_TEST_P(NavigationControllerBrowserTest,
   EXPECT_EQ(root3_fne, controller3.GetEntryAtIndex(1)->GetFrameEntry(root3));
 }
 
+// Test that restoring a corrupt PageState can still successfully load the
+// original URL without crashing, even if the rest of the PageState is lost.
+// See https://crbug.com/1412401 and https://crbug.com/1196330, as well as
+// SessionRestoreTest.RestoreInvalidPageState.
+IN_PROC_BROWSER_TEST_P(NavigationControllerBrowserTest,
+                       RestoreSessionWithInvalidPageState) {
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetPrimaryFrameTree()
+                            .root();
+  NavigationControllerImpl& controller = static_cast<NavigationControllerImpl&>(
+      shell()->web_contents()->GetController());
+
+  // 1. Navigate to a page with an iframe.
+  GURL url1(embedded_test_server()->GetURL(
+      "a.com", "/navigation_controller/page_with_iframe_simple.html"));
+  GURL original_frame_url(embedded_test_server()->GetURL(
+      "a.com", "/navigation_controller/simple_page_1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), url1));
+  NavigationEntryImpl* entry1 = controller.GetLastCommittedEntry();
+  FrameTreeNode* child = root->child_at(0);
+  ASSERT_EQ(original_frame_url, entry1->GetFrameEntry(child)->url());
+
+  // 2. Navigate the subframe.
+  GURL frame_url(embedded_test_server()->GetURL(
+      "b.com", "/navigation_controller/simple_page_2.html"));
+  {
+    FrameNavigateParamsCapturer capturer(child);
+    ASSERT_TRUE(NavigateFrameToURL(child, frame_url));
+    capturer.Wait();
+  }
+  NavigationEntryImpl* entry2 = controller.GetLastCommittedEntry();
+
+  // Simulate a session restore using a corrupted PageState.  The other values
+  // from the entry are preserved (simulated by cloning the entry).
+  blink::PageState garbage = blink::PageState::CreateFromEncodedData("garbage");
+  blink::ExplodedPageState exploded_state;
+  ASSERT_FALSE(
+      blink::DecodePageState(garbage.ToEncodedData(), &exploded_state));
+  std::unique_ptr<NavigationEntryRestoreContextImpl> context1 =
+      std::make_unique<NavigationEntryRestoreContextImpl>();
+  std::unique_ptr<NavigationEntryImpl> restored_entry1 = entry1->Clone();
+  restored_entry1->SetPageState(garbage, context1.get());
+  std::unique_ptr<NavigationEntryImpl> restored_entry2 = entry2->Clone();
+  restored_entry2->SetPageState(garbage, context1.get());
+
+  // 3. Actually restore the entries in a new tab.
+  std::vector<std::unique_ptr<NavigationEntry>> restored_entries;
+  restored_entries.push_back(std::move(restored_entry1));
+  restored_entries.push_back(std::move(restored_entry2));
+  Shell* shell2 = Shell::CreateNewWindow(
+      controller.GetBrowserContext(), GURL::EmptyGURL(), nullptr, gfx::Size());
+  WebContentsImpl* web_contents2 =
+      static_cast<WebContentsImpl*>(shell2->web_contents());
+  FrameTreeNode* root2 = static_cast<WebContentsImpl*>(shell2->web_contents())
+                             ->GetPrimaryFrameTree()
+                             .root();
+  NavigationControllerImpl& controller2 =
+      static_cast<NavigationControllerImpl&>(web_contents2->GetController());
+  controller2.Restore(restored_entries.size() - 1, RestoreType::kRestored,
+                      &restored_entries);
+  {
+    TestNavigationObserver restore_observer(shell2->web_contents());
+    controller2.LoadIfNecessary();
+    restore_observer.Wait();
+  }
+  NavigationEntryImpl* new_entry1 = controller2.GetEntryAtIndex(0);
+  NavigationEntryImpl* new_entry2 = controller2.GetEntryAtIndex(1);
+  EXPECT_EQ(new_entry2, controller2.GetLastCommittedEntry());
+
+  // Verify that the subframe loads its original URL, because frame_url was
+  // lost in the corrupted PageState.
+  FrameTreeNode* child2 = root2->child_at(0);
+  FrameNavigationEntry* child2_fne = new_entry2->GetFrameEntry(child2);
+  EXPECT_EQ(original_frame_url, child2_fne->url());
+
+  // Ensure that the main frame entries do not share a FrameNavigationEntry,
+  // despite ending up with identical item sequence numbers (i.e., 0) in the
+  // corrupted PageState. This works because ISNs of 0 are exempt from the
+  // de-deduplication logic.
+  EXPECT_NE(new_entry1->GetFrameEntry(root2), new_entry2->GetFrameEntry(root2));
+}
+
 // Tests the value of history.state after same-document replacement, in all
 // affected entries.
 IN_PROC_BROWSER_TEST_P(NavigationControllerBrowserTest,

content/browser/renderer_host/navigation_entry_impl.cc

@@ -523,11 +523,17 @@ void NavigationEntryImpl::SetPageState(const blink::PageState& state,
   if (!frame_tree_->children.empty())
     frame_tree_->children.clear();
 
-  // If the PageState can't be parsed, just store it on the main frame's
-  // FrameNavigationEntry without recursively creating subframe entries.
+  // If the PageState can't be parsed, store a clean PageState for the URL
+  // without recursively creating subframe entries. This ensures that the
+  // renderer and future sessions will be able to handle the history item, even
+  // if not all data can be preserved. See https://crbug.com/1196330.
   blink::ExplodedPageState exploded_state;
   if (!blink::DecodePageState(state.ToEncodedData(), &exploded_state)) {
-    frame_tree_->frame_entry->SetPageState(state);
+    // Replace frame_entry with a clone to avoid sharing with any other
+    // NavigationEntries, because the item sequence number will be gone.
+    frame_tree_->frame_entry = frame_tree_->frame_entry->Clone();
+    frame_tree_->frame_entry->SetPageState(
+        blink::PageState::CreateFromURL(GetURL()));
     return;
   }