weblayer: avoid calling WebContents::Stop() at certain points

Calling WebContents::Stop() from
WebContentsObserver::DidStartNavigation() or
WebContentsObserver::DidRedirectNavigation triggers a crash. This patch
works around this by installing a NavigationThrottle. The
NavigationThrottle cancels the navigation as necessary. Does this
ensures we don't attempt to call Stop() while servicing these functions,
yet ends up with the same result.

BUG=1068115
TEST=Covered by weblayer_browsertests

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

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/NavigationTest.java

@@ -527,6 +527,26 @@ public class NavigationTest {
         return navigationUrl.get();
     }
 
+    @Test
+    @SmallTest
+    public void testStopFromOnNavigationStarted() throws Exception {
+        final InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(null);
+        final BoundedCountDownLatch doneLatch = new BoundedCountDownLatch(1);
+        NavigationCallback navigationCallback = new NavigationCallback() {
+            @Override
+            public void onNavigationStarted(Navigation navigation) {
+                activity.getTab().getNavigationController().stop();
+                doneLatch.countDown();
+            }
+        };
+        runOnUiThreadBlocking(() -> {
+            NavigationController controller = activity.getTab().getNavigationController();
+            controller.registerNavigationCallback(navigationCallback);
+            controller.navigate(Uri.parse(URL1));
+        });
+        doneLatch.timedAwait();
+    }
+
     // NavigationCallback implementation that sets a header in either start or redirect.
     private static final class HeaderSetter extends NavigationCallback {
         private final String mName;

weblayer/browser/content_browser_client_impl.cc

@@ -53,6 +53,7 @@
 #include "weblayer/browser/browser_process.h"
 #include "weblayer/browser/feature_list_creator.h"
 #include "weblayer/browser/i18n_util.h"
+#include "weblayer/browser/navigation_controller_impl.h"
 #include "weblayer/browser/profile_impl.h"
 #include "weblayer/browser/system_network_context_manager.h"
 #include "weblayer/browser/tab_impl.h"
@@ -406,6 +407,18 @@ std::vector<std::unique_ptr<content::NavigationThrottle>>
 ContentBrowserClientImpl::CreateThrottlesForNavigation(
     content::NavigationHandle* handle) {
   std::vector<std::unique_ptr<content::NavigationThrottle>> throttles;
+
+  // This throttle *must* be first as it's responsible for calling to
+  // NavigationController for certain events.
+  TabImpl* tab = TabImpl::FromWebContents(handle->GetWebContents());
+  if (tab) {
+    auto throttle =
+        static_cast<NavigationControllerImpl*>(tab->GetNavigationController())
+            ->CreateNavigationThrottle(handle);
+    if (throttle)
+      throttles.push_back(std::move(throttle));
+  }
+
   throttles.push_back(std::make_unique<SSLErrorNavigationThrottle>(
       handle, std::make_unique<SSLCertReporterImpl>(),
       base::BindOnce(&HandleSSLErrorWrapper), base::BindOnce(&IsInHostedApp)));

weblayer/browser/navigation_browsertest.cc

@@ -20,6 +20,35 @@ namespace weblayer {
 
 namespace {
 
+class StopNavigationObserver : public NavigationObserver {
+ public:
+  StopNavigationObserver(NavigationController* controller, bool stop_in_start)
+      : controller_(controller), stop_in_start_(stop_in_start) {
+    controller_->AddObserver(this);
+  }
+  ~StopNavigationObserver() override { controller_->RemoveObserver(this); }
+
+  void WaitForNavigation() { run_loop_.Run(); }
+
+  // NavigationObserver:
+  void NavigationStarted(Navigation* navigation) override {
+    if (stop_in_start_)
+      controller_->Stop();
+  }
+  void NavigationRedirected(Navigation* navigation) override {
+    if (!stop_in_start_)
+      controller_->Stop();
+  }
+  void NavigationFailed(Navigation* navigation) override { run_loop_.Quit(); }
+
+ private:
+  NavigationController* controller_;
+  // If true Stop() is called in NavigationStarted(), otherwise Stop() is
+  // called in NavigationRedirected().
+  const bool stop_in_start_;
+  base::RunLoop run_loop_;
+};
+
 class OneShotNavigationObserver : public NavigationObserver {
  public:
   explicit OneShotNavigationObserver(Shell* shell) : tab_(shell->tab()) {
@@ -149,6 +178,28 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, HttpConnectivityError) {
   EXPECT_EQ(observer.navigation_state(), NavigationState::kFailed);
 }
 
+IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnStart) {
+  EXPECT_TRUE(embedded_test_server()->Start());
+  StopNavigationObserver observer(shell()->tab()->GetNavigationController(),
+                                  true);
+  shell()->tab()->GetNavigationController()->Navigate(
+      embedded_test_server()->GetURL("/simple_page.html"));
+
+  observer.WaitForNavigation();
+}
+
+IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnRedirect) {
+  EXPECT_TRUE(embedded_test_server()->Start());
+  StopNavigationObserver observer(shell()->tab()->GetNavigationController(),
+                                  false);
+  const GURL original_url = embedded_test_server()->GetURL("/simple_page.html");
+  shell()->tab()->GetNavigationController()->Navigate(
+      embedded_test_server()->GetURL("/server-redirect?" +
+                                     original_url.spec()));
+
+  observer.WaitForNavigation();
+}
+
 namespace {
 
 class HeaderInjectorNavigationObserver : public NavigationObserver {

weblayer/browser/navigation_controller_impl.cc

@@ -4,9 +4,12 @@
 
 #include "weblayer/browser/navigation_controller_impl.h"
 
+#include "base/auto_reset.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/page_transition_types.h"
 #include "weblayer/browser/tab_impl.h"
@@ -26,11 +29,75 @@ 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.
+//
+// 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:
+  NavigationThrottleImpl(NavigationControllerImpl* controller,
+                         content::NavigationHandle* handle)
+      : NavigationThrottle(handle), controller_(controller) {}
+  NavigationThrottleImpl(const NavigationThrottleImpl&) = delete;
+  NavigationThrottleImpl& operator=(const NavigationThrottleImpl&) = delete;
+  ~NavigationThrottleImpl() override = default;
+
+  void ScheduleCancel() { should_cancel_ = true; }
+
+  // content::NavigationThrottle:
+  ThrottleCheckResult WillStartRequest() override {
+    return should_cancel_ ? CANCEL : PROCEED;
+  }
+
+  ThrottleCheckResult WillRedirectRequest() override {
+    controller_->WillRedirectRequest(this, navigation_handle());
+    return should_cancel_ ? CANCEL : PROCEED;
+  }
+
+  const char* GetNameForLogging() override {
+    return "WebLayerNavigationControllerThrottle";
+  }
+
+ private:
+  NavigationControllerImpl* controller_;
+  bool should_cancel_ = false;
+};
+
 NavigationControllerImpl::NavigationControllerImpl(TabImpl* tab)
     : WebContentsObserver(tab->web_contents()) {}
 
 NavigationControllerImpl::~NavigationControllerImpl() = default;
 
+std::unique_ptr<content::NavigationThrottle>
+NavigationControllerImpl::CreateNavigationThrottle(
+    content::NavigationHandle* handle) {
+  if (!handle->IsInMainFrame())
+    return nullptr;
+
+  auto throttle = std::make_unique<NavigationThrottleImpl>(this, handle);
+  DCHECK(navigation_map_.find(handle) != navigation_map_.end());
+  auto* navigation = navigation_map_[handle].get();
+  if (navigation->should_stop_when_throttle_created())
+    throttle->ScheduleCancel();
+  return throttle;
+}
+
 #if defined(OS_ANDROID)
 void NavigationControllerImpl::SetNavigationControllerImpl(
     JNIEnv* env,
@@ -74,6 +141,29 @@ ScopedJavaLocalRef<jstring> NavigationControllerImpl::GetNavigationEntryTitle(
 }
 #endif
 
+void NavigationControllerImpl::WillRedirectRequest(
+    NavigationThrottleImpl* throttle,
+    content::NavigationHandle* navigation_handle) {
+  DCHECK(navigation_handle->IsInMainFrame());
+  DCHECK(navigation_map_.find(navigation_handle) != navigation_map_.end());
+  auto* navigation = navigation_map_[navigation_handle].get();
+  navigation->set_safe_to_set_request_headers(true);
+  DCHECK(!active_throttle_);
+  base::AutoReset<NavigationThrottleImpl*> auto_reset(&active_throttle_,
+                                                      throttle);
+#if defined(OS_ANDROID)
+  if (java_controller_) {
+    TRACE_EVENT0("weblayer",
+                 "Java_NavigationControllerImpl_navigationRedirected");
+    Java_NavigationControllerImpl_navigationRedirected(
+        AttachCurrentThread(), java_controller_, navigation->java_navigation());
+  }
+#endif
+  for (auto& observer : observers_)
+    observer.NavigationRedirected(navigation);
+  navigation->set_safe_to_set_request_headers(false);
+}
+
 void NavigationControllerImpl::AddObserver(NavigationObserver* observer) {
   observers_.AddObserver(observer);
 }
@@ -117,7 +207,12 @@ void NavigationControllerImpl::Reload() {
 }
 
 void NavigationControllerImpl::Stop() {
-  web_contents()->Stop();
+  if (navigation_starting_)
+    navigation_starting_->set_should_stop_when_throttle_created();
+  else if (active_throttle_)
+    active_throttle_->ScheduleCancel();
+  else
+    web_contents()->Stop();
 }
 
 int NavigationControllerImpl::GetNavigationListSize() {
@@ -147,9 +242,15 @@ void NavigationControllerImpl::DidStartNavigation(
   if (!navigation_handle->IsInMainFrame())
     return;
 
+  // This function should not be called reentrantly.
+  DCHECK(!navigation_starting_);
+
+  DCHECK(!base::Contains(navigation_map_, navigation_handle));
   navigation_map_[navigation_handle] =
       std::make_unique<NavigationImpl>(navigation_handle);
   auto* navigation = navigation_map_[navigation_handle].get();
+  base::AutoReset<NavigationImpl*> auto_reset(&navigation_starting_,
+                                              navigation);
   navigation->set_safe_to_set_request_headers(true);
 #if defined(OS_ANDROID)
   if (java_controller_) {
@@ -172,23 +273,9 @@ void NavigationControllerImpl::DidStartNavigation(
 
 void NavigationControllerImpl::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (!navigation_handle->IsInMainFrame())
-    return;
-
-  DCHECK(navigation_map_.find(navigation_handle) != navigation_map_.end());
-  auto* navigation = navigation_map_[navigation_handle].get();
-  navigation->set_safe_to_set_request_headers(true);
-#if defined(OS_ANDROID)
-  if (java_controller_) {
-    TRACE_EVENT0("weblayer",
-                 "Java_NavigationControllerImpl_navigationRedirected");
-    Java_NavigationControllerImpl_navigationRedirected(
-        AttachCurrentThread(), java_controller_, navigation->java_navigation());
-  }
-#endif
-  for (auto& observer : observers_)
-    observer.NavigationRedirected(navigation);
-  navigation->set_safe_to_set_request_headers(false);
+  // NOTE: this implementation should remain empty. Real implementation is in
+  // WillRedirectNavigation(). See description of NavigationThrottleImpl for
+  // more information.
 }
 
 void NavigationControllerImpl::ReadyToCommitNavigation(

weblayer/browser/navigation_controller_impl.h

@@ -19,6 +19,10 @@
 #include "base/android/scoped_java_ref.h"
 #endif
 
+namespace content {
+class NavigationThrottle;
+}
+
 namespace weblayer {
 class TabImpl;
 
@@ -28,6 +32,11 @@ class NavigationControllerImpl : public NavigationController,
   explicit NavigationControllerImpl(TabImpl* tab);
   ~NavigationControllerImpl() override;
 
+  // Creates the NavigationThrottle used to ensure WebContents::Stop() is called
+  // at safe times. See NavigationControllerImpl for details.
+  std::unique_ptr<content::NavigationThrottle> CreateNavigationThrottle(
+      content::NavigationHandle* handle);
+
 #if defined(OS_ANDROID)
   void SetNavigationControllerImpl(
       JNIEnv* env,
@@ -80,6 +89,13 @@ class NavigationControllerImpl : public NavigationController,
 #endif
 
  private:
+  class NavigationThrottleImpl;
+
+  // Called from NavigationControllerImpl::WillRedirectRequest(). See
+  // description of NavigationControllerImpl for details.
+  void WillRedirectRequest(NavigationThrottleImpl* throttle,
+                           content::NavigationHandle* navigation_handle);
+
   // NavigationController implementation:
   void AddObserver(NavigationObserver* observer) override;
   void RemoveObserver(NavigationObserver* observer) override;
@@ -119,6 +135,13 @@ class NavigationControllerImpl : public NavigationController,
   std::map<content::NavigationHandle*, std::unique_ptr<NavigationImpl>>
       navigation_map_;
 
+  // If non-null then processing is inside DidStartNavigation() and
+  // |navigation_starting_| is the NavigationImpl that was created.
+  NavigationImpl* navigation_starting_ = nullptr;
+
+  // Set to non-null while in WillRedirectRequest().
+  NavigationThrottleImpl* active_throttle_ = nullptr;
+
 #if defined(OS_ANDROID)
   base::android::ScopedJavaGlobalRef<jobject> java_controller_;
 #endif

weblayer/browser/navigation_impl.h

@@ -24,6 +24,13 @@ class NavigationImpl : public Navigation {
   explicit NavigationImpl(content::NavigationHandle* navigation_handle);
   ~NavigationImpl() override;
 
+  void set_should_stop_when_throttle_created() {
+    should_stop_when_throttle_created_ = true;
+  }
+  bool should_stop_when_throttle_created() const {
+    return should_stop_when_throttle_created_;
+  }
+
   void set_safe_to_set_request_headers(bool value) {
     safe_to_set_request_headers_ = value;
   }
@@ -81,6 +88,10 @@ class NavigationImpl : public Navigation {
 
   content::NavigationHandle* navigation_handle_;
 
+  // Used to delay calling Stop() until safe. See
+  // NavigationControllerImpl::NavigationThrottleImpl for details.
+  bool should_stop_when_throttle_created_ = false;
+
   // Whether SetRequestHeader() is allowed at this time.
   bool safe_to_set_request_headers_ = false;