[reland] 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).

This is a reland because of a use-after-free. In particular, when
NavigationThrottle calls out to DoNavigate, navigationThrottle may
be deleted. The code was not set up to handle that.

BUG=1070569
TEST=NavigationBrowserTest.NavigateFromRendererInitiatedNavigation

Change-Id: I769ef0e0143d333e2fddbd997be2dc9de7aba3bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2154592Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760102}

weblayer/browser/navigation_browsertest.cc

@@ -32,7 +32,7 @@ class NavigationObserverImpl : public NavigationObserver {
   }
   ~NavigationObserverImpl() override { controller_->RemoveObserver(this); }
 
-  using Callback = base::OnceCallback<void(Navigation*)>;
+  using Callback = base::RepeatingCallback<void(Navigation*)>;
 
   void SetStartedCallback(Callback callback) {
     started_callback_ = std::move(callback);
@@ -42,29 +42,38 @@ class NavigationObserverImpl : public NavigationObserver {
     redirected_callback_ = std::move(callback);
   }
 
-  void SetFailedClosure(base::OnceClosure closure) {
+  void SetFailedClosure(base::RepeatingClosure closure) {
     failed_closure_ = std::move(closure);
   }
 
+  void SetCompletedClosure(Callback closure) {
+    completed_closure_ = std::move(closure);
+  }
+
   // NavigationObserver:
   void NavigationStarted(Navigation* navigation) override {
     if (started_callback_)
-      std::move(started_callback_).Run(navigation);
+      started_callback_.Run(navigation);
   }
   void NavigationRedirected(Navigation* navigation) override {
     if (redirected_callback_)
-      std::move(redirected_callback_).Run(navigation);
+      redirected_callback_.Run(navigation);
+  }
+  void NavigationCompleted(Navigation* navigation) override {
+    if (completed_closure_)
+      completed_closure_.Run(navigation);
   }
   void NavigationFailed(Navigation* navigation) override {
     if (failed_closure_)
-      std::move(failed_closure_).Run();
+      failed_closure_.Run();
   }
 
  private:
   NavigationController* controller_;
   Callback started_callback_;
   Callback redirected_callback_;
-  base::OnceClosure failed_closure_;
+  Callback completed_closure_;
+  base::RepeatingClosure failed_closure_;
 };
 
 class OneShotNavigationObserver : public NavigationObserver {
@@ -208,7 +217,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnStart) {
   observer.SetStartedCallback(base::BindLambdaForTesting(
       [&](Navigation*) { GetNavigationController()->Stop(); }));
   observer.SetFailedClosure(
-      base::BindOnce(&base::RunLoop::Quit, base::Unretained(&run_loop)));
+      base::BindRepeating(&base::RunLoop::Quit, base::Unretained(&run_loop)));
   GetNavigationController()->Navigate(
       embedded_test_server()->GetURL("/simple_page.html"));
 
@@ -222,7 +231,7 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnRedirect) {
   observer.SetRedirectedCallback(base::BindLambdaForTesting(
       [&](Navigation*) { GetNavigationController()->Stop(); }));
   observer.SetFailedClosure(
-      base::BindOnce(&base::RunLoop::Quit, base::Unretained(&run_loop)));
+      base::BindRepeating(&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()));
@@ -230,6 +239,36 @@ 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,25 +29,19 @@ using base::android::ScopedJavaLocalRef;
 
 namespace weblayer {
 
-// 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.
+// 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.
 //
-// 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().
+// 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.
 class NavigationControllerImpl::NavigationThrottleImpl
     : public content::NavigationThrottle {
  public:
@@ -59,10 +53,18 @@ 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 {
-    return should_cancel_ ? CANCEL : PROCEED;
+    const bool should_cancel = should_cancel_;
+    if (load_params_)
+      controller_->DoNavigate(std::move(load_params_));
+    // WARNING: this may have been deleted.
+    return should_cancel ? CANCEL : PROCEED;
   }
 
   ThrottleCheckResult WillRedirectRequest() override {
@@ -77,6 +79,7 @@ class NavigationControllerImpl::NavigationThrottleImpl
  private:
   NavigationControllerImpl* controller_;
   bool should_cancel_ = false;
+  std::unique_ptr<content::NavigationController::LoadURLParams> load_params_;
 };
 
 NavigationControllerImpl::NavigationControllerImpl(TabImpl* tab)
@@ -95,6 +98,9 @@ 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;
 }
 
@@ -122,9 +128,9 @@ void NavigationControllerImpl::NavigateWithParams(
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& url,
     jboolean should_replace_current_entry) {
-  content::NavigationController::LoadURLParams params(
+  auto params = std::make_unique<content::NavigationController::LoadURLParams>(
       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));
 }
 
@@ -178,14 +184,16 @@ void NavigationControllerImpl::RemoveObserver(NavigationObserver* observer) {
 }
 
 void NavigationControllerImpl::Navigate(const GURL& url) {
-  DoNavigate(content::NavigationController::LoadURLParams(url));
+  DoNavigate(
+      std::make_unique<content::NavigationController::LoadURLParams>(url));
 }
 
 void NavigationControllerImpl::Navigate(
     const GURL& url,
     const NavigationController::NavigateParams& params) {
-  content::NavigationController::LoadURLParams load_params(url);
-  load_params.should_replace_current_entry =
+  auto load_params =
+      std::make_unique<content::NavigationController::LoadURLParams>(url);
+  load_params->should_replace_current_entry =
       params.should_replace_current_entry;
   DoNavigate(std::move(load_params));
 }
@@ -391,10 +399,18 @@ void NavigationControllerImpl::NotifyLoadStateChanged() {
 }
 
 void NavigationControllerImpl::DoNavigate(
-    content::NavigationController::LoadURLParams&& params) {
-  params.transition_type = ui::PageTransitionFromInt(
+    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(
       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,6 +7,7 @@
 
 #include <map>
 
+#include <memory>
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "build/build_config.h"
@@ -130,7 +131,8 @@ class NavigationControllerImpl : public NavigationController,
 
   void NotifyLoadStateChanged();
 
-  void DoNavigate(content::NavigationController::LoadURLParams&& params);
+  void DoNavigate(
+      std::unique_ptr<content::NavigationController::LoadURLParams> params);
 
   base::ObserverList<NavigationObserver>::Unchecked observers_;
   std::map<content::NavigationHandle*, std::unique_ptr<NavigationImpl>>

weblayer/browser/navigation_impl.cc

@@ -33,6 +33,16 @@ 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,8 +5,11 @@
 #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)
@@ -35,6 +38,11 @@ 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,
@@ -99,6 +107,13 @@ 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>