Portals: Use WebContents::ClosePage to run unload handlers for portals.

On the way, refactor how the Portal <-> WebContentsImpl pointers are
maintained, as this work caused the raw WebContentsImpl* to dangling while
writing the main code of this CL, which made for a confusing bug. This
refactoring is mainly a mechanical gathering of the code responsible for
updating these into a small nested class.

Bug: 964481
Change-Id: I8a63e1a5531de4807569b35ea0f3fcb19baf343c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1976000Reviewed-by: Adithya Srinivasan <adithyas@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728057}

content/browser/portal/portal.h

@@ -68,6 +68,12 @@ class CONTENT_EXPORT Portal : public blink::mojom::Portal,
   // the proxy.
   RenderFrameProxyHost* CreateProxyAndAttachPortal();
 
+  // Closes the contents associated with this object gracefully, and destroys
+  // itself thereafter. This will fire unload and related event handlers.
+  // Once closing begins, the Portal interface receiver is closed. The host
+  // document can no longer manage the lifetime.
+  void Close();
+
   // blink::mojom::Portal implementation.
   void Navigate(const GURL& url,
                 blink::mojom::ReferrerPtr referrer,
@@ -94,6 +100,7 @@ class CONTENT_EXPORT Portal : public blink::mojom::Portal,
   void LoadingStateChanged(WebContents* source,
                            bool to_different_document) override;
   void PortalWebContentsCreated(WebContents* portal_web_contents) override;
+  void CloseContents(WebContents*) override;
 
   // Returns the token which uniquely identifies this Portal.
   const base::UnguessableToken& portal_token() const { return portal_token_; }
@@ -121,6 +128,57 @@ class CONTENT_EXPORT Portal : public blink::mojom::Portal,
   blink::mojom::PortalClient& client() { return *(client_.get()); }
 
  private:
+  // Manages the relationship between the Portal and its guest WebContents.
+  //
+  // The WebContents may either be:
+  // * owned by this object (via unique_ptr) when it is not attached to the
+  //   FrameTreeNode/WebContents tree, e.g. during activation but before
+  //   adoption
+  // * unowned by this object, in which case it is owned elsewhere, generally
+  //   via by WebContentsTreeNode
+  //
+  // It can transition between these two states. In either state, the Portal
+  // must be configured as the portal and delegate of the WebContents.
+  //
+  // Finally, if the Portal drops its relationship with a WebContents, it must
+  // also stop observing the corresponding outer FrameTreeNode.
+  class WebContentsHolder {
+   public:
+    explicit WebContentsHolder(Portal* portal);
+    WebContentsHolder(const WebContentsHolder&) = delete;
+    ~WebContentsHolder();
+
+    WebContentsHolder& operator=(const WebContentsHolder&) = delete;
+
+    explicit operator bool() const { return contents_; }
+    WebContentsImpl& operator*() const { return *contents_; }
+    WebContentsImpl* operator->() const { return contents_; }
+    WebContentsImpl* get() const { return contents_; }
+    bool OwnsContents() const;
+
+    void SetUnowned(WebContentsImpl*);
+    void SetOwned(std::unique_ptr<WebContents>);
+    void Clear();
+
+    // Maintains a link to the same contents, but yields ownership to the
+    // caller.
+    std::unique_ptr<WebContents> ReleaseOwnership() {
+      DCHECK(OwnsContents());
+      return std::move(owned_contents_);
+    }
+
+   private:
+    // The outer Portal object.
+    Portal* portal_ = nullptr;
+
+    // Non-null, even when the contents is not owned.
+    WebContentsImpl* contents_ = nullptr;
+
+    // When the portal is not attached, the Portal owns its WebContents.
+    // If not null, |owned_contents_| is equal to |contents_|.
+    std::unique_ptr<WebContents> owned_contents_;
+  };
+
   void SetPortalContents(std::unique_ptr<WebContents> web_contents);
 
   RenderFrameHostImpl* owner_render_frame_host_;
@@ -141,9 +199,10 @@ class CONTENT_EXPORT Portal : public blink::mojom::Portal,
   mojo::AssociatedRemote<blink::mojom::PortalClient> client_;
 
   // When the portal is not attached, the Portal owns its WebContents.
-  std::unique_ptr<WebContents> portal_contents_;
+  WebContentsHolder portal_contents_{this};
 
-  WebContentsImpl* portal_contents_impl_ = nullptr;
+  // Set when |Close| is called. Destruction will occur shortly thereafter.
+  bool is_closing_ = false;
 
   // Another implementation of blink::mojom::Portal to bind instead.
   // For use in testing only.

content/browser/portal/portal_browsertest.cc

@@ -10,6 +10,8 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
+#include "components/viz/host/host_frame_sink_manager.h"
+#include "content/browser/compositor/surface_utils.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/frame_host/render_frame_host_manager.h"
 #include "content/browser/frame_host/render_frame_proxy_host.h"
@@ -17,6 +19,7 @@
 #include "content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.h"
 #include "content/browser/renderer_host/input/synthetic_tap_gesture.h"
 #include "content/browser/renderer_host/render_widget_host_input_event_router.h"
+#include "content/browser/renderer_host/render_widget_host_view_base.h"
 #include "content/browser/renderer_host/render_widget_host_view_child_frame.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame.mojom-test-utils.h"
@@ -512,7 +515,33 @@ IN_PROC_BROWSER_TEST_F(PortalHitTestBrowserTest, InputToOOPIFAfterActivation) {
                        "portal.activate().then(() => { "
                        "  document.body.removeChild(portal); "
                        "});"));
-    activated_observer.WaitForActivateAndHitTestData();
+    activated_observer.WaitForActivate();
+
+    RenderWidgetHostViewBase* view =
+        portal_frame->GetRenderWidgetHost()->GetView();
+    viz::FrameSinkId root_frame_sink_id = view->GetRootFrameSinkId();
+    HitTestRegionObserver hit_test_observer(root_frame_sink_id);
+
+    // The hit test region for the portal frame should be at index 1 after
+    // activation, so we wait for the hit test data to update until it's in
+    // this state.
+    auto hit_test_index = [&]() -> base::Optional<size_t> {
+      const auto& display_hit_test_query_map =
+          GetHostFrameSinkManager()->display_hit_test_query();
+      auto it = display_hit_test_query_map.find(root_frame_sink_id);
+      // On Mac, we create a new root layer after activation, so the hit test
+      // data may not have anything for the new layer yet.
+      if (it == display_hit_test_query_map.end())
+        return base::nullopt;
+      CHECK_EQ(portal_frame->GetRenderWidgetHost()->GetView(), view);
+      size_t index;
+      if (!it->second->FindIndexOfFrameSink(view->GetFrameSinkId(), &index))
+        return base::nullopt;
+      return index;
+    };
+    hit_test_observer.WaitForHitTestData();
+    while (hit_test_index() != 1u)
+      hit_test_observer.WaitForHitTestDataChange();
   }
   EXPECT_EQ(shell()->web_contents(), portal_contents);
 

content/test/BUILD.gn

@@ -1090,6 +1090,7 @@ test("content_browsertests") {
     "//components/services/storage/public/cpp",
     "//components/ukm:test_support",
     "//components/url_formatter:url_formatter",
+    "//components/viz/host",
     "//components/viz/test:test_support",
     "//content:resources",
     "//content/app:both_for_content_tests",

content/test/portal/portal_activated_observer.cc

@@ -6,11 +6,7 @@
 
 #include "base/auto_reset.h"
 #include "base/run_loop.h"
-#include "components/viz/host/host_frame_sink_manager.h"
-#include "content/browser/compositor/surface_utils.h"
-#include "content/browser/renderer_host/render_widget_host_view_base.h"
 #include "content/browser/web_contents/web_contents_impl.h"
-#include "content/public/test/hit_test_region_observer.h"
 
 namespace content {
 
@@ -49,38 +45,6 @@ PortalActivatedObserver::WaitForActivateResult() {
   return *result_;
 }
 
-void PortalActivatedObserver::WaitForActivateAndHitTestData() {
-  RenderFrameHostImpl* portal_frame =
-      interceptor_->GetPortalContents()->GetMainFrame();
-  WaitForActivate();
-
-  RenderWidgetHostViewBase* view =
-      portal_frame->GetRenderWidgetHost()->GetView();
-  viz::FrameSinkId root_frame_sink_id = view->GetRootFrameSinkId();
-  HitTestRegionObserver observer(root_frame_sink_id);
-  observer.WaitForHitTestData();
-
-  while (true) {
-    const auto& display_hit_test_query_map =
-        GetHostFrameSinkManager()->display_hit_test_query();
-    auto it = display_hit_test_query_map.find(root_frame_sink_id);
-    // On Mac, we create a new root layer after activation, so the hit test data
-    // may not have anything for the new layer yet.
-    if (it != display_hit_test_query_map.end()) {
-      viz::HitTestQuery* query = it->second.get();
-      size_t index = 0;
-      if (query->FindIndexOfFrameSink(view->GetFrameSinkId(), &index)) {
-        // The hit test region for the portal frame should be at index 1 after
-        // activation, so we wait for the hit test data to update until it's in
-        // this state.
-        if (index == 1)
-          return;
-      }
-    }
-    observer.WaitForHitTestDataChange();
-  }
-}
-
 void PortalActivatedObserver::OnPortalActivate() {
   DCHECK(!has_activated_)
       << "PortalActivatedObserver can't handle overlapping activations.";

content/test/portal/portal_activated_observer.h

@@ -44,10 +44,6 @@ class PortalActivatedObserver : public PortalInterceptorForTesting::Observer {
   // point.
   blink::mojom::PortalActivateResult WaitForActivateResult();
 
-  // Waits for the Activate method to be called by the predecessor renderer and
-  // for hit test data to be refreshed.
-  void WaitForActivateAndHitTestData();
-
  private:
   // PortalInterceptorForTesting::Observer:
   void OnPortalActivate() override;

third_party/blink/web_tests/external/wpt/portals/predecessor-fires-unload.html

+<!DOCTYPE html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/open-blank-host.js"></script>
+<script>
+function nextEvent(target, type) {
+  return new Promise((resolve, reject) => target.addEventListener(type, e => resolve(e), {once: true}));
+}
+
+function timePasses(delay) {
+  return new Promise((resolve, reject) => step_timeout(() => resolve(), delay));
+}
+
+promise_test(async () => {
+  const w = await openBlankPortalHost();
+  try {
+    const portal = w.document.createElement('portal');
+    portal.src = new URL('resources/simple-portal.html', location.href);
+    w.document.body.appendChild(portal);
+    const pagehideFired = nextEvent(w, 'pagehide');
+    const unloadFired = nextEvent(w, 'unload');
+    await portal.activate();
+    assert_true((await pagehideFired) instanceof w.PageTransitionEvent);
+    assert_true((await unloadFired) instanceof w.Event);
+  } finally {
+    w.close();
+  }
+}, "pagehide and unload should fire if the predecessor is not adopted");
+
+promise_test(async () => {
+  localStorage.setItem('predecessor-fires-unload-events', '');
+  window.open('resources/predecessor-fires-unload-watch-unload.html', '_blank', 'noopener');
+  while (localStorage.getItem('predecessor-fires-unload-events') != 'pagehide unload') {
+    await timePasses(50);
+  }
+}, "pagehide and unload should fire if the predecessor is not adopted, even without a window/opener association");
+</script>

third_party/blink/web_tests/external/wpt/portals/resources/predecessor-fires-unload-watch-unload.html

+<!DOCTYPE html>
+<body>
+<script>
+function nextEvent(target, type) {
+  return new Promise((resolve, reject) => target.addEventListener(type, e => resolve(e), {once: true}));
+}
+
+onload = function() {
+  const portal = document.createElement('portal');
+  portal.src = new URL('simple-portal.html', location.href);
+  document.body.appendChild(portal);
+
+  let firedEvents = [];
+  for (let type of ['pagehide', 'unload']) {
+    nextEvent(window, type).then(() => {
+      firedEvents.push(type);
+      localStorage.setItem('predecessor-fires-unload-events', firedEvents.join(' '));
+    });
+  }
+
+  portal.activate();
+}
+</script>
+</body>