[mparch] Return an empty manifest on a non primary page in GetManifest

This CL updates GetManifest() to return an empty manifest on a non
primary page since MaybeOverrideManifest() called from
OnRequestManifestResponse() works only with a primary page and it
causes inconsistent behavior on a primary page and a non primary
page.
It also updates ManifestManagerHost with PageUserData since it's
available only for a mainframe.

Bug: 1296125
Change-Id: Ib551afd2eee94051a31dff76f9401316baf6d9e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483876
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Julie Jeongeun Kim <jkim@igalia.com>
Cr-Commit-Position: refs/heads/main@{#983740}

chrome/browser/web_applications/policy/web_app_policy_manager.cc

@@ -444,6 +444,12 @@ void WebAppPolicyManager::MaybeOverrideManifest(
     content::RenderFrameHost* frame_host,
     blink::mojom::ManifestPtr& manifest) const {
 #if BUILDFLAG(IS_CHROMEOS)
+  // This doesn't override the manifest properly on a non primary page since it
+  // checks the url from PreRedirectionURLObserver that works only on a primary
+  // page.
+  if (!frame_host->IsInPrimaryMainFrame())
+    return;
+
   if (!manifest)
     return;
 

content/browser/devtools/protocol/page_handler.cc

@@ -1015,7 +1015,7 @@ void PageHandler::GetAppManifest(
   }
   if (!CanExecuteGlobalCommands(host_, callback))
     return;
-  ManifestManagerHost::GetOrCreateForCurrentDocument(host_->GetMainFrame())
+  ManifestManagerHost::GetOrCreateForPage(host_->GetPage())
       ->RequestManifestDebugInfo(base::BindOnce(&PageHandler::GotManifest,
                                                 weak_factory_.GetWeakPtr(),
                                                 std::move(callback)));

content/browser/manifest/manifest_browsertest.cc

@@ -13,6 +13,7 @@
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/bind.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "content/public/browser/page.h"
@@ -23,6 +24,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/prerender_test_util.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/shell/browser/shell.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
@@ -63,6 +65,9 @@ class MockWebContentsDelegate : public WebContentsDelegate {
                               const std::u16string& message,
                               int32_t line_no,
                               const std::u16string& source_id) override;
+  bool IsPrerender2Supported(WebContents& web_contents) override {
+    return true;
+  }
 
  private:
   raw_ptr<WebContents> web_contents_;
@@ -738,4 +743,76 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, UniqueOrigin) {
   EXPECT_EQ(0u, reported_manifest_urls().size());
 }
 
+class ManifestBrowserPrerenderingTest : public ManifestBrowserTest {
+ public:
+  ManifestBrowserPrerenderingTest()
+      : prerender_helper_(
+            base::BindRepeating(&ManifestBrowserPrerenderingTest::web_contents,
+                                base::Unretained(this))) {}
+
+  ~ManifestBrowserPrerenderingTest() override = default;
+
+ protected:
+  test::PrerenderTestHelper& prerender_helper() { return prerender_helper_; }
+
+ private:
+  test::PrerenderTestHelper prerender_helper_;
+};
+
+// Tests that GetManifest() returns an empty manifest if it's requested in
+// prerendering.
+IN_PROC_BROWSER_TEST_F(ManifestBrowserPrerenderingTest,
+                       GetManifestInPrerendering) {
+  GURL test_url =
+      embedded_test_server()->GetURL("/manifest/empty-manifest.html");
+
+  ASSERT_TRUE(NavigateToURL(shell(), test_url));
+  {
+    base::RunLoop run_loop;
+    web_contents()->GetPrimaryPage().GetManifest(
+        base::BindOnce(base::BindLambdaForTesting(
+            [&](const GURL& manifest_url, blink::mojom::ManifestPtr manifest) {
+              // Get the manifest on a primary page.
+              EXPECT_FALSE(manifest_url.is_empty());
+              EXPECT_FALSE(blink::IsEmptyManifest(*manifest));
+              run_loop.Quit();
+            })));
+    run_loop.Run();
+  }
+
+  GURL prerender_url =
+      embedded_test_server()->GetURL("/manifest/dummy-manifest.html");
+  // Loads a page in the prerender.
+  int host_id = prerender_helper().AddPrerender(prerender_url);
+  content::RenderFrameHost* prerender_rfh =
+      prerender_helper().GetPrerenderedMainFrameHost(host_id);
+  {
+    base::RunLoop run_loop;
+    prerender_rfh->GetPage().GetManifest(
+        base::BindOnce(base::BindLambdaForTesting(
+            [&](const GURL& manifest_url, blink::mojom::ManifestPtr manifest) {
+              // Ensure that the manifest is empty in prerendering.
+              EXPECT_TRUE(manifest_url.is_empty());
+              EXPECT_TRUE(blink::IsEmptyManifest(*manifest));
+              run_loop.Quit();
+            })));
+    run_loop.Run();
+  }
+
+  prerender_helper().NavigatePrimaryPage(prerender_url);
+  {
+    base::RunLoop run_loop;
+    prerender_rfh->GetPage().GetManifest(
+        base::BindOnce(base::BindLambdaForTesting(
+            [&](const GURL& manifest_url, blink::mojom::ManifestPtr manifest) {
+              // Ensure that getting the manifest works after prerendering
+              // activation.
+              EXPECT_FALSE(manifest_url.is_empty());
+              EXPECT_FALSE(blink::IsEmptyManifest(*manifest));
+              run_loop.Quit();
+            })));
+    run_loop.Run();
+  }
+}
+
 } // namespace content

content/browser/manifest/manifest_manager_host.cc

@@ -16,11 +16,8 @@
 
 namespace content {
 
-ManifestManagerHost::ManifestManagerHost(RenderFrameHost* rfh)
-    : DocumentUserData<ManifestManagerHost>(rfh) {
-  // Check that |rfh| is a main frame.
-  DCHECK(!rfh->GetParent());
-}
+ManifestManagerHost::ManifestManagerHost(Page& page)
+    : PageUserData<ManifestManagerHost>(page) {}
 
 ManifestManagerHost::~ManifestManagerHost() {
   DispatchPendingCallbacks();
@@ -31,12 +28,21 @@ void ManifestManagerHost::BindObserver(
         receiver) {
   manifest_url_change_observer_receiver_.Bind(std::move(receiver));
   manifest_url_change_observer_receiver_.SetFilter(
-      static_cast<RenderFrameHostImpl&>(render_frame_host())
+      static_cast<RenderFrameHostImpl&>(page().GetMainDocument())
           .CreateMessageFilterForAssociatedReceiver(
               blink::mojom::ManifestUrlChangeObserver::Name_));
 }
 
 void ManifestManagerHost::GetManifest(GetManifestCallback callback) {
+  // Do not call into MaybeOverrideManifest in a non primary page since
+  // it checks the url from PreRedirectionURLObserver that works only in
+  // a primary page.
+  // TODO(crbug.com/1296125): Maybe cancel prerendering if it hits this.
+  if (!page().IsPrimary()) {
+    std::move(callback).Run(GURL(), blink::mojom::Manifest::New());
+    return;
+  }
+
   auto& manifest_manager = GetManifestManager();
   int request_id = callbacks_.Add(
       std::make_unique<GetManifestCallback>(std::move(callback)));
@@ -52,7 +58,7 @@ void ManifestManagerHost::RequestManifestDebugInfo(
 
 blink::mojom::ManifestManager& ManifestManagerHost::GetManifestManager() {
   if (!manifest_manager_) {
-    render_frame_host().GetRemoteInterfaces()->GetInterface(
+    page().GetMainDocument().GetRemoteInterfaces()->GetInterface(
         manifest_manager_.BindNewPipeAndPassReceiver());
     manifest_manager_.set_disconnect_handler(base::BindOnce(
         &ManifestManagerHost::OnConnectionError, base::Unretained(this)));
@@ -72,27 +78,24 @@ void ManifestManagerHost::DispatchPendingCallbacks() {
 
 void ManifestManagerHost::OnConnectionError() {
   DispatchPendingCallbacks();
-  if (GetForCurrentDocument(&render_frame_host())) {
-    DeleteForCurrentDocument(&render_frame_host());
-  }
+  if (GetForPage(page()))
+    DeleteForPage(page());
 }
 
 void ManifestManagerHost::OnRequestManifestResponse(
     int request_id,
     const GURL& url,
     blink::mojom::ManifestPtr manifest) {
-  GetContentClient()->browser()->MaybeOverrideManifest(&render_frame_host(),
-                                                       manifest);
+  GetContentClient()->browser()->MaybeOverrideManifest(
+      &page().GetMainDocument(), manifest);
   auto callback = std::move(*callbacks_.Lookup(request_id));
   callbacks_.Remove(request_id);
   std::move(callback).Run(url, std::move(manifest));
 }
 
 void ManifestManagerHost::ManifestUrlChanged(const GURL& manifest_url) {
-  static_cast<RenderFrameHostImpl&>(render_frame_host())
-      .GetPage()
-      .UpdateManifestUrl(manifest_url);
+  static_cast<PageImpl&>(page()).UpdateManifestUrl(manifest_url);
 }
 
-DOCUMENT_USER_DATA_KEY_IMPL(ManifestManagerHost);
+PAGE_USER_DATA_KEY_IMPL(ManifestManagerHost);
 }  // namespace content

content/browser/manifest/manifest_manager_host.h

@@ -7,7 +7,7 @@
 
 #include "base/callback_forward.h"
 #include "base/containers/id_map.h"
-#include "content/public/browser/document_user_data.h"
+#include "content/public/browser/page_user_data.h"
 #include "mojo/public/cpp/bindings/associated_receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "third_party/blink/public/mojom/manifest/manifest.mojom-forward.h"
@@ -20,7 +20,7 @@ namespace content {
 // associated with the main frame of the observed WebContents. It handles the
 // IPC messaging with the child process.
 // TODO(mlamouri): keep a cached version and a dirty bit here.
-class ManifestManagerHost : public DocumentUserData<ManifestManagerHost>,
+class ManifestManagerHost : public PageUserData<ManifestManagerHost>,
                             public blink::mojom::ManifestUrlChangeObserver {
  public:
   ManifestManagerHost(const ManifestManagerHost&) = delete;
@@ -44,9 +44,9 @@ class ManifestManagerHost : public DocumentUserData<ManifestManagerHost>,
           receiver);
 
  private:
-  explicit ManifestManagerHost(RenderFrameHost* render_frame_host);
+  explicit ManifestManagerHost(Page& page);
 
-  friend class DocumentUserData<ManifestManagerHost>;
+  friend class PageUserData<ManifestManagerHost>;
 
   using CallbackMap = base::IDMap<std::unique_ptr<GetManifestCallback>>;
 
@@ -68,7 +68,7 @@ class ManifestManagerHost : public DocumentUserData<ManifestManagerHost>,
   mojo::AssociatedReceiver<blink::mojom::ManifestUrlChangeObserver>
       manifest_url_change_observer_receiver_{this};
 
-  DOCUMENT_USER_DATA_KEY_DECL();
+  PAGE_USER_DATA_KEY_DECL();
 };
 
 }  // namespace content

content/browser/payments/payment_app_info_fetcher.cc

@@ -126,7 +126,7 @@ void PaymentAppInfoFetcher::SelfDeleteFetcher::Start(
     }
 
     web_contents_ = web_content->GetWeakPtr();
-    web_contents_->GetMainFrame()->GetPage().GetManifest(
+    web_contents_->GetPrimaryPage().GetManifest(
         base::BindOnce(&PaymentAppInfoFetcher::SelfDeleteFetcher::
                            FetchPaymentAppManifestCallback,
                        weak_ptr_factory_.GetWeakPtr()));

content/browser/renderer_host/page_impl.cc

@@ -17,6 +17,7 @@
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/public/browser/render_view_host.h"
 #include "third_party/blink/public/common/loader/loader_constants.h"
+#include "third_party/blink/public/mojom/manifest/manifest.mojom.h"
 #include "third_party/perfetto/include/perfetto/tracing/traced_value.h"
 
 namespace content {
@@ -41,7 +42,7 @@ const absl::optional<GURL>& PageImpl::GetManifestUrl() const {
 
 void PageImpl::GetManifest(GetManifestCallback callback) {
   ManifestManagerHost* manifest_manager_host =
-      ManifestManagerHost::GetOrCreateForCurrentDocument(&main_document_);
+      ManifestManagerHost::GetOrCreateForPage(*this);
   manifest_manager_host->GetManifest(std::move(callback));
 }
 

content/browser/renderer_host/render_frame_host_impl.cc

@@ -8799,7 +8799,7 @@ void RenderFrameHostImpl::SetUpMojoConnection() {
         [](RenderFrameHostImpl* impl,
            mojo::PendingAssociatedReceiver<
                blink::mojom::ManifestUrlChangeObserver> receiver) {
-          ManifestManagerHost::GetOrCreateForCurrentDocument(impl)
+          ManifestManagerHost::GetOrCreateForPage(impl->GetPage())
               ->BindObserver(std::move(receiver));
         },
         base::Unretained(this)));