Revert "weblayer: allow calling Navigate() from onNavigationStarted"

This reverts commit c462e4b7.

Reason for revert: Causes a heap-use-after-free on win-asan. First failure: https://ci.chromium.org/p/chromium/builders/ci/win-asan/11631

Original change's description:
> weblayer: allow calling Navigate() from onNavigationStarted
> 
> At the time OnNavigationStarted() is called it's not safe to call
> Navigate(). This patch delays the navigate until it is safe (when
> NavigationThrottleImpl::WillStartRequest() is called).
> 
> BUG=1070569
> TEST=NavigationBrowserTest.NavigateFromRendererInitiatedNavigation
> 
> Change-Id: Ia110bb94e64d6551c9db302ae41b735ad5a8a5e5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2153758
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Scott Violet <sky@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#759872}

TBR=sky@chromium.org,jam@chromium.org

Change-Id: I67a8b8d121ea1223650ee35a013068da20de6fb9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1070569
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2154348Reviewed-by: Meredith Lane <meredithl@chromium.org>
Commit-Queue: Meredith Lane <meredithl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759943}

weblayer/browser/navigation_browsertest.cc

@@ -32,7 +32,7 @@ class NavigationObserverImpl : public NavigationObserver {
   }
   ~NavigationObserverImpl() override { controller_->RemoveObserver(this); }
 
-  using Callback = base::RepeatingCallback<void(Navigation*)>;
+  using Callback = base::OnceCallback<void(Navigation*)>;
 
   void SetStartedCallback(Callback callback) {
     started_callback_ = std::move(callback);
@@ -42,38 +42,29 @@ class NavigationObserverImpl : public NavigationObserver {
     redirected_callback_ = std::move(callback);
   }
 
-  void SetFailedClosure(base::RepeatingClosure closure) {
+  void SetFailedClosure(base::OnceClosure closure) {
     failed_closure_ = std::move(closure);
   }
 
-  void SetCompletedClosure(Callback closure) {
-    completed_closure_ = std::move(closure);
-  }
-
   // NavigationObserver:
   void NavigationStarted(Navigation* navigation) override {
     if (started_callback_)
-      started_callback_.Run(navigation);
+      std::move(started_callback_).Run(navigation);
   }
   void NavigationRedirected(Navigation* navigation) override {
     if (redirected_callback_)
-      redirected_callback_.Run(navigation);
-  }
-  void NavigationCompleted(Navigation* navigation) override {
-    if (completed_closure_)
-      completed_closure_.Run(navigation);
+      std::move(redirected_callback_).Run(navigation);
   }
   void NavigationFailed(Navigation* navigation) override {
     if (failed_closure_)
-      failed_closure_.Run();
+      std::move(failed_closure_).Run();
   }
 
  private:
   NavigationController* controller_;
   Callback started_callback_;
   Callback redirected_callback_;
-  Callback completed_closure_;
-  base::RepeatingClosure failed_closure_;
+  base::OnceClosure failed_closure_;
 };
 
 class OneShotNavigationObserver : public NavigationObserver {
@@ -217,7 +208,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnStart) {
   observer.SetStartedCallback(base::BindLambdaForTesting(
       [&](Navigation*) { GetNavigationController()->Stop(); }));
   observer.SetFailedClosure(
-      base::BindRepeating(&base::RunLoop::Quit, base::Unretained(&run_loop)));
+      base::BindOnce(&base::RunLoop::Quit, base::Unretained(&run_loop)));
   GetNavigationController()->Navigate(
       embedded_test_server()->GetURL("/simple_page.html"));
 
@@ -231,7 +222,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnRedirect) {
   observer.SetRedirectedCallback(base::BindLambdaForTesting(
       [&](Navigation*) { GetNavigationController()->Stop(); }));
   observer.SetFailedClosure(
-      base::BindRepeating(&base::RunLoop::Quit, base::Unretained(&run_loop)));
+      base::BindOnce(&base::RunLoop::Quit, base::Unretained(&run_loop)));
   const GURL original_url = embedded_test_server()->GetURL("/simple_page.html");
   GetNavigationController()->Navigate(embedded_test_server()->GetURL(
       "/server-redirect?" + original_url.spec()));
@@ -239,36 +230,6 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnRedirect) {
   run_loop.Run();
 }
 
-IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
-                       NavigateFromRendererInitiatedNavigation) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-  NavigationController* controller = shell()->tab()->GetNavigationController();
-  const GURL final_url = embedded_test_server()->GetURL("/simple_page2.html");
-  int failed_count = 0;
-  int completed_count = 0;
-  NavigationObserverImpl observer(controller);
-  base::RunLoop run_loop;
-  observer.SetFailedClosure(
-      base::BindLambdaForTesting([&]() { failed_count++; }));
-  observer.SetCompletedClosure(
-      base::BindLambdaForTesting([&](Navigation* navigation) {
-        completed_count++;
-        if (navigation->GetURL().path() == "/simple_page2.html")
-          run_loop.Quit();
-      }));
-  observer.SetStartedCallback(
-      base::BindLambdaForTesting([&](Navigation* navigation) {
-        if (navigation->GetURL().path() == "/simple_page.html")
-          controller->Navigate(final_url);
-      }));
-  controller->Navigate(embedded_test_server()->GetURL("/simple_page4.html"));
-  run_loop.Run();
-  EXPECT_EQ(1, failed_count);
-  EXPECT_EQ(2, completed_count);
-  ASSERT_EQ(2, controller->GetNavigationListSize());
-  EXPECT_EQ(final_url, controller->GetNavigationEntryDisplayURL(1));
-}
-
 IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, SetRequestHeader) {
   net::test_server::ControllableHttpResponse response_1(embedded_test_server(),
                                                         "", true);

weblayer/browser/navigation_controller_impl.cc

@@ -29,19 +29,25 @@ using base::android::ScopedJavaLocalRef;
 
 namespace weblayer {
 
-// NavigationThrottle implementation responsible for delaying certain
-// operations and performing them when safe. This is necessary as content
-// does allow certain operations to be called at certain times. For example,
-// content does not allow calling WebContents::Stop() from
-// WebContentsObserver::DidStartNavigation() (to do so crashes). To work around
-// this NavigationControllerImpl detects these scenarios and delays processing
-// until safe.
+// NavigationThrottle implementation responsible for stopping the navigation in
+// certain situations. In particular, WebContents::Stop() can not be called from
+// WebContentsObserver::DidStartNavigation() or
+// WebContentsObserver::DidRedirectNavigation() (to do so crashes). To work
+// around this NavigationControllerImpl detects if Stop() is called while
+// processing DidStartNavigation() or DidRedirectNavigation() and sets state in
+// NavigationThrottleImpl such that the navigation is canceled.
 //
-// Most of the support for these scenarios is handled by a custom
-// NavigationThrottle.  To make things interesting, the NavigationThrottle is
-// created after some of the scenarios this code wants to handle. As such,
-// NavigationImpl does some amount of caching until the NavigationThrottle is
-// created.
+// The call order for starting is WebContents::DidStartNavigation() followed
+// by NavigationThrottle::WillStartRequest(). To make things interesting,
+// the NavigationThrottle is created *after* DidStartNavigation(). OTOH,
+// the call order for redirect is NavigationThrottle::WillRedirectRequest()
+// followed by WebContentsObserver::DidRedirectNavigation(). To ensure the
+// right call order NavigationControllerImpl does nothing in
+// DidRedirectNavigation(), instead this class calls to the
+// NavigationController and then returns cancel if necessary. While redirect
+// handling is initiated from this class, start must be handled from the
+// WebContentsObserver call. This is necessary as some code paths do *not*
+// call to NavigationThrottle::WillStartRequest().
 class NavigationControllerImpl::NavigationThrottleImpl
     : public content::NavigationThrottle {
  public:
@@ -53,15 +59,9 @@ class NavigationControllerImpl::NavigationThrottleImpl
   ~NavigationThrottleImpl() override = default;
 
   void ScheduleCancel() { should_cancel_ = true; }
-  void ScheduleNavigate(
-      std::unique_ptr<content::NavigationController::LoadURLParams> params) {
-    load_params_ = std::move(params);
-  }
 
   // content::NavigationThrottle:
   ThrottleCheckResult WillStartRequest() override {
-    if (load_params_)
-      controller_->DoNavigate(std::move(load_params_));
     return should_cancel_ ? CANCEL : PROCEED;
   }
 
@@ -77,7 +77,6 @@ class NavigationControllerImpl::NavigationThrottleImpl
  private:
   NavigationControllerImpl* controller_;
   bool should_cancel_ = false;
-  std::unique_ptr<content::NavigationController::LoadURLParams> load_params_;
 };
 
 NavigationControllerImpl::NavigationControllerImpl(TabImpl* tab)
@@ -96,9 +95,6 @@ NavigationControllerImpl::CreateNavigationThrottle(
   auto* navigation = navigation_map_[handle].get();
   if (navigation->should_stop_when_throttle_created())
     throttle->ScheduleCancel();
-  auto load_params = navigation->TakeParamsToLoadWhenSafe();
-  if (load_params)
-    throttle->ScheduleNavigate(std::move(load_params));
   return throttle;
 }
 
@@ -126,9 +122,9 @@ void NavigationControllerImpl::NavigateWithParams(
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& url,
     jboolean should_replace_current_entry) {
-  auto params = std::make_unique<content::NavigationController::LoadURLParams>(
+  content::NavigationController::LoadURLParams params(
       GURL(base::android::ConvertJavaStringToUTF8(env, url)));
-  params->should_replace_current_entry = should_replace_current_entry;
+  params.should_replace_current_entry = should_replace_current_entry;
   DoNavigate(std::move(params));
 }
 
@@ -182,16 +178,14 @@ void NavigationControllerImpl::RemoveObserver(NavigationObserver* observer) {
 }
 
 void NavigationControllerImpl::Navigate(const GURL& url) {
-  DoNavigate(
-      std::make_unique<content::NavigationController::LoadURLParams>(url));
+  DoNavigate(content::NavigationController::LoadURLParams(url));
 }
 
 void NavigationControllerImpl::Navigate(
     const GURL& url,
     const NavigationController::NavigateParams& params) {
-  auto load_params =
-      std::make_unique<content::NavigationController::LoadURLParams>(url);
-  load_params->should_replace_current_entry =
+  content::NavigationController::LoadURLParams load_params(url);
+  load_params.should_replace_current_entry =
       params.should_replace_current_entry;
   DoNavigate(std::move(load_params));
 }
@@ -397,18 +391,10 @@ void NavigationControllerImpl::NotifyLoadStateChanged() {
 }
 
 void NavigationControllerImpl::DoNavigate(
-    std::unique_ptr<content::NavigationController::LoadURLParams> params) {
-  if (navigation_starting_) {
-    // DoNavigate() is being called reentrantly. Delay processing until it's
-    // safe.
-    Stop();
-    navigation_starting_->SetParamsToLoadWhenSafe(std::move(params));
-    return;
-  }
-
-  params->transition_type = ui::PageTransitionFromInt(
+    content::NavigationController::LoadURLParams&& params) {
+  params.transition_type = ui::PageTransitionFromInt(
       ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
-  web_contents()->GetController().LoadURLWithParams(*params);
+  web_contents()->GetController().LoadURLWithParams(params);
   // So that if the user had entered the UI in a bar it stops flashing the
   // caret.
   web_contents()->Focus();

weblayer/browser/navigation_controller_impl.h

@@ -7,7 +7,6 @@
 
 #include <map>
 
-#include <memory>
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "build/build_config.h"
@@ -131,8 +130,7 @@ class NavigationControllerImpl : public NavigationController,
 
   void NotifyLoadStateChanged();
 
-  void DoNavigate(
-      std::unique_ptr<content::NavigationController::LoadURLParams> params);
+  void DoNavigate(content::NavigationController::LoadURLParams&& params);
 
   base::ObserverList<NavigationObserver>::Unchecked observers_;
   std::map<content::NavigationHandle*, std::unique_ptr<NavigationImpl>>

weblayer/browser/navigation_impl.cc

@@ -33,16 +33,6 @@ NavigationImpl::~NavigationImpl() {
 #endif
 }
 
-void NavigationImpl::SetParamsToLoadWhenSafe(
-    std::unique_ptr<content::NavigationController::LoadURLParams> params) {
-  scheduled_load_params_ = std::move(params);
-}
-
-std::unique_ptr<content::NavigationController::LoadURLParams>
-NavigationImpl::TakeParamsToLoadWhenSafe() {
-  return std::move(scheduled_load_params_);
-}
-
 #if defined(OS_ANDROID)
 void NavigationImpl::SetJavaNavigation(
     JNIEnv* env,

weblayer/browser/navigation_impl.h

@@ -5,11 +5,8 @@
 #ifndef WEBLAYER_BROWSER_NAVIGATION_IMPL_H_
 #define WEBLAYER_BROWSER_NAVIGATION_IMPL_H_
 
-#include <memory>
 #include "base/macros.h"
-#include "base/optional.h"
 #include "build/build_config.h"
-#include "content/public/browser/navigation_controller.h"
 #include "weblayer/public/navigation.h"
 
 #if defined(OS_ANDROID)
@@ -38,11 +35,6 @@ class NavigationImpl : public Navigation {
     safe_to_set_request_headers_ = value;
   }
 
-  void SetParamsToLoadWhenSafe(
-      std::unique_ptr<content::NavigationController::LoadURLParams> params);
-  std::unique_ptr<content::NavigationController::LoadURLParams>
-  TakeParamsToLoadWhenSafe();
-
 #if defined(OS_ANDROID)
   void SetJavaNavigation(
       JNIEnv* env,
@@ -107,13 +99,6 @@ class NavigationImpl : public Navigation {
   base::android::ScopedJavaGlobalRef<jobject> java_navigation_;
 #endif
 
-  // Used to delay loading until safe. In particular, if Navigate() is called
-  // from NavigationStarted(), then the parameters are captured and the
-  // navigation started later on. The delaying is necessary as content is not
-  // reentrant, and this triggers some amount of reentrancy.
-  std::unique_ptr<content::NavigationController::LoadURLParams>
-      scheduled_load_params_;
-
   DISALLOW_COPY_AND_ASSIGN(NavigationImpl);
 };
 

weblayer/test/data/simple_page4.html

-<html>
-<head><title>OK</title></head>
-<body>
-      <meta http-equiv = "refresh" content = "2; url = /simple_page.html" />
-</body>
-</html>