[Portals] Disallow activation during unload

This CL makes it so that portal activation is blocked when the document
enters a beforeunload handler. If the unload proceeds, portal activation
will continue to be blocked

This must be implemented inside the renderer as the renderer can
initiate navigations which the browser won't hear about until after
beforeunload is confirmed. We do this by looking at the Document's
LoadEventProgress state.

Unfortunately, the current behavior is that, following a beforeunload,
this state would progress unconditionally to
kBeforeUnloadEventCompleted. This prevents us from distinguishing the
case where the navigation is canceled, which should once again allow
portal activation. This CL updates this state to only progress to the
(renamed for consistency and clarity) kBeforeUnloadEventCompleted state
only after we confirm the unload will proceed.

Bug: 1043764
Change-Id: I80363ccf2228e3dc5434486c95c62c17e00743aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2251104
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780470}

content/browser/portal/portal_browsertest.cc

@@ -1282,8 +1282,9 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest,
   EXPECT_EQ(bad_message::RPH_MOJO_PROCESS_ERROR, kill_waiter.Wait());
 }
 
-// Tests that activation early in navigation succeeds, cancelling the pending
-// navigation.
+// Tests that activation early in navigation fails. Even though the navigation
+// hasn't yet committed, allowing activation could allow a portal to prevent
+// the user from navigating away.
 IN_PROC_BROWSER_TEST_F(PortalBrowserTest, ActivateEarlyInNavigation) {
   EXPECT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("portal.test", "/title1.html")));
@@ -1292,8 +1293,7 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, ActivateEarlyInNavigation) {
   RenderFrameHostImpl* main_frame = web_contents_impl->GetMainFrame();
 
   GURL url = embedded_test_server()->GetURL("a.com", "/title2.html");
-  Portal* portal = CreatePortalToUrl(web_contents_impl, url);
-  WebContents* portal_contents = portal->GetPortalContents();
+  CreatePortalToUrl(web_contents_impl, url);
 
   // Have the outer page try to navigate away but stop it early in the request,
   // where it is still possible to stop.
@@ -1308,16 +1308,21 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, ActivateEarlyInNavigation) {
   web_contents_impl->GetController().LoadURLWithParams(params);
   ASSERT_TRUE(navigation_manager.WaitForRequestStart());
 
-  // Then activate the portal. Since this is early in navigation, it should be
-  // aborted and the portal activation should succeed.
-  PortalActivatedObserver activated_observer(portal);
-  ExecuteScriptAsync(main_frame,
-                     "document.querySelector('portal').activate();");
-  EXPECT_EQ(blink::mojom::PortalActivateResult::kPredecessorWillUnload,
-            activated_observer.WaitForActivateResult());
-  EXPECT_EQ(portal_contents, shell()->web_contents());
+  // Then activate the portal, because navigation has begun and beforeunload
+  // has been dispatched, the activation should fail.
+  EvalJsResult result = EvalJs(main_frame,
+                               "document.querySelector('portal').activate()"
+                               ".then(() => 'success', e => e.message)");
+  EXPECT_THAT(result.ExtractString(),
+              ::testing::HasSubstr("Cannot activate portal while document is in"
+                                   " beforeunload or has started unloading"));
+
+  // The navigation should commit properly thereafter.
+  navigation_manager.WaitForNavigationFinished();
   navigation_observer.Wait();
-  EXPECT_EQ(handle_observer.net_error_code(), net::ERR_ABORTED);
+  EXPECT_EQ(web_contents_impl, shell()->web_contents());
+  EXPECT_TRUE(navigation_observer.last_navigation_succeeded());
+  EXPECT_EQ(destination, navigation_observer.last_navigation_url());
 }
 
 // Tests that activation late in navigation is rejected (since it's too late to
@@ -1330,7 +1335,7 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, ActivateLateInNavigation) {
   RenderFrameHostImpl* main_frame = web_contents_impl->GetMainFrame();
 
   GURL url = embedded_test_server()->GetURL("a.com", "/title2.html");
-  Portal* portal = CreatePortalToUrl(web_contents_impl, url);
+  CreatePortalToUrl(web_contents_impl, url);
 
   // Have the outer page try to navigate away and reach the point where it's
   // about to process the response (after which it will commit). It is too late
@@ -1348,15 +1353,12 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, ActivateLateInNavigation) {
   // Then activate the portal. Since this is late in navigation, we expect the
   // activation to fail. Since commit hasn't actually happened yet, though,
   // there is time for the renderer to process the promise rejection.
-  PortalActivatedObserver activated_observer(portal);
   EvalJsResult result = EvalJs(main_frame,
                                "document.querySelector('portal').activate()"
                                ".then(() => 'success', e => e.message)");
   EXPECT_THAT(result.ExtractString(),
-              ::testing::HasSubstr("navigation is in progress"));
-  EXPECT_EQ(
-      blink::mojom::PortalActivateResult::kRejectedDueToPredecessorNavigation,
-      activated_observer.result());
+              ::testing::HasSubstr("Cannot activate portal while document is in"
+                                   " beforeunload or has started unloading"));
 
   // The navigation should commit properly thereafter.
   navigation_manager.ResumeNavigation();

third_party/blink/renderer/core/BUILD.gn

@@ -1280,6 +1280,7 @@ jumbo_source_set("unit_tests") {
     "loader/document_loader_test.cc",
     "loader/font_preload_manager_test.cc",
     "loader/frame_fetch_context_test.cc",
+    "loader/frame_loader_test.cc",
     "loader/frame_resource_fetcher_properties_test.cc",
     "loader/idleness_detector_test.cc",
     "loader/image_loader_test.cc",

third_party/blink/renderer/core/dom/document.cc

@@ -4029,11 +4029,21 @@ bool Document::DispatchBeforeUnloadEvent(ChromeClient* chrome_client,
   MainThreadDisallowSynchronousXHRScope disallow_synchronous_xhr;
   auto& before_unload_event = *MakeGarbageCollected<BeforeUnloadEvent>();
   before_unload_event.initEvent(event_type_names::kBeforeunload, false, true);
-  load_event_progress_ = kBeforeUnloadEventInProgress;
   const base::TimeTicks beforeunload_event_start = base::TimeTicks::Now();
-  dom_window_->DispatchEvent(before_unload_event, this);
+
+  {
+    // We want to avoid progressing to kBeforeUnloadEventHandled if the page
+    // cancels the unload. Because a subframe may cancel unload on our behalf,
+    // only the caller, which makes this call over the frame subtree, can know
+    // whether or not  we'll unload so the caller is responsible for advancing
+    // to kBeforeUnloadEventHandled. Here, we'll reset back to our prior value
+    // once the handler has run.
+    base::AutoReset<LoadEventProgress> set_in_progress(
+        &load_event_progress_, kBeforeUnloadEventInProgress);
+    dom_window_->DispatchEvent(before_unload_event, this);
+  }
+
   const base::TimeTicks beforeunload_event_end = base::TimeTicks::Now();
-  load_event_progress_ = kBeforeUnloadEventCompleted;
   DEFINE_STATIC_LOCAL(
       CustomCountHistogram, beforeunload_histogram,
       ("DocumentEventTiming.BeforeUnloadDuration", 0, 10000000, 50));
@@ -4219,7 +4229,7 @@ Document::PageDismissalType Document::PageDismissalEventBeingDispatched()
     case kLoadEventNotRun:
     case kLoadEventInProgress:
     case kLoadEventCompleted:
-    case kBeforeUnloadEventCompleted:
+    case kBeforeUnloadEventHandled:
     case kUnloadEventHandled:
       return kNoDismissal;
   }

third_party/blink/renderer/core/dom/document.h

@@ -1225,7 +1225,11 @@ class CORE_EXPORT Document : public ContainerNode,
     kLoadEventInProgress,
     kLoadEventCompleted,
     kBeforeUnloadEventInProgress,
-    kBeforeUnloadEventCompleted,
+    // Advanced to only if the beforeunload event in this document and
+    // subdocuments isn't canceled and will cause an unload. If beforeunload is
+    // canceled |load_event_progress_| will revert to its value prior to the
+    // beforeunload being dispatched.
+    kBeforeUnloadEventHandled,
     kPageHideInProgress,
     kUnloadVisibilityChangeInProgress,
     kUnloadEventInProgress,
@@ -1237,12 +1241,19 @@ class CORE_EXPORT Document : public ContainerNode,
   bool LoadEventFinished() const {
     return load_event_progress_ >= kLoadEventCompleted;
   }
-  bool UnloadStarted() const {
-    return load_event_progress_ >= kPageHideInProgress;
+  bool BeforeUnloadStarted() const {
+    return load_event_progress_ >= kBeforeUnloadEventInProgress;
   }
   bool ProcessingBeforeUnload() const {
     return load_event_progress_ == kBeforeUnloadEventInProgress;
   }
+  bool UnloadStarted() const {
+    return load_event_progress_ >= kPageHideInProgress;
+  }
+
+  void BeforeUnloadDoneWillUnload() {
+    load_event_progress_ = kBeforeUnloadEventHandled;
+  }
 
   void SetContainsPlugins() { contains_plugins_ = true; }
   bool ContainsPlugins() const { return contains_plugins_; }

third_party/blink/renderer/core/html/portal/html_portal_element.cc

@@ -279,6 +279,13 @@ ScriptPromise HTMLPortalElement::activate(ScriptState* script_state,
         "Cannot activate a portal that is inside another portal.");
     return ScriptPromise();
   }
+  if (GetDocument().BeforeUnloadStarted()) {
+    exception_state.ThrowDOMException(
+        DOMExceptionCode::kInvalidStateError,
+        "Cannot activate portal while document is in beforeunload or has "
+        "started unloading.");
+    return ScriptPromise();
+  }
 
   BlinkTransferableMessage data =
       ActivateDataAsMessage(script_state, options, exception_state);

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -1407,6 +1407,15 @@ bool FrameLoader::ShouldClose(bool is_reload) {
     }
   }
 
+  // Now that none of the unloading frames canceled the BeforeUnload, tell each
+  // of them so they can advance to the appropriate load state.
+  frame_->GetDocument()->BeforeUnloadDoneWillUnload();
+  for (Member<LocalFrame>& descendant_frame : descendant_frames) {
+    if (!descendant_frame->Tree().IsDescendantOf(frame_))
+      continue;
+    descendant_frame->GetDocument()->BeforeUnloadDoneWillUnload();
+  }
+
   return true;
 }
 

third_party/blink/renderer/core/loader/frame_loader_test.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
+#include "third_party/blink/renderer/core/frame/web_frame_widget_base.h"
+#include "third_party/blink/renderer/core/html/html_anchor_element.h"
+#include "third_party/blink/renderer/core/page/chrome_client_impl.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_request.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_test.h"
+#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
+#include "third_party/blink/renderer/platform/testing/url_test_helpers.h"
+
+namespace blink {
+
+class FrameLoaderSimTest : public SimTest {
+ public:
+  FrameLoaderSimTest() = default;
+
+  void SetUp() override {
+    SimTest::SetUp();
+    WebView().MainFrameWidgetBase()->UpdateAllLifecyclePhases(
+        DocumentUpdateReason::kTest);
+  }
+};
+
+// Ensure that the load event progress is progressed through BeforeUnload only
+// if the event is uncanceled.
+TEST_F(FrameLoaderSimTest, LoadEventProgressBeforeUnloadCanceled) {
+  SimRequest request("https://example.com/test.html", "text/html");
+  SimRequest request_a("https://example.com/subframe-a.html", "text/html");
+  SimRequest request_b("https://example.com/subframe-b.html", "text/html");
+  SimRequest request_c("https://example.com/subframe-c.html", "text/html");
+  SimRequest request_unload("https://example.com/next-page.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  request.Complete(R"HTML(
+      <!DOCTYPE html>
+      <iframe src="subframe-a.html"></iframe>
+  )HTML");
+
+  request_a.Complete(R"HTML(
+      <!DOCTYPE html>
+      <iframe src="subframe-b.html"></iframe>
+      <a id="link" href="next-page.html">Next Page</a>
+  )HTML");
+  request_b.Complete(R"HTML(
+      <!DOCTYPE html>
+      <script>
+        window.onbeforeunload = (e) => {
+          e.returnValue = '';
+          e.preventDefault();
+        };
+      </script>
+      <iframe src="subframe-c.html"></iframe>
+  )HTML");
+  request_c.Complete(R"HTML(
+      <!DOCTYPE html>
+  )HTML");
+  Compositor().BeginFrame();
+
+  auto* main_frame = To<LocalFrame>(GetDocument().GetPage()->MainFrame());
+  auto* frame_a = To<LocalFrame>(main_frame->Tree().FirstChild());
+  auto* frame_b = To<LocalFrame>(frame_a->Tree().FirstChild());
+  auto* frame_c = To<LocalFrame>(frame_b->Tree().FirstChild());
+
+  ASSERT_FALSE(main_frame->GetDocument()->BeforeUnloadStarted());
+  ASSERT_FALSE(frame_a->GetDocument()->BeforeUnloadStarted());
+  ASSERT_FALSE(frame_b->GetDocument()->BeforeUnloadStarted());
+  ASSERT_FALSE(frame_c->GetDocument()->BeforeUnloadStarted());
+
+  // We'll only allow canceling a beforeunload if there's a sticky user
+  // activation present so simulate a user gesture.
+  frame_b->NotifyUserActivationInLocalTree();
+
+  auto& chrome_client =
+      To<ChromeClientImpl>(WebView().GetPage()->GetChromeClient());
+
+  // Simulate the user canceling the navigation away. Since the navigation was
+  // "canceled", we expect that each of the frames should remain in their state
+  // before the beforeunload was dispatched.
+  {
+    chrome_client.SetBeforeUnloadConfirmPanelResultForTesting(false);
+
+    // Note: We can't perform a navigation to check this because the
+    // beforeunload event is dispatched from content's RenderFrameImpl, Blink
+    // tests mock this out using a WebFrameTestProxy which doesn't check
+    // beforeunload before navigating.
+    ASSERT_FALSE(frame_a->Loader().ShouldClose());
+
+    EXPECT_FALSE(main_frame->GetDocument()->BeforeUnloadStarted());
+    EXPECT_FALSE(frame_a->GetDocument()->BeforeUnloadStarted());
+    EXPECT_FALSE(frame_b->GetDocument()->BeforeUnloadStarted());
+    EXPECT_FALSE(frame_c->GetDocument()->BeforeUnloadStarted());
+  }
+
+  // Now test the opposite, the user allowing the navigation away.
+  {
+    chrome_client.SetBeforeUnloadConfirmPanelResultForTesting(true);
+    ASSERT_TRUE(frame_a->Loader().ShouldClose());
+
+    // The navigation was in frame a so it shouldn't affect the parent.
+    EXPECT_FALSE(main_frame->GetDocument()->BeforeUnloadStarted());
+    EXPECT_TRUE(frame_a->GetDocument()->BeforeUnloadStarted());
+    EXPECT_TRUE(frame_b->GetDocument()->BeforeUnloadStarted());
+    EXPECT_TRUE(frame_c->GetDocument()->BeforeUnloadStarted());
+  }
+}
+
+}  // namespace blink

third_party/blink/renderer/core/page/chrome_client_impl.cc

@@ -352,12 +352,23 @@ bool ChromeClientImpl::CanOpenBeforeUnloadConfirmPanel() {
 bool ChromeClientImpl::OpenBeforeUnloadConfirmPanelDelegate(LocalFrame* frame,
                                                             bool is_reload) {
   NotifyPopupOpeningObservers();
+
+  if (before_unload_confirm_panel_result_for_testing_.has_value()) {
+    bool success = before_unload_confirm_panel_result_for_testing_.value();
+    before_unload_confirm_panel_result_for_testing_.reset();
+    return success;
+  }
   bool success = false;
   // Synchronous mojo call.
   frame->GetLocalFrameHostRemote().RunBeforeUnloadConfirm(is_reload, &success);
   return success;
 }
 
+void ChromeClientImpl::SetBeforeUnloadConfirmPanelResultForTesting(
+    bool result) {
+  before_unload_confirm_panel_result_for_testing_ = result;
+}
+
 void ChromeClientImpl::CloseWindowSoon() {
   if (web_view_->Client())
     web_view_->Client()->CloseWindowSoon();

third_party/blink/renderer/core/page/chrome_client_impl.h

@@ -116,6 +116,10 @@ class CORE_EXPORT ChromeClientImpl final : public ChromeClient {
   bool CanOpenBeforeUnloadConfirmPanel() override;
   bool OpenBeforeUnloadConfirmPanelDelegate(LocalFrame*,
                                             bool is_reload) override;
+  // Used in tests to set a mock value for a before unload confirmation dialog
+  // box. The value is cleared after being read.
+  void SetBeforeUnloadConfirmPanelResultForTesting(bool result_success);
+
   void CloseWindowSoon() override;
   bool OpenJavaScriptAlertDelegate(LocalFrame*, const String&) override;
   bool OpenJavaScriptConfirmDelegate(LocalFrame*, const String&) override;
@@ -302,6 +306,7 @@ class CORE_EXPORT ChromeClientImpl final : public ChromeClient {
   bool cursor_overridden_;
   Member<ExternalDateTimeChooser> external_date_time_chooser_;
   bool did_request_non_empty_tool_tip_;
+  base::Optional<bool> before_unload_confirm_panel_result_for_testing_;
 
   FRIEND_TEST_ALL_PREFIXES(FileChooserQueueTest, DerefQueuedChooser);
 };

third_party/blink/web_tests/external/wpt/portals/portals-activate-while-unloading.html

+<!DOCTYPE html>
+<html>
+  <head>
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+    <script src="/resources/testdriver.js"></script>
+    <script src="/resources/testdriver-vendor.js"></script>
+    <script>
+      function childReady() {
+        return new Promise((resolve) => {
+          window.onmessage = resolve;
+        });
+      }
+
+      const handlers = ['beforeunload', 'pagehide', 'unload'];
+      for (let handler of handlers) {
+        promise_test(async (test) => {
+          let popup;
+
+          // Open a popup that has a portal, wait for both to be loaded.
+          {
+            await test_driver.bless('Open a popup', () => {
+              popup = open(`resources/portal-activate-in-handler.html?${handler}`,
+                           '_blank');
+            });
+            await childReady();
+          }
+
+          // We need the exception type below to ensure the activate() call
+          // throws but the popup global may be gone by then so stash it here.
+          const exception_type = popup.DOMException;
+
+          // Navigate the popup away.
+          const cur_path = popup.location.pathname;
+          popup.location = 'resources/blank-host.html';
+
+          // We need to wait until the handler is called but because of the
+          // nature of these handlers, we can't reliably communicate with the
+          // popup while they're running so we use a promise established
+          // earlier to wait until a time we know the portal has been activated
+          // and the returned promise stored on this global.
+          await window.handler_called_promise;
+          assert_not_equals(typeof(window.portal_promise), 'undefined',
+                            'Portal.activate() must be called');
+
+          // The popup should have called activate from the handler, and placed
+          // the promise returned from that call into this window in the
+          // |portal_promise| variable. We expect that this call should reject,
+          // however, if it does activate, it's timing dependent whether the
+          // handler will be run to completion so we may never fulfil the
+          // promise. In that case timeout and fail the test.
+          {
+            test.step_timeout(() => {
+              assert_unreached('Activation didn\'t fulfil.');
+            }, 3000);
+
+            await promise_rejects_dom(test,
+                                      "InvalidStateError",
+                                      exception_type,
+                                      window.portal_promise,
+                                      "Portal activation must fail.");
+          }
+          popup.close();
+        }, `cannot activate portal from ${handler}`);
+      }
+    </script>
+  </head>
+  <body>
+  </body>
+</html>

third_party/blink/web_tests/external/wpt/portals/resources/portal-activate-in-handler.html

+<!DOCTYPE html>
+<html>
+  <head>
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+    <script src="/resources/testdriver.js"></script>
+    <script src="/resources/testdriver-vendor.js"></script>
+  </head>
+  <body>
+  </body>
+  <script>
+    // This page is reused with a different query parameter indicating which
+    // handler to register and activate a portal from.
+    const handler_name = window.location.search.substring(1);
+
+    const portal_element = document.createElement('portal');
+    portal_element.src = 'simple-portal.html';
+    document.body.appendChild(portal_element);
+
+    let page_loaded = false;
+    let portal_loaded = false;
+
+    function notifyReady() {
+      if (page_loaded && portal_loaded) {
+        window.opener.postMessage('done', '*');
+      }
+    }
+
+    portal_element.addEventListener('load', () => {
+      portal_loaded = true;
+      notifyReady();
+    });
+
+    window.addEventListener('load', () => {
+      page_loaded = true;
+      notifyReady();
+    });
+
+    // This will be used to let the parent page know the handler has run and
+    // |portal_promise| is now valid.
+    window.opener.handler_called_promise = new Promise((resolve) => {
+      window.addEventListener(handler_name, () => {
+        window.opener.portal_promise = portal_element.activate();
+
+        // Let the parent page know it can now look at |portal_promise|.
+        resolve();
+      }, {once: true});
+    });
+
+  </script>
+</html>