Revert "Add another BeforeUnloadType in support of discarding."

This reverts commit e200b3b1.

Reason for revert: This CL seems to break tests. See issue 883160 for details

Original change's description:
> Add another BeforeUnloadType in support of discarding.
> 
> In order to not discard sites with potential unsaved user state it is necessary
> to run beforeunload handlers to see if they return the empty string or not. When running
> handlers in this context it is undesirable to launch a modal dialog, but rather to
> silently block the discard and continue running the page.
> 
> This CL adds a new BeforeUnloadType to RenderFrameHostImpl in support of this. A
> follow-up change will expose functionality for invoking DispatchBeforeUnload with
> this BeforeUnloadType, and returning the results of the unload via a callback.
> 
> BUG=877550
> 
> Change-Id: Iad029fb0f70f9f44695f15c5ca0c0f6e4ed7ff78
> Reviewed-on: https://chromium-review.googlesource.com/1208962
> Commit-Queue: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#590362}

TBR=chrisha@chromium.org,alexmos@chromium.org

Change-Id: If974ab384c3fdda13ca8e3eb89940d5a4026fa68
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 877550, 883160
Reviewed-on: https://chromium-review.googlesource.com/1220408Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590575}

content/browser/frame_host/render_frame_host_impl.cc

@@ -564,7 +564,6 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
       is_waiting_for_swapout_ack_(false),
       render_frame_created_(false),
       is_waiting_for_beforeunload_ack_(false),
-      beforeunload_dialog_request_cancels_unload_(false),
       unload_ack_is_for_navigation_(false),
       beforeunload_timeout_delay_(base::TimeDelta::FromMilliseconds(
           RenderViewHostImpl::kUnloadTimeoutMS)),
@@ -2319,15 +2318,6 @@ void RenderFrameHostImpl::OnRunBeforeUnloadConfirm(
   // Allow at most one attempt to show a beforeunload dialog per navigation.
   RenderFrameHostImpl* beforeunload_initiator = GetBeforeUnloadInitiator();
   if (beforeunload_initiator) {
-    // If the running beforeunload handler wants to display a dialog and the
-    // before-unload type wants to ignore it, then short-circuit the request and
-    // respond as if the user decided to stay on the page, canceling the unload.
-    if (beforeunload_initiator->beforeunload_dialog_request_cancels_unload_) {
-      SendJavaScriptDialogReply(reply_msg, false /* success */,
-                                base::string16());
-      return;
-    }
-
     if (beforeunload_initiator->has_shown_beforeunload_dialog_) {
       // TODO(alexmos): Pass enough data back to renderer to record histograms
       // for Document.BeforeUnloadDialog and add the intervention console
@@ -3702,9 +3692,8 @@ void RenderFrameHostImpl::DispatchBeforeUnload(BeforeUnloadType type,
       type == BeforeUnloadType::RENDERER_INITIATED_NAVIGATION;
   DCHECK(for_navigation || !is_reload);
 
-  // TAB_CLOSE and DISCARD should only dispatch beforeunload on main frames.
-  DCHECK(type == BeforeUnloadType::BROWSER_INITIATED_NAVIGATION ||
-         type == BeforeUnloadType::RENDERER_INITIATED_NAVIGATION ||
+  // Tab close should only dispatch beforeunload on main frames.
+  DCHECK(type != BeforeUnloadType::TAB_CLOSE ||
          frame_tree_node_->IsMainFrame());
 
   if (!for_navigation) {
@@ -3761,16 +3750,13 @@ void RenderFrameHostImpl::DispatchBeforeUnload(BeforeUnloadType type,
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // handler.
     is_waiting_for_beforeunload_ack_ = true;
-    beforeunload_dialog_request_cancels_unload_ = false;
     unload_ack_is_for_navigation_ = for_navigation;
     send_before_unload_start_time_ = base::TimeTicks::Now();
     if (render_view_host_->GetDelegate()->IsJavaScriptDialogShowing()) {
       // If there is a JavaScript dialog up, don't bother sending the renderer
       // the unload event because it is known unresponsive, waiting for the
-      // reply from the dialog. If this incoming request is for a DISCARD be
-      // sure to reply with |proceed = false|, because the presence of a dialog
-      // indicates that the page can't be discarded.
-      SimulateBeforeUnloadAck(type != BeforeUnloadType::DISCARD);
+      // reply from the dialog.
+      SimulateBeforeUnloadAck();
     } else {
       // Start a timer that will be shared by all frames that need to run
       // beforeunload in the current frame's subtree.
@@ -3778,8 +3764,6 @@ void RenderFrameHostImpl::DispatchBeforeUnload(BeforeUnloadType type,
         beforeunload_timeout_->Start(beforeunload_timeout_delay_);
 
       beforeunload_pending_replies_.clear();
-      beforeunload_dialog_request_cancels_unload_ =
-          (type == BeforeUnloadType::DISCARD);
 
       // Run beforeunload in this frame and its cross-process descendant
       // frames, in parallel.
@@ -3872,7 +3856,7 @@ bool RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForSubtree(
   return found_beforeunload;
 }
 
-void RenderFrameHostImpl::SimulateBeforeUnloadAck(bool proceed) {
+void RenderFrameHostImpl::SimulateBeforeUnloadAck() {
   DCHECK(is_waiting_for_beforeunload_ack_);
   base::TimeTicks approx_renderer_start_time = send_before_unload_start_time_;
 
@@ -3880,7 +3864,7 @@ void RenderFrameHostImpl::SimulateBeforeUnloadAck(bool proceed) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&RenderFrameHostImpl::ProcessBeforeUnloadACK,
-                     weak_ptr_factory_.GetWeakPtr(), proceed,
+                     weak_ptr_factory_.GetWeakPtr(), true /* proceed */,
                      true /* treat_as_final_ack */, approx_renderer_start_time,
                      base::TimeTicks::Now()));
 }
@@ -5292,7 +5276,7 @@ void RenderFrameHostImpl::BeforeUnloadTimeout() {
   if (render_view_host_->GetDelegate()->ShouldIgnoreUnresponsiveRenderer())
     return;
 
-  SimulateBeforeUnloadAck(true /* proceed */);
+  SimulateBeforeUnloadAck();
 }
 
 void RenderFrameHostImpl::SetLastCommittedSiteUrl(const GURL& url) {

content/browser/frame_host/render_frame_host_impl.h

@@ -457,12 +457,7 @@ class CONTENT_EXPORT RenderFrameHostImpl
   enum class BeforeUnloadType {
     BROWSER_INITIATED_NAVIGATION,
     RENDERER_INITIATED_NAVIGATION,
-    TAB_CLOSE,
-    // This reason is used before a tab is discarded in order to free up
-    // resources. When this is used and the handler returns a non-empty string,
-    // the confirmation dialog will not be displayed and the discard will
-    // automatically be canceled.
-    DISCARD,
+    TAB_CLOSE
   };
 
   // Runs the beforeunload handler for this frame and its subframes. |type|
@@ -473,7 +468,7 @@ class CONTENT_EXPORT RenderFrameHostImpl
   void DispatchBeforeUnload(BeforeUnloadType type, bool is_reload);
 
   // Simulate beforeunload ack on behalf of renderer if it's unrenresponsive.
-  void SimulateBeforeUnloadAck(bool proceed);
+  void SimulateBeforeUnloadAck();
 
   // Returns true if a call to DispatchBeforeUnload will actually send the
   // BeforeUnload IPC.  This can be called on a main frame or subframe.  If
@@ -1387,11 +1382,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // machine.
   bool is_waiting_for_beforeunload_ack_;
 
-  // Valid only when |is_waiting_for_beforeunload_ack_| is true. This indicates
-  // whether a subsequent request to launch a modal dialog should be honored or
-  // whether it should implicitly cause the unload to be canceled.
-  bool beforeunload_dialog_request_cancels_unload_;
-
   // Valid only when is_waiting_for_beforeunload_ack_ or
   // IsWaitingForUnloadACK is true.  This tells us if the unload request
   // is for closing the entire tab ( = false), or only this RenderFrameHost in

content/browser/frame_host/render_frame_host_impl_browsertest.cc

@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/run_loop.h"
-#include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
@@ -202,22 +201,14 @@ class TestJavaScriptDialogManager : public JavaScriptDialogManager,
       : message_loop_runner_(new MessageLoopRunner), url_invalidate_count_(0) {}
   ~TestJavaScriptDialogManager() override {}
 
-  // This waits until either WCD::BeforeUnloadFired is called (the unload has
-  // been handled) or JSDM::RunJavaScriptDialog/RunBeforeUnloadDialog is called
-  // (a request to display a dialog has been received).
   void Wait() {
     message_loop_runner_->Run();
     message_loop_runner_ = new MessageLoopRunner;
   }
 
-  // Runs the dialog callback.
-  void Run(bool success, const base::string16& user_input) {
-    std::move(callback_).Run(success, user_input);
-  }
+  DialogClosedCallback& callback() { return callback_; }
 
   int num_beforeunload_dialogs_seen() { return num_beforeunload_dialogs_seen_; }
-  int num_beforeunload_fired_seen() { return num_beforeunload_fired_seen_; }
-  bool proceed() { return proceed_; }
 
   // WebContentsDelegate
 
@@ -226,14 +217,6 @@ class TestJavaScriptDialogManager : public JavaScriptDialogManager,
     return this;
   }
 
-  void BeforeUnloadFired(WebContents* tab,
-                         bool proceed,
-                         bool* proceed_to_fire_unload) override {
-    ++num_beforeunload_fired_seen_;
-    proceed_ = proceed;
-    message_loop_runner_->Quit();
-  }
-
   // JavaScriptDialogManager
 
   void RunJavaScriptDialog(WebContents* web_contents,
@@ -242,10 +225,7 @@ class TestJavaScriptDialogManager : public JavaScriptDialogManager,
                            const base::string16& message_text,
                            const base::string16& default_prompt_text,
                            DialogClosedCallback callback,
-                           bool* did_suppress_message) override {
-    callback_ = std::move(callback);
-    message_loop_runner_->Quit();
-  }
+                           bool* did_suppress_message) override {}
 
   void RunBeforeUnloadDialog(WebContents* web_contents,
                              RenderFrameHost* render_frame_host,
@@ -287,13 +267,6 @@ class TestJavaScriptDialogManager : public JavaScriptDialogManager,
   // The total number of beforeunload dialogs seen by this dialog manager.
   int num_beforeunload_dialogs_seen_ = 0;
 
-  // The total number of BeforeUnloadFired events witnessed by the
-  // WebContentsDelegate.
-  int num_beforeunload_fired_seen_ = 0;
-
-  // The |proceed| value returned by the last unload event.
-  bool proceed_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(TestJavaScriptDialogManager);
 };
 
@@ -365,7 +338,7 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
   main_frame->GetProcess()->AddFilter(filter.get());
 
   // Answer the dialog.
-  dialog_manager.Run(true, base::string16());
+  std::move(dialog_manager.callback()).Run(true, base::string16());
 
   // There will be no beforeunload ACK, so if the beforeunload ACK timer isn't
   // functioning then the navigation will hang forever and this test will time
@@ -404,7 +377,7 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
   dialog_manager.Wait();
 
   // Answer the dialog.
-  dialog_manager.Run(true, base::string16());
+  std::move(dialog_manager.callback()).Run(true, base::string16());
   EXPECT_TRUE(WaitForLoadStop(wc));
 
   // The reload should have cleared the user gesture bit, so upon leaving again
@@ -435,7 +408,7 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
 
   // Cancel the dialog.
   dialog_manager.reset_url_invalidate_count();
-  dialog_manager.Run(false, base::string16());
+  std::move(dialog_manager.callback()).Run(false, base::string16());
   EXPECT_FALSE(wc->IsLoading());
 
   // Verify there are no pending history items after the dialog is cancelled.
@@ -473,13 +446,13 @@ class RenderFrameHostImplBeforeUnloadBrowserTest
   }
 
   void CloseDialogAndProceed() {
-    dialog_manager_->Run(true /* navigation should proceed */,
-                         base::string16());
+    std::move(dialog_manager()->callback())
+        .Run(true /* navigation should proceed */, base::string16());
   }
 
   void CloseDialogAndCancel() {
-    dialog_manager_->Run(false /* navigation should proceed */,
-                         base::string16());
+    std::move(dialog_manager()->callback())
+        .Run(false /* navigation should proceed */, base::string16());
   }
 
   // Installs a beforeunload handler in the given frame.
@@ -1899,75 +1872,4 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
   //   EXPECT_EQ(blocked_url, nav_handle_observer.last_committed_url());
 }
 
-IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
-                       BeforeUnloadDialogSuppressedForDiscard) {
-  WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell()->web_contents());
-  TestJavaScriptDialogManager dialog_manager;
-  wc->SetDelegate(&dialog_manager);
-
-  EXPECT_TRUE(NavigateToURL(
-      shell(), GetTestUrl("render_frame_host", "beforeunload.html")));
-  // Disable the hang monitor, otherwise there will be a race between the
-  // beforeunload dialog and the beforeunload hang timer.
-  wc->GetMainFrame()->DisableBeforeUnloadHangMonitorForTesting();
-
-  // Give the page a user gesture so javascript beforeunload works, and then
-  // dispatch a before unload with discard as a reason. This should return
-  // without any dialog being seen.
-  wc->GetMainFrame()->ExecuteJavaScriptWithUserGestureForTests(
-      base::string16());
-  wc->GetMainFrame()->DispatchBeforeUnload(
-      RenderFrameHostImpl::BeforeUnloadType::DISCARD, false);
-  dialog_manager.Wait();
-  EXPECT_EQ(0, dialog_manager.num_beforeunload_dialogs_seen());
-  EXPECT_EQ(1, dialog_manager.num_beforeunload_fired_seen());
-  EXPECT_FALSE(dialog_manager.proceed());
-
-  wc->SetDelegate(nullptr);
-  wc->SetJavaScriptDialogManagerForTesting(nullptr);
-}
-
-IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
-                       PendingDialogMakesDiscardUnloadReturnFalse) {
-  WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell()->web_contents());
-  TestJavaScriptDialogManager dialog_manager;
-  wc->SetDelegate(&dialog_manager);
-
-  EXPECT_TRUE(NavigateToURL(
-      shell(), GetTestUrl("render_frame_host", "beforeunload.html")));
-  // Disable the hang monitor, otherwise there will be a race between the
-  // beforeunload dialog and the beforeunload hang timer.
-  wc->GetMainFrame()->DisableBeforeUnloadHangMonitorForTesting();
-
-  // Give the page a user gesture so javascript beforeunload works, and then
-  // dispatch a before unload with discard as a reason. This should return
-  // without any dialog being seen.
-  wc->GetMainFrame()->ExecuteJavaScriptWithUserGestureForTests(
-      base::string16());
-
-  // Launch an alert javascript dialog. This pending dialog should block a
-  // subsequent discarding before unload request.
-  wc->GetMainFrame()->ExecuteJavaScriptForTests(
-      base::ASCIIToUTF16("setTimeout(function(){alert('hello');}, 10);"));
-  dialog_manager.Wait();
-  EXPECT_EQ(0, dialog_manager.num_beforeunload_dialogs_seen());
-  EXPECT_EQ(0, dialog_manager.num_beforeunload_fired_seen());
-
-  // Dispatch a before unload request while the first is still blocked
-  // on the dialog, and expect it to return false immediately (synchronously).
-  wc->GetMainFrame()->DispatchBeforeUnload(
-      RenderFrameHostImpl::BeforeUnloadType::DISCARD, false);
-  dialog_manager.Wait();
-  EXPECT_EQ(0, dialog_manager.num_beforeunload_dialogs_seen());
-  EXPECT_EQ(1, dialog_manager.num_beforeunload_fired_seen());
-  EXPECT_FALSE(dialog_manager.proceed());
-
-  // Clear the existing javascript dialog so that the associated IPC message
-  // doesn't leak.
-  dialog_manager.Run(true, base::string16());
-
-  wc->SetDelegate(nullptr);
-  wc->SetJavaScriptDialogManagerForTesting(nullptr);
-}
-
 }  // namespace content

content/browser/web_contents/web_contents_impl.h

@@ -1049,10 +1049,6 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
                            BeforeUnloadDialogRequiresGesture);
   FRIEND_TEST_ALL_PREFIXES(RenderFrameHostImplBrowserTest,
                            CancelBeforeUnloadResetsURL);
-  FRIEND_TEST_ALL_PREFIXES(RenderFrameHostImplBrowserTest,
-                           BeforeUnloadDialogSuppressedForDiscard);
-  FRIEND_TEST_ALL_PREFIXES(RenderFrameHostImplBrowserTest,
-                           PendingDialogMakesDiscardUnloadReturnFalse);
   FRIEND_TEST_ALL_PREFIXES(DevToolsProtocolTest, JavaScriptDialogNotifications);
   FRIEND_TEST_ALL_PREFIXES(DevToolsProtocolTest, JavaScriptDialogInterop);
   FRIEND_TEST_ALL_PREFIXES(DevToolsProtocolTest, BeforeUnloadDialog);