Make bloated renderer infobar showing logic more robust.

This CL fixes a potential race when the user starts navigation at the
same time as the bloated renderer is being reloaded. The race may
cause the infobar to be shown on user navigation, which is misleading.

Now the navigation id is saved for the bloated renderer reload and
later on it is matched with the navigation id provided to the
DidFinishNavigation event. The infobar is shown only if the ids match.

Bug: 808143
Change-Id: I1e3c37e565e1434b6ab5557df44281fb418c6e12
Reviewed-on: https://chromium-review.googlesource.com/1106619
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572589}

chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/infobars/core/simple_alert_infobar_delegate.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/page_importance_signals.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -41,13 +42,21 @@ BloatedRendererTabHelper::BloatedRendererTabHelper(
   }
 }
 
+void BloatedRendererTabHelper::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (state_ == State::kRequestingReload) {
+    saved_navigation_id_ = navigation_handle->GetNavigationId();
+    state_ = State::kStartedNavigation;
+  }
+}
+
 void BloatedRendererTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
-  // TODO(ulan): Use nagivation_handle to ensure that the finished navigation
-  // is the same nagivation started by reloading the bloated tab.
-  if (reloading_bloated_renderer_) {
+  if (state_ == State::kStartedNavigation &&
+      saved_navigation_id_ == navigation_handle->GetNavigationId()) {
     ShowInfoBar(InfoBarService::FromWebContents(web_contents()));
-    reloading_bloated_renderer_ = false;
+    state_ = State::kInactive;
+    saved_navigation_id_ = 0;
   }
 }
 
@@ -116,9 +125,12 @@ void BloatedRendererTabHelper::OnRendererIsBloated(
     if (renderer->FastShutdownIfPossible(expected_page_count,
                                          skip_unload_handlers)) {
       const bool check_for_repost = true;
-      reloading_bloated_renderer_ = true;
+      // Clear the state and the saved navigation id.
+      state_ = State::kRequestingReload;
+      saved_navigation_id_ = 0;
       web_contents()->GetController().Reload(content::ReloadType::NORMAL,
                                              check_for_repost);
+      DCHECK_EQ(State::kStartedNavigation, state_);
       RecordBloatedRendererHandling(
           BloatedRendererHandlingInBrowser::kReloaded);
     } else {

chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.h

@@ -28,6 +28,8 @@ class BloatedRendererTabHelper
   ~BloatedRendererTabHelper() override = default;
 
   // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void WebContentsDestroyed() override;
@@ -42,6 +44,8 @@ class BloatedRendererTabHelper
  private:
   friend class content::WebContentsUserData<BloatedRendererTabHelper>;
   FRIEND_TEST_ALL_PREFIXES(BloatedRendererTabHelperTest, DetectReload);
+  FRIEND_TEST_ALL_PREFIXES(BloatedRendererTabHelperTest,
+                           IgnoreUnrelatedNavigation);
   FRIEND_TEST_ALL_PREFIXES(BloatedRendererTabHelperTest, CanReloadBloatedTab);
   FRIEND_TEST_ALL_PREFIXES(BloatedRendererTabHelperTest,
                            CannotReloadBloatedTabCrashed);
@@ -49,12 +53,31 @@ class BloatedRendererTabHelper
                            CannotReloadBloatedTabInvalidURL);
   FRIEND_TEST_ALL_PREFIXES(BloatedRendererTabHelperTest,
                            CannotReloadBloatedTabPendingUserInteraction);
+  enum class State { kInactive, kRequestingReload, kStartedNavigation };
 
   explicit BloatedRendererTabHelper(content::WebContents* contents);
 
   bool CanReloadBloatedTab();
 
-  bool reloading_bloated_renderer_ = false;
+  // The state transitions as follows:
+  // - kInactive is the initial state.
+  //
+  // - any state => kRequestingReload transition happens in
+  //   OnRendererIsBloated before invoking NavigationController::Reload.
+  //
+  // - kRequestingReload => kStartedNavigation transition happens in
+  //   NavigationController::Reload when it invokes DidStartNavigation.
+  //
+  // - kStartedNavigation => kInactive transitions happens in
+  //   DidFinishNavigation.
+  State state_ = State::kInactive;
+
+  // The navigation id is saved on DidStartNavigation event when the state is
+  // kRequestingReload. The infobar is shown on the subsequent
+  // DidFinishNavigation only if its navigation id matches the saved id. This
+  // ensures that the infobar is shown only for the reload that was requested
+  // in OnRendererIsBloated event.
+  int64_t saved_navigation_id_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(BloatedRendererTabHelper);
 };

chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/common/page_importance_signals.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -21,10 +22,39 @@ class BloatedRendererTabHelperTest : public ChromeRenderViewHostTestHarness {
 };
 
 TEST_F(BloatedRendererTabHelperTest, DetectReload) {
-  EXPECT_FALSE(tab_helper_->reloading_bloated_renderer_);
-  tab_helper_->reloading_bloated_renderer_ = true;
-  tab_helper_->DidFinishNavigation(nullptr);
-  EXPECT_FALSE(tab_helper_->reloading_bloated_renderer_);
+  EXPECT_EQ(BloatedRendererTabHelper::State::kInactive, tab_helper_->state_);
+  tab_helper_->state_ = BloatedRendererTabHelper::State::kRequestingReload;
+  auto reload_navigation =
+      content::NavigationHandle::CreateNavigationHandleForTesting(
+          GURL(), web_contents()->GetMainFrame());
+  tab_helper_->DidStartNavigation(reload_navigation.get());
+  EXPECT_EQ(BloatedRendererTabHelper::State::kStartedNavigation,
+            tab_helper_->state_);
+  EXPECT_EQ(reload_navigation->GetNavigationId(),
+            tab_helper_->saved_navigation_id_);
+  tab_helper_->DidFinishNavigation(reload_navigation.get());
+  EXPECT_EQ(BloatedRendererTabHelper::State::kInactive, tab_helper_->state_);
+}
+
+TEST_F(BloatedRendererTabHelperTest, IgnoreUnrelatedNavigation) {
+  EXPECT_EQ(BloatedRendererTabHelper::State::kInactive, tab_helper_->state_);
+  tab_helper_->state_ = BloatedRendererTabHelper::State::kRequestingReload;
+  auto reload_navigation =
+      content::NavigationHandle::CreateNavigationHandleForTesting(
+          GURL(), web_contents()->GetMainFrame());
+  tab_helper_->DidStartNavigation(reload_navigation.get());
+  EXPECT_EQ(BloatedRendererTabHelper::State::kStartedNavigation,
+            tab_helper_->state_);
+  EXPECT_EQ(reload_navigation->GetNavigationId(),
+            tab_helper_->saved_navigation_id_);
+  auto unrelated_navigation =
+      content::NavigationHandle::CreateNavigationHandleForTesting(
+          GURL(), web_contents()->GetMainFrame());
+  tab_helper_->DidFinishNavigation(unrelated_navigation.get());
+  EXPECT_EQ(BloatedRendererTabHelper::State::kStartedNavigation,
+            tab_helper_->state_);
+  EXPECT_EQ(reload_navigation->GetNavigationId(),
+            tab_helper_->saved_navigation_id_);
 }
 
 TEST_F(BloatedRendererTabHelperTest, CanReloadBloatedTab) {