[sheriff] Revert "weblayer: make it possible to delete tab in OnNavigationFailed..."

This reverts commit 7d8ab070.

Reason for revert: NavigationBrowserTest.DestroyTabInNavigation fails
on Android P, https://crbug.com/1123021

Original change's description:
> 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}

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

Change-Id: I3a52b345d649e48ae0d9b6a4e480cf602214d2c4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1111127, 1123021
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2380657Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802649}

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

@@ -827,32 +827,4 @@ 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,12 +271,6 @@ 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);
@@ -289,12 +283,7 @@ 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,14 +97,6 @@ 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,7 +434,6 @@ 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,7 +14,6 @@
 #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"
@@ -69,11 +68,8 @@ class NavigationObserverImpl : public NavigationObserver {
       completed_callback_.Run(navigation);
   }
   void NavigationFailed(Navigation* navigation) override {
-    // 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);
+    if (failed_callback_)
+      failed_callback_.Run(navigation);
   }
 
  private:
@@ -263,25 +259,6 @@ 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,29 +31,6 @@ 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,
@@ -366,7 +343,6 @@ 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 &&
@@ -378,15 +354,10 @@ 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_) {
@@ -395,15 +366,10 @@ 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,13 +73,7 @@ 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
@@ -140,11 +134,6 @@ 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,6 +47,50 @@ 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,7 +28,6 @@
 #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"
@@ -233,10 +232,6 @@ 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());
@@ -265,16 +260,6 @@ 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();
@@ -682,8 +667,4 @@ 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,7 +6,6 @@
 #define WEBLAYER_BROWSER_PROFILE_IMPL_H_
 
 #include <set>
-#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -24,7 +23,6 @@
 
 namespace content {
 class BrowserContext;
-class WebContents;
 }
 
 namespace weblayer {
@@ -66,11 +64,6 @@ 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
@@ -163,8 +156,6 @@ class ProfileImpl : public Profile {
   // Returns the number of Browsers with this profile.
   int GetNumberOfBrowsers();
 
-  void DeleteScheduleWebContents();
-
   ProfileInfo info_;
 
   std::unique_ptr<BrowserContextImpl> browser_context_;
@@ -187,10 +178,6 @@ 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,7 +301,8 @@ TabImpl::TabImpl(ProfileImpl* profile,
 
   sessions::SessionTabHelper::CreateForWebContents(
       web_contents_.get(),
-      base::BindRepeating(&TabImpl::GetSessionServiceTabHelperDelegate));
+      base::BindRepeating(&TabImpl::GetSessionServiceTabHelperDelegate,
+                          base::Unretained(this)));
 
   permissions::PermissionRequestManager::CreateForWebContents(
       web_contents_.get());
@@ -360,14 +361,6 @@ 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);
 }
@@ -575,8 +568,7 @@ static void JNI_TabImpl_DeleteTab(JNIEnv* env, jlong tab) {
   TabImpl* tab_impl = reinterpret_cast<TabImpl*>(tab);
   DCHECK(tab_impl);
   DCHECK(tab_impl->browser());
-  // Don't call Browser::DestroyTab() as it calls back to the java side.
-  tab_impl->browser()->DestroyTabFromJava(tab_impl);
+  tab_impl->browser()->DestroyTab(tab_impl);
 }
 
 ScopedJavaLocalRef<jobject> TabImpl::GetWebContents(JNIEnv* env) {
@@ -1254,11 +1246,10 @@ find_in_page::FindTabHelper* TabImpl::GetFindTabHelper() {
   return find_in_page::FindTabHelper::FromWebContents(web_contents_.get());
 }
 
-// static
 sessions::SessionTabHelperDelegate* TabImpl::GetSessionServiceTabHelperDelegate(
     content::WebContents* web_contents) {
-  TabImpl* tab = FromWebContents(web_contents);
-  return (tab && tab->browser_) ? tab->browser_->browser_persister() : nullptr;
+  DCHECK_EQ(web_contents, web_contents_.get());
+  return browser_ ? 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();
 
-  static sessions::SessionTabHelperDelegate* GetSessionServiceTabHelperDelegate(
+  sessions::SessionTabHelperDelegate* GetSessionServiceTabHelperDelegate(
       content::WebContents* web_contents);
 
 #if defined(OS_ANDROID)