Move WebBundleNavigationInfo to FrameNavigationEntry

There is a bug that navigation of OOPIF's remote iframe in Web Bundle
file cause DCHECK failure. The root cause of the bug is that:
  - When NavigationControllerImpl::NavigateFromFrameProxy() is called
    for a remote iframe, this method clones the committed
    NavigationEntryImpl.
  - NavigationRequest's constructor clones the WebBundleNavigationInfo
    of the passed NavigationEntryImpl. This is not correct, because
    WebBundleNavigationInfo should be attached to the frame, not the
    page.

To fix this bug, this CL moves WebBundleNavigationInfo from
NavigationEntryImpl to FrameNavigationEntry.

Bug: 1058721, 1040800
Change-Id: Ia654876cfd08433550d1bdb9b81e0aac402fe61b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091191Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748218}

content/browser/frame_host/frame_navigation_entry.cc

@@ -8,6 +8,7 @@
 
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "content/browser/web_package/web_bundle_navigation_info.h"
 #include "content/common/page_state_serialization.h"
 
 namespace content {
@@ -146,4 +147,14 @@ scoped_refptr<network::ResourceRequestBody> FrameNavigationEntry::GetPostData(
   return exploded_state.top.http_body.request_body;
 }
 
+void FrameNavigationEntry::set_web_bundle_navigation_info(
+    std::unique_ptr<WebBundleNavigationInfo> web_bundle_navigation_info) {
+  web_bundle_navigation_info_ = std::move(web_bundle_navigation_info);
+}
+
+WebBundleNavigationInfo* FrameNavigationEntry::web_bundle_navigation_info()
+    const {
+  return web_bundle_navigation_info_.get();
+}
+
 }  // namespace content

content/browser/frame_host/frame_navigation_entry.h

@@ -20,6 +20,8 @@
 
 namespace content {
 
+class WebBundleNavigationInfo;
+
 // Represents a session history item for a particular frame.  It is matched with
 // corresponding FrameTreeNodes using unique name (or by the root position).
 //
@@ -189,6 +191,10 @@ class CONTENT_EXPORT FrameNavigationEntry
     blob_url_loader_factory_ = std::move(factory);
   }
 
+  void set_web_bundle_navigation_info(
+      std::unique_ptr<WebBundleNavigationInfo> web_bundle_navigation_info);
+  WebBundleNavigationInfo* web_bundle_navigation_info() const;
+
  private:
   friend class base::RefCounted<FrameNavigationEntry>;
   virtual ~FrameNavigationEntry();
@@ -226,6 +232,13 @@ class CONTENT_EXPORT FrameNavigationEntry
   int64_t post_id_;
   scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory_;
 
+  // Keeps the Web Bundles related information when |this| is for a navigation
+  // within a Web Bundle file. Used when WebBundles feature or
+  // WebBundlesFromNetwork feature is enabled or TrustableWebBundleFileUrl
+  // switch is set.
+  // TODO(995177): Support Session/Tab restore.
+  std::unique_ptr<WebBundleNavigationInfo> web_bundle_navigation_info_;
+
   DISALLOW_COPY_AND_ASSIGN(FrameNavigationEntry);
 };
 

content/browser/frame_host/navigation_controller_impl.cc

@@ -1481,11 +1481,6 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
   new_entry->SetOriginalRequestURL(params.original_request_url);
   new_entry->SetIsOverridingUserAgent(params.is_overriding_user_agent);
 
-  if (request->web_bundle_navigation_info()) {
-    new_entry->set_web_bundle_navigation_info(
-        request->web_bundle_navigation_info()->Clone());
-  }
-
   // Update the FrameNavigationEntry for new main frame commits.
   FrameNavigationEntry* frame_entry =
       new_entry->GetFrameEntry(rfh->frame_tree_node());
@@ -1498,6 +1493,10 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
   frame_entry->set_post_id(params.post_id);
   if (!params.url_is_unreachable)
     frame_entry->set_committed_origin(params.origin);
+  if (request->web_bundle_navigation_info()) {
+    frame_entry->set_web_bundle_navigation_info(
+        request->web_bundle_navigation_info()->Clone());
+  }
 
   // history.pushState() is classified as a navigation to a new page, but sets
   // is_same_document to true. In this case, we already have the title and

content/browser/frame_host/navigation_entry_impl.cc

@@ -20,7 +20,6 @@
 #include "build/build_config.h"
 #include "components/url_formatter/url_formatter.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
-#include "content/browser/web_package/web_bundle_navigation_info.h"
 #include "content/common/content_constants_internal.h"
 #include "content/common/navigation_params.h"
 #include "content/common/page_state_serialization.h"
@@ -724,8 +723,6 @@ std::unique_ptr<NavigationEntryImpl> NavigationEntryImpl::CloneAndReplace(
   copy->CloneDataFrom(*this);
   copy->replaced_entry_data_ = replaced_entry_data_;
   copy->should_skip_on_back_forward_ui_ = should_skip_on_back_forward_ui_;
-  if (web_bundle_navigation_info_)
-    copy->web_bundle_navigation_info_ = web_bundle_navigation_info_->Clone();
 
   return copy;
 }
@@ -994,14 +991,4 @@ GURL NavigationEntryImpl::GetHistoryURLForDataURL() {
   return GetBaseURLForDataURL().is_empty() ? GURL() : GetVirtualURL();
 }
 
-void NavigationEntryImpl::set_web_bundle_navigation_info(
-    std::unique_ptr<WebBundleNavigationInfo> web_bundle_navigation_info) {
-  web_bundle_navigation_info_ = std::move(web_bundle_navigation_info);
-}
-
-WebBundleNavigationInfo* NavigationEntryImpl::web_bundle_navigation_info()
-    const {
-  return web_bundle_navigation_info_.get();
-}
-
 }  // namespace content

content/browser/frame_host/navigation_entry_impl.h

@@ -35,8 +35,6 @@
 
 namespace content {
 
-class WebBundleNavigationInfo;
-
 class CONTENT_EXPORT NavigationEntryImpl : public NavigationEntry {
  public:
   // Represents a tree of FrameNavigationEntries that make up this joint session
@@ -404,10 +402,6 @@ class CONTENT_EXPORT NavigationEntryImpl : public NavigationEntry {
     back_forward_cache_metrics_ = metrics;
   }
 
-  void set_web_bundle_navigation_info(
-      std::unique_ptr<WebBundleNavigationInfo> web_bundle_navigation_info);
-  WebBundleNavigationInfo* web_bundle_navigation_info() const;
-
  private:
   // WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING
   // Session/Tab restore save portions of this class so that it can be recreated
@@ -542,14 +536,6 @@ class CONTENT_EXPORT NavigationEntryImpl : public NavigationEntry {
   // It is preserved at commit but not persisted.
   scoped_refptr<BackForwardCacheMetrics> back_forward_cache_metrics_;
 
-  // Keeps the Web Bundles related information when |this| is for a navigation
-  // within a Web Bundle file. Used when WebBundles feature is enabled or
-  // TrustableWebBundleFileUrl switch is set.
-  // TODO(995177): Support Session/Tab restore.
-  // TODO(995177): Consider if this should be here or in FrameNavigationEntry
-  // for a correct iframe support.
-  std::unique_ptr<WebBundleNavigationInfo> web_bundle_navigation_info_;
-
   DISALLOW_COPY_AND_ASSIGN(NavigationEntryImpl);
 };
 

content/browser/frame_host/navigation_request.cc

@@ -948,6 +948,10 @@ NavigationRequest::NavigationRequest(
     frame_entry_item_sequence_number_ = frame_entry->item_sequence_number();
     frame_entry_document_sequence_number_ =
         frame_entry->document_sequence_number();
+    if (frame_entry->web_bundle_navigation_info()) {
+      web_bundle_navigation_info_ =
+          frame_entry->web_bundle_navigation_info()->Clone();
+    }
   }
 
   // Sanitize the referrer.
@@ -1031,10 +1035,6 @@ NavigationRequest::NavigationRequest(
       entry->back_forward_cache_metrics()
           ->MainFrameDidStartNavigationToDocument();
     }
-    if (entry->web_bundle_navigation_info()) {
-      web_bundle_navigation_info_ =
-          entry->web_bundle_navigation_info()->Clone();
-    }
 
     // If this NavigationRequest is for the current pending entry, make sure
     // that we will discard the pending entry if all of associated its requests

content/browser/web_package/web_bundle_browsertest.cc

@@ -974,6 +974,47 @@ IN_PROC_BROWSER_TEST_P(WebBundleFileBrowserTest, Variants) {
                                "fr");
 }
 
+IN_PROC_BROWSER_TEST_P(WebBundleFileBrowserTest, IframeNavigationNoCrash) {
+  // Regression test for crbug.com/1058721. There was a bug that navigation of
+  // OOPIF's remote iframe in Web Bundle file cause crash.
+  const GURL test_data_url =
+      GetTestUrlForFile(GetTestDataPath("web_bundle_browsertest.wbn"));
+  NavigateToBundleAndWaitForReady(
+      test_data_url, web_bundle_utils::GetSynthesizedUrlForWebBundle(
+                         test_data_url, GURL(kTestPageUrl)));
+
+  const std::string empty_page_path = "/web_bundle/empty_page.html";
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL empty_page_url = embedded_test_server()->GetURL(empty_page_path);
+
+  ExecuteScriptAndWaitForTitle(
+      base::StringPrintf(R"(
+    (async function() {
+      const empty_page_url = '%s';
+      const iframe = document.createElement('iframe');
+      const onload = () => {
+        iframe.removeEventListener('load', onload);
+        document.title = 'Iframe loaded';
+      }
+      iframe.addEventListener('load', onload);
+      iframe.src = empty_page_url;
+      document.body.appendChild(iframe);
+    })();)",
+                         empty_page_url.spec().c_str()),
+      "Iframe loaded");
+
+  ExecuteScriptAndWaitForTitle(R"(
+    (async function() {
+      const iframe = document.querySelector("iframe");
+      const onload = () => {
+        document.title = 'Iframe loaded again';
+      }
+      iframe.addEventListener('load', onload);
+      iframe.src = iframe.src + '?';
+    })();)",
+                               "Iframe loaded again");
+}
+
 INSTANTIATE_TEST_SUITE_P(WebBundleFileBrowserTest,
                          WebBundleFileBrowserTest,
                          testing::Values(TestFilePathMode::kNormalFilePath