Update a TabLifecycleUnit's TabStripModel on detach.

For correctness, set the TabStripModel of a TabLifeycleUnit to nullptr
when it is detached.

Bug: 775644
Change-Id: Icf7ad8d1be8c130ae934f74f2855eab40d21207d
Reviewed-on: https://chromium-review.googlesource.com/980777
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545865}

chrome/browser/resource_coordinator/tab_lifecycle_unit.cc

@@ -41,8 +41,7 @@ TabLifecycleUnitSource::TabLifecycleUnit::~TabLifecycleUnit() {
 
 void TabLifecycleUnitSource::TabLifecycleUnit::SetTabStripModel(
     TabStripModel* tab_strip_model) {
-  DCHECK(tab_strip_model);
-  tab_strip_model = tab_strip_model_;
+  tab_strip_model_ = tab_strip_model;
 }
 
 void TabLifecycleUnitSource::TabLifecycleUnit::SetWebContents(
@@ -108,8 +107,12 @@ int TabLifecycleUnitSource::TabLifecycleUnit::
 
 bool TabLifecycleUnitSource::TabLifecycleUnit::CanDiscard(
     DiscardReason reason) const {
+  // Can't discard a tab that isn't in a TabStripModel.
+  if (!tab_strip_model_)
+    return false;
+
   // Can't discard a tab that is already discarded.
-  if (GetState() == State::DISCARDED)
+  if (IsDiscarded())
     return false;
 
   if (GetWebContents()->IsCrashed())
@@ -175,7 +178,7 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::CanDiscard(
 
 bool TabLifecycleUnitSource::TabLifecycleUnit::Discard(
     DiscardReason discard_reason) {
-  if (IsDiscarded())
+  if (!tab_strip_model_ || IsDiscarded())
     return false;
 
   UMA_HISTOGRAM_BOOLEAN(

chrome/browser/resource_coordinator/tab_lifecycle_unit.h

@@ -51,8 +51,8 @@ class TabLifecycleUnitSource::TabLifecycleUnit
   ~TabLifecycleUnit() override;
 
   // Sets the TabStripModel associated with this tab. The source that created
-  // this TabLifecycleUnit is responsible for calling this when the tab moves to
-  // a different TabStripModel.
+  // this TabLifecycleUnit is responsible for calling this when the tab is
+  // removed from a TabStripModel or inserted into a new TabStripModel.
   void SetTabStripModel(TabStripModel* tab_strip_model);
 
   // Sets the WebContents associated with this tab. The source that created this

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc

@@ -127,11 +127,29 @@ void TabLifecycleUnitSource::TabClosingAt(TabStripModel* tab_strip_model,
   DCHECK(it != tabs_.end());
   TabLifecycleUnit* lifecycle_unit = it->second.get();
   if (focused_tab_lifecycle_unit_ == lifecycle_unit)
-    focused_tab_lifecycle_unit_ = nullptr;
+    UpdateFocusedTabTo(nullptr);
   NotifyLifecycleUnitDestroyed(lifecycle_unit);
   tabs_.erase(contents);
 }
 
+void TabLifecycleUnitSource::TabDetachedAt(content::WebContents* contents,
+                                           int index) {
+  auto it = tabs_.find(contents);
+
+  // TabDetachedAt() can be called after the TabLifecycleUnit has been destroyed
+  // by TabClosingAt().
+  // TODO(fdoray): Changed this to a DCHECK once TabLifecycleUnits are destroyed
+  // from WebContentsDestroyed() rather than TabClosingAt().
+  // https://crbug.com/775644
+  if (it == tabs_.end())
+    return;
+
+  TabLifecycleUnit* lifecycle_unit = it->second.get();
+  if (focused_tab_lifecycle_unit_ == lifecycle_unit)
+    UpdateFocusedTabTo(nullptr);
+  lifecycle_unit->SetTabStripModel(nullptr);
+}
+
 void TabLifecycleUnitSource::ActiveTabChanged(
     content::WebContents* old_contents,
     content::WebContents* new_contents,

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h

@@ -74,6 +74,7 @@ class TabLifecycleUnitSource : public BrowserListObserver,
   void TabClosingAt(TabStripModel* tab_strip_model,
                     content::WebContents* contents,
                     int index) override;
+  void TabDetachedAt(content::WebContents* contents, int index) override;
   void ActiveTabChanged(content::WebContents* old_contents,
                         content::WebContents* new_contents,
                         int index,

chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc

@@ -218,6 +218,13 @@ class TabLifecycleUnitSourceTest : public ChromeRenderViewHostTestHarness {
   DISALLOW_COPY_AND_ASSIGN(TabLifecycleUnitSourceTest);
 };
 
+#define EXPECT_FOR_ALL_DISCARD_REASONS(lifecycle_unit, method, value)    \
+  do {                                                                   \
+    EXPECT_EQ(value, lifecycle_unit->method(DiscardReason::kExternal));  \
+    EXPECT_EQ(value, lifecycle_unit->method(DiscardReason::kProactive)); \
+    EXPECT_EQ(value, lifecycle_unit->method(DiscardReason::kUrgent));    \
+  } while (false)
+
 }  // namespace
 
 TEST_F(TabLifecycleUnitSourceTest, AppendTabsToFocusedTabStrip) {
@@ -297,6 +304,41 @@ TEST_F(TabLifecycleUnitSourceTest, ReplaceWebContents) {
   tab_strip_model_->CloseAllTabs();
 }
 
+TEST_F(TabLifecycleUnitSourceTest, DetachWebContents) {
+  LifecycleUnit* first_lifecycle_unit = nullptr;
+  LifecycleUnit* second_lifecycle_unit = nullptr;
+  CreateTwoTabs(true /* focus_tab_strip */, &first_lifecycle_unit,
+                &second_lifecycle_unit);
+  test_clock_.Advance(kTabFocusedProtectionTime);
+
+  // Detach the non-active tab. Verify that it can no longer be discarded.
+  EXPECT_FOR_ALL_DISCARD_REASONS(first_lifecycle_unit, CanDiscard, true);
+  auto* contents = tab_strip_model_->DetachWebContentsAt(0);
+  EXPECT_FOR_ALL_DISCARD_REASONS(first_lifecycle_unit, CanDiscard, false);
+
+  // Create a second tab strip.
+  NoUnloadListenerTabStripModelDelegate other_tab_strip_model_delegate;
+  TabStripModel other_tab_strip_model(&other_tab_strip_model_delegate,
+                                      profile());
+  other_tab_strip_model.AddObserver(&source_);
+
+  // Insert the tab into the second tab strip without focusing it. Verify that
+  // it can be discarded.
+  other_tab_strip_model.AppendWebContents(contents, false);
+  EXPECT_FOR_ALL_DISCARD_REASONS(first_lifecycle_unit, CanDiscard, true);
+
+  EXPECT_EQ(LifecycleUnit::State::LOADED, first_lifecycle_unit->GetState());
+  EXPECT_CALL(tab_observer_, OnDiscardedStateChange(testing::_, true));
+  first_lifecycle_unit->Discard(DiscardReason::kProactive);
+  testing::Mock::VerifyAndClear(&tab_observer_);
+  EXPECT_EQ(LifecycleUnit::State::DISCARDED, first_lifecycle_unit->GetState());
+
+  // Expect a notification when the tab is closed.
+  EXPECT_CALL(source_observer_, OnLifecycleUnitDestroyed(testing::_))
+      .Times(other_tab_strip_model.count());
+  other_tab_strip_model.CloseAllTabs();
+}
+
 // Tab discarding is tested here rather than in TabLifecycleUnitTest because
 // collaboration from the TabLifecycleUnitSource is required to replace the
 // WebContents in the TabLifecycleUnit.
@@ -402,9 +444,7 @@ TEST_F(TabLifecycleUnitSourceTest, CanOnlyDiscardOnce) {
   test_clock_.Advance(kTabFocusedProtectionTime);
 
   // It should be possible to discard the background tab.
-  EXPECT_TRUE(background_lifecycle_unit->CanDiscard(DiscardReason::kExternal));
-  EXPECT_TRUE(background_lifecycle_unit->CanDiscard(DiscardReason::kProactive));
-  EXPECT_TRUE(background_lifecycle_unit->CanDiscard(DiscardReason::kUrgent));
+  EXPECT_FOR_ALL_DISCARD_REASONS(background_lifecycle_unit, CanDiscard, true);
 
   // Discard the tab.
   EXPECT_EQ(LifecycleUnit::State::LOADED,