weblayer: make it possible to delete tab in OnNavigationFailed...

and OnNavigationCompleted.

Content does not allow WebContents to be deleted from
WebContentsObserver::DidFinishNavigation(). Unfortunately we
keep seeing crashes because of this restriction, so I'm
inclined to provide some mitigation.

This patch takes the approach of detecting this scenario
and delaying deletion of the WebContents. All the surrounding
WebLayer classes are deleted/destroyed and ownership of the
WebContents is passed to the Profile and deleted from a
post-task.

Doing this may introduce it's own set of problems. For example,
if a callback/notification is still in flight from content that gets
processed before the deletion, and the code assumes there is a Tab
associated with the WebContents, then there will be problems.

My preference is still to fix this in content, but in the mean
time this should work.

BUG=1111127
TEST=covered by test

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

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

@@ -827,4 +827,32 @@ public class NavigationTest {
         mActivityTestRule.navigateAndWait(URL2);
         assertFalse(mCallback.onStartedCallback.isPageInitiated());
     }
+
+    /**
+     * This test verifies calling destroyTab() from within onNavigationFailed doesn't crash.
+     */
+    @Test
+    @SmallTest
+    @MinWebLayerVersion(87)
+    public void testDestroyTabInNavigationFailed() throws Throwable {
+        InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(null);
+        CallbackHelper callbackHelper = new CallbackHelper();
+        runOnUiThreadBlocking(() -> {
+            NavigationController navigationController = activity.getTab().getNavigationController();
+            navigationController.registerNavigationCallback(new NavigationCallback() {
+                @Override
+                public void onNavigationFailed(Navigation navigation) {
+                    navigationController.unregisterNavigationCallback(this);
+                    Tab tab = activity.getTab();
+                    tab.getBrowser().destroyTab(tab);
+                    callbackHelper.notifyCalled();
+                }
+            });
+        });
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            activity.getTab().getNavigationController().navigate(
+                    Uri.parse("http://localhost:7/non_existent"));
+        });
+        callbackHelper.waitForFirst();
+    }
 }

weblayer/browser/browser_impl.cc

@@ -271,6 +271,12 @@ void BrowserImpl::SetWebPreferences(content::WebPreferences* prefs) {
 #endif
 }
 
+#if defined(OS_ANDROID)
+void BrowserImpl::DestroyTabFromJava(Tab* tab) {
+  RemoveTab(tab);
+}
+#endif
+
 void BrowserImpl::AddTab(Tab* tab) {
   DCHECK(tab);
   TabImpl* tab_impl = static_cast<TabImpl*>(tab);
@@ -283,7 +289,12 @@ void BrowserImpl::AddTab(Tab* tab) {
 }
 
 void BrowserImpl::DestroyTab(Tab* tab) {
+#if defined(OS_ANDROID)
+  Java_BrowserImpl_destroyTabImpl(AttachCurrentThread(), java_impl_,
+                                  static_cast<TabImpl*>(tab)->GetJavaTab());
+#else
   RemoveTab(tab);
+#endif
 }
 
 void BrowserImpl::SetActiveTab(Tab* tab) {

weblayer/browser/browser_impl.h

@@ -97,6 +97,14 @@ class BrowserImpl : public Browser {
   bool GetPasswordEchoEnabled();
   void SetWebPreferences(content::WebPreferences* prefs);
 
+#if defined(OS_ANDROID)
+  // On Android the Java Tab class owns the C++ Tab. DestroyTab() calls to the
+  // Java Tab class to initiate deletion. This function is called from the Java
+  // side, and must not call DestroyTab(), otherwise we get stuck in infinite
+  // recursion.
+  void DestroyTabFromJava(Tab* tab);
+#endif
+
   // Browser:
   void AddTab(Tab* tab) override;
   void DestroyTab(Tab* tab) override;

weblayer/browser/java/org/chromium/weblayer_private/BrowserImpl.java

@@ -434,6 +434,7 @@ public class BrowserImpl extends IBrowser.Stub implements View.OnAttachStateChan
         destroyTabImpl((TabImpl) iTab);
     }
 
+    @CalledByNative
     private void destroyTabImpl(TabImpl tab) {
         tab.destroy();
     }

weblayer/browser/navigation_browsertest.cc

@@ -14,6 +14,7 @@
 #include "net/test/embedded_test_server/controllable_http_response.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "weblayer/public/browser.h"
 #include "weblayer/public/navigation.h"
 #include "weblayer/public/navigation_controller.h"
 #include "weblayer/public/navigation_observer.h"
@@ -68,8 +69,11 @@ class NavigationObserverImpl : public NavigationObserver {
       completed_callback_.Run(navigation);
   }
   void NavigationFailed(Navigation* navigation) override {
-    if (failed_callback_)
-      failed_callback_.Run(navigation);
+    // As |this| may be deleted when running the callback, the callback must be
+    // copied before running. To do otherwise results in use-after-free.
+    auto callback = failed_callback_;
+    if (callback)
+      callback.Run(navigation);
   }
 
  private:
@@ -259,6 +263,25 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnStart) {
   run_loop.Run();
 }
 
+IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, DestroyTabInNavigation) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  Tab* new_tab = shell()->browser()->CreateTab();
+  base::RunLoop run_loop;
+  std::unique_ptr<NavigationObserverImpl> observer =
+      std::make_unique<NavigationObserverImpl>(
+          new_tab->GetNavigationController());
+  observer->SetFailedCallback(
+      base::BindLambdaForTesting([&](Navigation* navigation) {
+        observer.reset();
+        shell()->browser()->DestroyTab(new_tab);
+        run_loop.Quit();
+      }));
+  new_tab->GetNavigationController()->Navigate(
+      embedded_test_server()->GetURL("/simple_pageX.html"));
+
+  run_loop.Run();
+}
+
 IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, StopInOnRedirect) {
   ASSERT_TRUE(embedded_test_server()->Start());
   base::RunLoop run_loop;

weblayer/browser/navigation_controller_impl.cc

@@ -31,6 +31,29 @@ using base::android::ScopedJavaLocalRef;
 
 namespace weblayer {
 
+class NavigationControllerImpl::DelayDeletionHelper {
+ public:
+  explicit DelayDeletionHelper(NavigationControllerImpl* controller)
+      : controller_(controller->weak_ptr_factory_.GetWeakPtr()) {
+    // This should never be called reentrantly.
+    DCHECK(!controller->should_delay_web_contents_deletion_);
+    controller->should_delay_web_contents_deletion_ = true;
+  }
+
+  DelayDeletionHelper(const DelayDeletionHelper&) = delete;
+  DelayDeletionHelper& operator=(const DelayDeletionHelper&) = delete;
+
+  ~DelayDeletionHelper() {
+    if (controller_)
+      controller_->should_delay_web_contents_deletion_ = false;
+  }
+
+  bool WasControllerDeleted() { return controller_.get() == nullptr; }
+
+ private:
+  base::WeakPtr<NavigationControllerImpl> controller_;
+};
+
 // 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,
@@ -343,6 +366,7 @@ void NavigationControllerImpl::DidFinishNavigation(
   if (!navigation_handle->IsInMainFrame())
     return;
 
+  DelayDeletionHelper deletion_helper(this);
   DCHECK(navigation_map_.find(navigation_handle) != navigation_map_.end());
   auto* navigation = navigation_map_[navigation_handle].get();
   if (navigation_handle->GetNetErrorCode() == net::OK &&
@@ -354,10 +378,15 @@ void NavigationControllerImpl::DidFinishNavigation(
       Java_NavigationControllerImpl_navigationCompleted(
           AttachCurrentThread(), java_controller_,
           navigation->java_navigation());
+      if (deletion_helper.WasControllerDeleted())
+        return;
     }
 #endif
-    for (auto& observer : observers_)
+    for (auto& observer : observers_) {
       observer.NavigationCompleted(navigation);
+      if (deletion_helper.WasControllerDeleted())
+        return;
+    }
   } else {
 #if defined(OS_ANDROID)
     if (java_controller_) {
@@ -366,10 +395,15 @@ void NavigationControllerImpl::DidFinishNavigation(
       Java_NavigationControllerImpl_navigationFailed(
           AttachCurrentThread(), java_controller_,
           navigation->java_navigation());
+      if (deletion_helper.WasControllerDeleted())
+        return;
     }
 #endif
-    for (auto& observer : observers_)
+    for (auto& observer : observers_) {
       observer.NavigationFailed(navigation);
+      if (deletion_helper.WasControllerDeleted())
+        return;
+    }
   }
 
   // Note InsertVisualStateCallback currently does not take into account

weblayer/browser/navigation_controller_impl.h

@@ -73,7 +73,13 @@ class NavigationControllerImpl : public NavigationController,
   bool IsNavigationEntrySkippable(JNIEnv* env, int index);
 #endif
 
+  bool should_delay_web_contents_deletion() {
+    return should_delay_web_contents_deletion_;
+  }
+
  private:
+  class DelayDeletionHelper;
+
   class NavigationThrottleImpl;
 
   // Called from NavigationControllerImpl::WillRedirectRequest(). See
@@ -134,6 +140,11 @@ class NavigationControllerImpl : public NavigationController,
   base::android::ScopedJavaGlobalRef<jobject> java_controller_;
 #endif
 
+  // Set to true while processing an observer/callback and it's unsafe to
+  // delete the WebContents. This is not used for all callbacks, just the
+  // ones that we need to allow deletion from (such as completed/failed).
+  bool should_delay_web_contents_deletion_ = false;
+
   base::WeakPtrFactory<NavigationControllerImpl> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(NavigationControllerImpl);

weblayer/browser/persistence/browser_persister_browsertest.cc

@@ -47,50 +47,6 @@ class BrowserPersisterTestHelper {
 namespace {
 using testing::UnorderedElementsAre;
 
-class OneShotNavigationObserver : public NavigationObserver {
- public:
-  explicit OneShotNavigationObserver(Shell* shell) : tab_(shell->tab()) {
-    tab_->GetNavigationController()->AddObserver(this);
-  }
-
-  ~OneShotNavigationObserver() override {
-    tab_->GetNavigationController()->RemoveObserver(this);
-  }
-
-  void WaitForNavigation() { run_loop_.Run(); }
-
-  bool completed() { return completed_; }
-  bool is_error_page() { return is_error_page_; }
-  Navigation::LoadError load_error() { return load_error_; }
-  int http_status_code() { return http_status_code_; }
-  NavigationState navigation_state() { return navigation_state_; }
-
- private:
-  // NavigationObserver implementation:
-  void NavigationCompleted(Navigation* navigation) override {
-    completed_ = true;
-    Finish(navigation);
-  }
-
-  void NavigationFailed(Navigation* navigation) override { Finish(navigation); }
-
-  void Finish(Navigation* navigation) {
-    is_error_page_ = navigation->IsErrorPage();
-    load_error_ = navigation->GetLoadError();
-    http_status_code_ = navigation->GetHttpStatusCode();
-    navigation_state_ = navigation->GetState();
-    run_loop_.Quit();
-  }
-
-  base::RunLoop run_loop_;
-  Tab* tab_;
-  bool completed_ = false;
-  bool is_error_page_ = false;
-  Navigation::LoadError load_error_ = Navigation::kNoError;
-  int http_status_code_ = 0;
-  NavigationState navigation_state_ = NavigationState::kWaitingResponse;
-};
-
 class BrowserObserverImpl : public BrowserObserver {
  public:
   static void WaitForNewTab(Browser* browser) {

weblayer/browser/profile_impl.cc

@@ -28,6 +28,7 @@
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/storage_partition.h"
+#include "content/public/browser/web_contents.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
@@ -232,6 +233,10 @@ ProfileImpl::ProfileImpl(const std::string& name)
 }
 
 ProfileImpl::~ProfileImpl() {
+  // Destroy any scheduled WebContents. These implicitly refer to the
+  // BrowserContext and must be destroyed before the BrowserContext.
+  web_contents_to_delete_.clear();
+
   if (browser_context_) {
     BrowserContextDependencyManager::GetInstance()
         ->DestroyBrowserContextServices(browser_context_.get());
@@ -260,6 +265,16 @@ void ProfileImpl::RemoveProfileObserver(ProfileObserver* observer) {
   GetObservers().RemoveObserver(observer);
 }
 
+void ProfileImpl::DeleteWebContentsSoon(
+    std::unique_ptr<content::WebContents> web_contents) {
+  if (web_contents_to_delete_.empty()) {
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&ProfileImpl::DeleteScheduleWebContents,
+                                  weak_ptr_factory_.GetWeakPtr()));
+  }
+  web_contents_to_delete_.push_back(std::move(web_contents));
+}
+
 BrowserContextImpl* ProfileImpl::GetBrowserContext() {
   if (browser_context_)
     return browser_context_.get();
@@ -667,4 +682,8 @@ int ProfileImpl::GetNumberOfBrowsers() {
                        [this](BrowserImpl* b) { return b->profile() == this; });
 }
 
+void ProfileImpl::DeleteScheduleWebContents() {
+  web_contents_to_delete_.clear();
+}
+
 }  // namespace weblayer

weblayer/browser/profile_impl.h

@@ -6,6 +6,7 @@
 #define WEBLAYER_BROWSER_PROFILE_IMPL_H_
 
 #include <set>
+#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -23,6 +24,7 @@
 
 namespace content {
 class BrowserContext;
+class WebContents;
 }
 
 namespace weblayer {
@@ -64,6 +66,11 @@ class ProfileImpl : public Profile {
   static void AddProfileObserver(ProfileObserver* observer);
   static void RemoveProfileObserver(ProfileObserver* observer);
 
+  // Deletes |web_contents| after a delay. This is used if the owning Tab is
+  // deleted and it's not safe to delete the WebContents.
+  void DeleteWebContentsSoon(
+      std::unique_ptr<content::WebContents> web_contents);
+
   BrowserContextImpl* GetBrowserContext();
 
   // Called when the download subsystem has finished initializing. By this point
@@ -156,6 +163,8 @@ class ProfileImpl : public Profile {
   // Returns the number of Browsers with this profile.
   int GetNumberOfBrowsers();
 
+  void DeleteScheduleWebContents();
+
   ProfileInfo info_;
 
   std::unique_ptr<BrowserContextImpl> browser_context_;
@@ -178,6 +187,10 @@ class ProfileImpl : public Profile {
   // CancelableTaskTracker is owned by Profile.
   base::CancelableTaskTracker cancelable_task_tracker_;
 
+  std::vector<std::unique_ptr<content::WebContents>> web_contents_to_delete_;
+
+  base::WeakPtrFactory<ProfileImpl> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ProfileImpl);
 };
 

weblayer/browser/tab_impl.cc

@@ -301,8 +301,7 @@ TabImpl::TabImpl(ProfileImpl* profile,
 
   sessions::SessionTabHelper::CreateForWebContents(
       web_contents_.get(),
-      base::BindRepeating(&TabImpl::GetSessionServiceTabHelperDelegate,
-                          base::Unretained(this)));
+      base::BindRepeating(&TabImpl::GetSessionServiceTabHelperDelegate));
 
   permissions::PermissionRequestManager::CreateForWebContents(
       web_contents_.get());
@@ -361,6 +360,14 @@ TabImpl::~TabImpl() {
 #endif
   Observe(nullptr);
   web_contents_->SetDelegate(nullptr);
+  if (navigation_controller_->should_delay_web_contents_deletion()) {
+    // Remove the linkage between this and the WebContents.
+    web_contents_->RemoveUserData(&kWebContentsUserDataKey);
+    // Have Profile handle the task posting to ensure the WebContents is
+    // deleted before Profile. To do otherwise means it would be possible for
+    // the Profile to outlive the WebContents, which is problematic (crash).
+    profile_->DeleteWebContentsSoon(std::move(web_contents_));
+  }
   web_contents_.reset();
   GetTabs().erase(this);
 }
@@ -568,7 +575,8 @@ static void JNI_TabImpl_DeleteTab(JNIEnv* env, jlong tab) {
   TabImpl* tab_impl = reinterpret_cast<TabImpl*>(tab);
   DCHECK(tab_impl);
   DCHECK(tab_impl->browser());
-  tab_impl->browser()->DestroyTab(tab_impl);
+  // Don't call Browser::DestroyTab() as it calls back to the java side.
+  tab_impl->browser()->DestroyTabFromJava(tab_impl);
 }
 
 ScopedJavaLocalRef<jobject> TabImpl::GetWebContents(JNIEnv* env) {
@@ -1246,10 +1254,11 @@ find_in_page::FindTabHelper* TabImpl::GetFindTabHelper() {
   return find_in_page::FindTabHelper::FromWebContents(web_contents_.get());
 }
 
+// static
 sessions::SessionTabHelperDelegate* TabImpl::GetSessionServiceTabHelperDelegate(
     content::WebContents* web_contents) {
-  DCHECK_EQ(web_contents, web_contents_.get());
-  return browser_ ? browser_->browser_persister() : nullptr;
+  TabImpl* tab = FromWebContents(web_contents);
+  return (tab && tab->browser_) ? tab->browser_->browser_persister() : nullptr;
 }
 
 bool TabImpl::SetDataInternal(const std::map<std::string, std::string>& data) {

weblayer/browser/tab_impl.h

@@ -345,7 +345,7 @@ class TabImpl : public Tab,
   // Returns the FindTabHelper for the page, or null if none exists.
   find_in_page::FindTabHelper* GetFindTabHelper();
 
-  sessions::SessionTabHelperDelegate* GetSessionServiceTabHelperDelegate(
+  static sessions::SessionTabHelperDelegate* GetSessionServiceTabHelperDelegate(
       content::WebContents* web_contents);
 
 #if defined(OS_ANDROID)