prerendering: Migrate to one Activate IPC per RenderView only.

Before this CL, we had an Activate IPC per RenderView and also an
Activate IPC for each document. This CL removes the per-document one.

This fixes a race where a document created in the renderer before the
RenderView IPC arrives starts with document.prerendering true, but it
never receives the per-document IPC so never transitions to
document.prerendering false.

This is the next step after commit 84e9a9b. See [1], [2] for more
details on the design decision.

This CL does not add tests because it is difficult to test the above
race condition. The existing tests including ones added in
https://chromium-review.googlesource.com/c/chromium/src/+/3034321 should
continue to pass.

[1] https://groups.google.com/a/chromium.org/g/navigation-dev/c/v3T8NfWPTg8/m/brmF98nKAAAJ
[2] https://docs.google.com/document/d/1S8_ilRC_TkDa4Wz3PvBPYhhqIuGNHDX0DKsvjMFotKM/edit?usp=sharing

Bug: 1229552
Change-Id: Iabadf0ed2c1ca36b1153553aeed0f9c412a4ba49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3033843
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904994}

content/browser/mojo_binder_policy_applier.cc

@@ -88,11 +88,20 @@ void MojoBinderPolicyApplier::PrepareToGrantAll() {
 void MojoBinderPolicyApplier::GrantAll() {
   DCHECK_NE(mode_, Mode::kGrantAll);
 
-  // GrantAll() should be called inside a Mojo message call stack, because it
-  // binds deferred receivers by invoking
-  // BrowserInterfaceBroker::BindInterface(), which assumes it is called within
-  // a Mojo messaging call. See https://crbug.com/1217977 for more information.
-  DCHECK(mojo::GetBadMessageCallback());
+  // Check that we are in a Mojo message dispatch, since the deferred binders
+  // might call mojo::ReportBadMessage().
+  //
+  // TODO(https://crbug.com/1217977): Give the deferred_binders_ a
+  // BadMessageCallback and forbid them from using mojo::ReportBadMessage()
+  // directly. We are currently in the message stack of one of the PageBroadcast
+  // Mojo callbacks handled by RenderViewHost, so if a binder calls
+  // mojo::ReportBadMessage() it kills possibly the wrong renderer. Even if we
+  // only run the binders associated with the RVH for each message per-RVH,
+  // there are still subtle problems with running all these callbacks at once:
+  // for example, mojo::GetMessageCallback()/mojo::ReportBadMessage() can only
+  // be called once per message dispatch.
+  DCHECK(mojo::IsInMessageDispatch());
+
   mode_ = Mode::kGrantAll;
 
   // It's safe to iterate over `deferred_binders_` because no more callbacks

content/browser/renderer_host/navigation_controller_impl_unittest.cc

@@ -108,8 +108,12 @@ class MockPageBroadcast : public blink::mojom::PageBroadcast {
                SetPageLifecycleStateCallback callback),
               (override));
   MOCK_METHOD(void, AudioStateChanged, (bool is_audio_playing), (override));
-  MOCK_METHOD(void, ActivatePrerenderedPage, (), (override));
   MOCK_METHOD(void, SetInsidePortal, (bool is_inside_portal), (override));
+  MOCK_METHOD(void,
+              ActivatePrerenderedPage,
+              (base::TimeTicks navigation_start,
+               ActivatePrerenderedPageCallback callback),
+              (override));
   MOCK_METHOD(void,
               UpdateWebPreferences,
               (const ::blink::web_pref::WebPreferences& preferences),

content/browser/renderer_host/page_impl.cc

@@ -4,12 +4,14 @@
 
 #include "content/browser/renderer_host/page_impl.h"
 
+#include "base/barrier_closure.h"
 #include "content/browser/manifest/manifest_manager_host.h"
 #include "content/browser/renderer_host/frame_tree_node.h"
 #include "content/browser/renderer_host/page_delegate.h"
 #include "content/browser/renderer_host/render_frame_host_delegate.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
+#include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/public/browser/render_view_host.h"
 #include "third_party/perfetto/include/perfetto/tracing/traced_value.h"
 
@@ -91,6 +93,61 @@ void PageImpl::SetContentsMimeType(std::string mime_type) {
   contents_mime_type_ = std::move(mime_type);
 }
 
+void PageImpl::SetActivationStartTime(base::TimeTicks activation_start) {
+  DCHECK(!activation_start_time_for_prerendering_);
+  activation_start_time_for_prerendering_ = activation_start;
+}
+
+void PageImpl::ActivateForPrerendering(
+    std::set<RenderViewHostImpl*>& render_view_hosts) {
+  base::OnceClosure did_activate_render_views =
+      base::BindOnce(&PageImpl::DidActivateAllRenderViewsForPrerendering,
+                     weak_factory_.GetWeakPtr());
+
+  base::RepeatingClosure barrier = base::BarrierClosure(
+      render_view_hosts.size(), std::move(did_activate_render_views));
+  for (RenderViewHostImpl* rvh : render_view_hosts) {
+    base::TimeTicks navigation_start_to_send;
+    // Only send navigation_start to the RenderViewHost for the main frame to
+    // avoid sending the info cross-origin. Only this RenderViewHost needs the
+    // info, as we expect the other RenderViewHosts are made for cross-origin
+    // iframes which have not yet loaded their document. To the renderer, it
+    // just looks like an ongoing navigation is happening in the frame and has
+    // not yet committed. These RenderViews still need to know about activation
+    // so their documents are created in the non-prerendered state once their
+    // navigation is committed.
+    if (main_document_.GetRenderViewHost() == rvh)
+      navigation_start_to_send = *activation_start_time_for_prerendering_;
+
+    rvh->ActivatePrerenderedPage(navigation_start_to_send, barrier);
+  }
+
+  // Prepare each RenderFrameHostImpl in this Page for activation.
+  // TODO(https://crbug.com/1232528): Currently we check GetPage() below because
+  // RenderFrameHostImpls may be in a different Page, if, e.g., they are in an
+  // inner WebContents. These are in a different FrameTree which might not know
+  // it is being prerendered. We should teach these FrameTrees that they are
+  // being prerendered, or ban inner FrameTrees in a prerendering page.
+  main_document_.ForEachRenderFrameHost(base::BindRepeating(
+      [](PageImpl* page, RenderFrameHostImpl* rfh) {
+        if (&rfh->GetPage() != page)
+          return;
+        rfh->RendererWillActivateForPrerendering();
+      },
+      this));
+}
+
+void PageImpl::DidActivateAllRenderViewsForPrerendering() {
+  // Tell each RenderFrameHostImpl in this Page that activation finished.
+  main_document_.ForEachRenderFrameHost(base::BindRepeating(
+      [](PageImpl* page, RenderFrameHostImpl* rfh) {
+        if (&rfh->GetPage() != page)
+          return;
+        rfh->RendererDidActivateForPrerendering();
+      },
+      this));
+}
+
 RenderFrameHost& PageImpl::GetMainDocumentHelper() {
   return main_document_;
 }

content/browser/renderer_host/page_impl.h

@@ -6,8 +6,11 @@
 #define CONTENT_BROWSER_RENDERER_HOST_PAGE_IMPL_H_
 
 #include <memory>
+#include <set>
 #include <vector>
 
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
 #include "content/browser/fenced_frame/fenced_frame_url_mapping.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/page.h"
@@ -22,6 +25,7 @@ namespace content {
 
 class PageDelegate;
 class RenderFrameHostImpl;
+class RenderViewHostImpl;
 
 // This implements the Page interface that is exposed to embedders of content,
 // and adds things only visible to content.
@@ -92,7 +96,21 @@ class CONTENT_EXPORT PageImpl : public Page {
     return anonymous_iframes_nonce_;
   }
 
+  // Sets the start time of the prerender activation navigation for this page.
+  // TODO(falken): Plumb NavigationRequest to
+  // RenderFrameHostManager::CommitPending and remove this.
+  void SetActivationStartTime(base::TimeTicks activation_start);
+
+  // Called during the prerender activation navigation. Sends an IPC to the
+  // RenderViews in the renderers, instructing them to transition their
+  // documents from prerendered to activated. Tells the corresponding
+  // RenderFrameHostImpls that the renderer will be activating their documents.
+  void ActivateForPrerendering(
+      std::set<RenderViewHostImpl*>& render_view_hosts_to_activate);
+
  private:
+  void DidActivateAllRenderViewsForPrerendering();
+
   // This method is needed to ensure that PageImpl can both implement a Page's
   // method and define a new GetMainDocument(). Please refer to page.h for more
   // details.
@@ -155,6 +173,16 @@ class CONTENT_EXPORT PageImpl : public Page {
   // key of anonymous iframes which are children of this page's document.
   const base::UnguessableToken anonymous_iframes_nonce_ =
       base::UnguessableToken::Create();
+
+  // Prerender2: The start time of the activation navigation for prerendering,
+  // which is passed to the renderer process, and will be accessible in the
+  // prerendered page as PerformanceNavigationTiming.activationStart. Set after
+  // navigation commit.
+  // TODO(falken): Plumb NavigationRequest to
+  // RenderFrameHostManager::CommitPending and remove this.
+  absl::optional<base::TimeTicks> activation_start_time_for_prerendering_;
+
+  base::WeakPtrFactory<PageImpl> weak_factory_{this};
 };
 
 }  // namespace content

content/browser/renderer_host/render_frame_host_impl.cc

@@ -3303,24 +3303,24 @@ void RenderFrameHostImpl::FrameSizeChanged(const gfx::Size& frame_size) {
   delegate_->FrameSizeChanged(this, frame_size);
 }
 
-void RenderFrameHostImpl::DidActivateForPrerendering() {
+void RenderFrameHostImpl::RendererDidActivateForPrerendering() {
   DCHECK(blink::features::IsPrerender2Enabled());
-  if (!is_notifying_activation_for_prerendering_) {
-    mojo::ReportBadMessage("RFHI: DidActivateForPrerendering is unexpected");
-    return;
-  }
-  is_notifying_activation_for_prerendering_ = false;
 
-  // The renderer calls `DidActivateForPrerendering()` to notify that it fired
-  // the prerenderingchange event on a document. The browser now runs any
-  // binders that were deferred during prerendering. This corresponds to the
-  // following steps of the activate algorithm:
+  // RendererDidActivateForPrerendering() is called after the renderer has
+  // notified that it fired the prerenderingchange event on the documents. The
+  // browser now runs any binders that were deferred during prerendering. This
+  // corresponds to the following steps of the activate algorithm:
   //
   // https://jeremyroman.github.io/alternate-loading-modes/#prerendering-browsing-context-activate
   // Step 8.3.4. "For each steps in doc's post-prerendering activation steps
   // list:"
   // Step 8.3.4.1. "Run steps."
-  broker_.ReleaseMojoBinderPolicies();
+
+  // Release Mojo capability control to run the binders. The RenderFrameHostImpl
+  // may have been created after activation started, in which case it already
+  // does not have Mojo capability control applied.
+  if (broker_.GetMojoBinderPolicyApplier())
+    broker_.ReleaseMojoBinderPolicies();
 }
 
 void RenderFrameHostImpl::OnCreateChildFrame(
@@ -8829,10 +8829,10 @@ void RenderFrameHostImpl::CancelPrerenderingByMojoBinderPolicy(
   CancelPrerendering(PrerenderHost::FinalStatus::kMojoBinderPolicy);
 }
 
-void RenderFrameHostImpl::ActivateForPrerendering() {
+void RenderFrameHostImpl::RendererWillActivateForPrerendering() {
   DCHECK(blink::features::IsPrerender2Enabled());
 
-  // Loosen the policies of the mojo capability control during dispatching the
+  // Loosen the policies of the Mojo capability control during dispatching the
   // prerenderingchange event in Blink, because the page may start legitimately
   // using controlled interfaces once prerenderingchange is dispatched. We
   // cannot release policies at this point, i.e., we cannot run the deferred
@@ -8842,24 +8842,6 @@ void RenderFrameHostImpl::ActivateForPrerendering() {
   auto* applier = broker_.GetMojoBinderPolicyApplier();
   DCHECK(applier) << "prerendering pages should have a policy applier";
   applier->PrepareToGrantAll();
-
-  DCHECK(!is_notifying_activation_for_prerendering_);
-  is_notifying_activation_for_prerendering_ = true;
-
-  // Currently cross origin iframes are deferred. So the origin must be same
-  // as the main frame's origin. But if we will decide not to defer the cross
-  // origin iframes, we need to remove the DCHECK_EQ and change the code not
-  // to send |activation_start_time_for_prerendering| to the renderer.
-  DCHECK_EQ(GetLastCommittedOrigin(), GetMainFrame()->GetLastCommittedOrigin());
-
-  // Notify the renderer of activation to update the prerendering state and
-  // dispatch the prerenderingchange event.
-  GetAssociatedLocalFrame()->ActivateForPrerendering(
-      *GetMainFrame()
-           ->document_associated_data_->activation_start_time_for_prerendering);
-
-  for (auto& child : children_)
-    child->current_frame_host()->ActivateForPrerendering();
 }
 
 void RenderFrameHostImpl::BindMediaInterfaceFactoryReceiver(
@@ -10004,18 +9986,7 @@ bool RenderFrameHostImpl::DidCommitNavigationInternal(
       // Set the NavigationStart time for
       // PerformanceNavigationTiming.activationStart.
       // https://jeremyroman.github.io/alternate-loading-modes/#performance-navigation-timing-extension
-
-      // Currently, prerendering is only supported on same-origin pages. When
-      // supporting cross-origin prerendering (https://crbug.com/1176054), we
-      // need to change this CHECK to "if ()" not to send the activation start
-      // time to the prerendering page so that it is not used to send
-      // identifiers between origins.
-      CHECK_EQ(GetLastCommittedOrigin(),
-               navigation_request->GetOriginToCommit());
-      DCHECK(
-          !document_associated_data_->activation_start_time_for_prerendering);
-      document_associated_data_->activation_start_time_for_prerendering =
-          navigation_request->NavigationStart();
+      GetPage().SetActivationStartTime(navigation_request->NavigationStart());
     }
 
   } else {

content/browser/renderer_host/render_frame_host_impl.h

@@ -1717,9 +1717,15 @@ class CONTENT_EXPORT RenderFrameHostImpl
   void CancelPrerenderingByMojoBinderPolicy(const std::string& interface_name);
 
   // Prerender2:
-  // Tells the renderer to dispatch the prerenderingchange event. Expects to
-  // receive an acknowledgement via DidActivateForPrerendering().
-  void ActivateForPrerendering();
+  // Called when the Activate IPC is sent to the renderer. Puts the
+  // MojoPolicyBinderApplier in "loose" mode via PrepareToGrantAll() until
+  // DidActivateForPrerending() is called.
+  void RendererWillActivateForPrerendering();
+
+  // Prerender2:
+  // Called when the Activate IPC is acknowledged by the renderer. Relinquishes
+  // the MojoPolicyBinderApplier.
+  void RendererDidActivateForPrerendering();
 
   // https://mikewest.github.io/corpp/#initialize-embedder-policy-for-global
   const network::CrossOriginEmbedderPolicy& cross_origin_embedder_policy()
@@ -2025,7 +2031,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
       const absl::optional<std::u16string>& source_id,
       const absl::optional<std::u16string>& untrusted_stack_trace) override;
   void FrameSizeChanged(const gfx::Size& frame_size) override;
-  void DidActivateForPrerendering() override;
 
   // blink::LocalMainFrameHost overrides:
   void ScaleFactorChanged(float scale) override;
@@ -3746,12 +3751,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
     // The Page object associated with the main document. It is nullptr for
     // subframes.
     std::unique_ptr<PageImpl> owned_page;
-
-    // Prerender2:
-    // The activation start time for prerendering which is passed to the
-    // renderer process, and will be accessible in the prerendered page as
-    // PerformanceNavigationTiming.activationStart.
-    absl::optional<base::TimeTicks> activation_start_time_for_prerendering;
   };
 
   std::unique_ptr<DocumentAssociatedData> document_associated_data_;
@@ -3844,11 +3843,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // class for more information.
   scoped_refptr<PolicyContainerHost> policy_container_host_;
 
-  // Prerender2:
-  // This is true while notifying the frame in the renderer of activation for
-  // prerendering.
-  bool is_notifying_activation_for_prerendering_ = false;
-
   // The current document's HTTP response head. This is used by back-forward
   // cache, for navigating a second time toward the same document.
   network::mojom::URLResponseHeadPtr last_response_head_;

content/browser/renderer_host/render_frame_host_manager.cc

@@ -3297,10 +3297,8 @@ void RenderFrameHostManager::CommitPending(
     } else {
       DCHECK_EQ(prev_state,
                 RenderFrameHostImpl::LifecycleStateImpl::kPrerendering);
-      for (RenderViewHostImpl* rvh : render_view_hosts_to_restore) {
-        rvh->ActivatePrerenderedPage();
-      }
-      current_frame_host()->ActivateForPrerendering();
+      current_frame_host()->GetPage().ActivateForPrerendering(
+          render_view_hosts_to_restore);
     }
   }
 

content/browser/renderer_host/render_view_host_impl.cc

@@ -560,13 +560,16 @@ void RenderViewHostImpl::LeaveBackForwardCache(
       is_in_back_forward_cache_, std::move(page_restore_params));
 }
 
-void RenderViewHostImpl::ActivatePrerenderedPage() {
-  // Null in some unit tests that use TestRenderViewHost.
-  // TODO(falken): Bind this in tests.
-  if (!page_broadcast_)
-    return;
-
-  page_broadcast_->ActivatePrerenderedPage();
+void RenderViewHostImpl::ActivatePrerenderedPage(
+    base::TimeTicks activation_start,
+    base::OnceClosure callback) {
+  // TODO(https://crbug.com/1217977): Consider using a ScopedClosureRunner here
+  // in case the renderer crashes before it can send us the callback. But we
+  // can't do that until the linked bug is fixed, or else we can reach
+  // DidActivateForPrerendering() outside of a Mojo message dispatch which
+  // breaks the DCHECK for releasing Mojo Capability Control.
+  page_broadcast_->ActivatePrerenderedPage(activation_start,
+                                           std::move(callback));
 }
 
 void RenderViewHostImpl::SetFrameTreeVisibility(

content/browser/renderer_host/render_view_host_impl.h

@@ -240,7 +240,8 @@ class CONTENT_EXPORT RenderViewHostImpl
 
   bool is_in_back_forward_cache() const { return is_in_back_forward_cache_; }
 
-  void ActivatePrerenderedPage();
+  void ActivatePrerenderedPage(base::TimeTicks activation_start,
+                               base::OnceClosure callback);
 
   void SetFrameTreeVisibility(blink::mojom::PageVisibilityState visibility);
 

content/public/test/fake_local_frame.cc

@@ -165,9 +165,6 @@ void FakeLocalFrame::MixedContentFound(
     bool had_redirect,
     network::mojom::SourceLocationPtr source_location) {}
 
-void FakeLocalFrame::ActivateForPrerendering(base::TimeTicks activation_start) {
-}
-
 void FakeLocalFrame::BindDevToolsAgent(
     mojo::PendingAssociatedRemote<blink::mojom::DevToolsAgentHost> host,
     mojo::PendingAssociatedReceiver<blink::mojom::DevToolsAgent> receiver) {}

content/public/test/fake_local_frame.h

@@ -124,7 +124,6 @@ class FakeLocalFrame : public blink::mojom::LocalFrame {
       const GURL& url_before_redirects,
       bool had_redirect,
       network::mojom::SourceLocationPtr source_location) override;
-  void ActivateForPrerendering(base::TimeTicks activation_start) override;
   void BindDevToolsAgent(
       mojo::PendingAssociatedRemote<blink::mojom::DevToolsAgentHost> host,
       mojo::PendingAssociatedReceiver<blink::mojom::DevToolsAgent> receiver)

content/test/BUILD.gn

@@ -372,6 +372,8 @@ static_library("test_support") {
     "test_navigation_url_loader_factory.h",
     "test_overscroll_delegate.cc",
     "test_overscroll_delegate.h",
+    "test_page_broadcast.cc",
+    "test_page_broadcast.h",
     "test_render_frame.cc",
     "test_render_frame.h",
     "test_render_frame_host.cc",