Revert "Fix some tab lifetime management issues in TabLifecycleUnitSource."

This reverts commit 70546dc6.

Reason for revert: Causing flaky crashes on waterfall, see https://crbug.com/820075.

Original change's description:
> Fix some tab lifetime management issues in TabLifecycleUnitSource.
> 
> Move the WebContentsObserver from TabLifeCycleUnit to TabLifeCycleUnitSource,
> this allows for a better tracking of the WebContents lifetime, in some
> situation a WebContents might get detached from the TabStrip and then
> destroyed, which mean that we won't get a TabClosingAt notification for
> this tab destruction.
> 
> Another solution would be to implement the TabStripModelObserver::TabDetachedAt
> function and track the tabs which are in a detached state but this is slightly
> more complex because TabDetachedAt might be called for several reasons:
> - A TabDetachedAt usually come after a TabClosedAt event.
> - TabDetachedAt might be followed by TabInsertedAt, or not if it get destroyed.
>   because of this we won't know if we should keep the TabLifeCycleUnit for this
>   WebContents around (i.e. if it'll get re-inserted in a tab strip) or destroy
>   it because it's being destroyed.
> 
> Observing WebContentsObserver::WebContentsDestroyed and moving the logic that
> was in TabClosingAt to this method address these issues, it's the same approach
> than the one we took in TabStatsTracker.
> 
> Bug: 819352, 818454
> Change-Id: Ibd3fe49b2798ade19ee781cb70eb30e52372d686
> Reviewed-on: https://chromium-review.googlesource.com/952405
> Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#541540}

TBR=chrisha@chromium.org,sebmarchand@chromium.org

Change-Id: I0aa17b4db9f4c1468e190ef4ec0dc86aeb08c3a8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 819352, 818454
Reviewed-on: https://chromium-review.googlesource.com/955882Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541802}

chrome/browser/resource_coordinator/tab_lifecycle_unit.cc

@@ -27,11 +27,11 @@ TabLifecycleUnitSource::TabLifecycleUnit::TabLifecycleUnit(
     base::ObserverList<TabLifecycleObserver>* observers,
     content::WebContents* web_contents,
     TabStripModel* tab_strip_model)
-    : observers_(observers),
-      web_contents_(web_contents),
+    : content::WebContentsObserver(web_contents),
+      observers_(observers),
       tab_strip_model_(tab_strip_model) {
   DCHECK(observers_);
-  DCHECK(web_contents_);
+  DCHECK(GetWebContents());
   DCHECK(tab_strip_model_);
 }
 
@@ -48,7 +48,7 @@ void TabLifecycleUnitSource::TabLifecycleUnit::SetTabStripModel(
 void TabLifecycleUnitSource::TabLifecycleUnit::SetWebContents(
     content::WebContents* web_contents) {
   DCHECK(web_contents);
-  web_contents_ = web_contents;
+  Observe(web_contents);
 }
 
 void TabLifecycleUnitSource::TabLifecycleUnit::SetFocused(bool focused) {
@@ -268,7 +268,7 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::Discard(
 
 content::WebContents* TabLifecycleUnitSource::TabLifecycleUnit::GetWebContents()
     const {
-  return web_contents_;
+  return web_contents();
 }
 
 bool TabLifecycleUnitSource::TabLifecycleUnit::IsMediaTab() const {
@@ -328,7 +328,7 @@ TabLifecycleUnitSource::TabLifecycleUnit::GetRenderProcessHost() const {
   return GetWebContents()->GetMainFrame()->GetProcess();
 }
 
-void TabLifecycleUnitSource::TabLifecycleUnit::OnDidStartLoading() {
+void TabLifecycleUnitSource::TabLifecycleUnit::DidStartLoading() {
   if (GetState() == State::DISCARDED) {
     SetState(State::LOADED);
     OnDiscardedStateChange();

chrome/browser/resource_coordinator/tab_lifecycle_unit.h

@@ -12,6 +12,7 @@
 #include "chrome/browser/resource_coordinator/tab_lifecycle_unit_external.h"
 #include "chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h"
 #include "chrome/browser/resource_coordinator/time.h"
+#include "content/public/browser/web_contents_observer.h"
 
 class TabStripModel;
 
@@ -35,7 +36,8 @@ static constexpr base::TimeDelta kTabFocusedProtectionTime =
 // Represents a tab.
 class TabLifecycleUnitSource::TabLifecycleUnit
     : public LifecycleUnitBase,
-      public TabLifecycleUnitExternal {
+      public TabLifecycleUnitExternal,
+      public content::WebContentsObserver {
  public:
   // |observers| is a list of observers to notify when the discarded state or
   // the auto-discardable state of this tab changes. It can be modified outside
@@ -67,10 +69,6 @@ class TabLifecycleUnitSource::TabLifecycleUnit
   // "recently audible" state of the tab changes.
   void SetRecentlyAudible(bool recently_audible);
 
-  // Invoked when we receive a DidStartLoading notification for the WebContents
-  // used by this tab.
-  void OnDidStartLoading();
-
   // LifecycleUnit:
   TabLifecycleUnitExternal* AsTabLifecycleUnitExternal() override;
   base::string16 GetTitle() const override;
@@ -98,13 +96,13 @@ class TabLifecycleUnitSource::TabLifecycleUnit
   // Returns the RenderProcessHost associated with this tab.
   content::RenderProcessHost* GetRenderProcessHost() const;
 
+  // content::WebContentsObserver:
+  void DidStartLoading() override;
+
   // List of observers to notify when the discarded state or the auto-
   // discardable state of this tab changes.
   base::ObserverList<TabLifecycleObserver>* observers_;
 
-  // The WebContents for this tab.
-  content::WebContents* web_contents_;
-
   // TabStripModel to which this tab belongs.
   TabStripModel* tab_strip_model_;
 

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "content/public/browser/web_contents_observer.h"
 
 namespace resource_coordinator {
 
@@ -64,41 +63,6 @@ void TabLifecycleUnitSource::SetFocusedTabStripModelForTesting(
   UpdateFocusedTab();
 }
 
-// A specialization of content::WebContentsObserver that allows to properly
-// track the destruction of tabs.
-class TabLifecycleUnitSource::TabLifecycleUnitWebContentsObserver
-    : public content::WebContentsObserver {
- public:
-  TabLifecycleUnitWebContentsObserver(
-      content::WebContents* web_contents,
-      TabLifecycleUnit* lifecycle_unit,
-      TabLifecycleUnitSource* lifecycle_unit_source)
-      : content::WebContentsObserver(web_contents),
-        lifecycle_unit_(lifecycle_unit),
-        lifecycle_unit_source_(lifecycle_unit_source) {}
-
-  void DidStartLoading() override {
-    DCHECK(lifecycle_unit_);
-    lifecycle_unit_->OnDidStartLoading();
-  }
-
-  void WebContentsDestroyed() override {
-    lifecycle_unit_source_->OnWebContentsDestroyed(web_contents());
-    // The call above will free |this| and so nothing should be done on this
-    // object starting from here.
-  }
-
- private:
-  // The lifecycle unit for this tab.
-  TabLifecycleUnit* lifecycle_unit_;
-
-  // The lifecycle unit source that should get notified when the observed
-  // WebContents is about to be destroyed.
-  TabLifecycleUnitSource* lifecycle_unit_source_;
-
-  DISALLOW_COPY_AND_ASSIGN(TabLifecycleUnitWebContentsObserver);
-};
-
 TabStripModel* TabLifecycleUnitSource::GetFocusedTabStripModel() const {
   if (focused_tab_strip_model_for_testing_)
     return focused_tab_strip_model_for_testing_;
@@ -149,11 +113,6 @@ void TabLifecycleUnitSource::TabInsertedAt(TabStripModel* tab_strip_model,
     // Add a self-owned observer to the LifecycleUnit to record metrics.
     lifecycle_unit->AddObserver(new DiscardMetricsLifecycleUnitObserver());
 
-    // Track the WebContents used by this tab.
-    web_contents_observers_.insert(std::make_pair(
-        contents, std::make_unique<TabLifecycleUnitWebContentsObserver>(
-                      contents, lifecycle_unit, this)));
-
     NotifyLifecycleUnitCreated(lifecycle_unit);
   } else {
     // A tab was moved to another window.
@@ -163,6 +122,18 @@ void TabLifecycleUnitSource::TabInsertedAt(TabStripModel* tab_strip_model,
   }
 }
 
+void TabLifecycleUnitSource::TabClosingAt(TabStripModel* tab_strip_model,
+                                          content::WebContents* contents,
+                                          int index) {
+  auto it = tabs_.find(contents);
+  DCHECK(it != tabs_.end());
+  TabLifecycleUnit* lifecycle_unit = it->second.get();
+  if (focused_tab_lifecycle_unit_ == lifecycle_unit)
+    focused_tab_lifecycle_unit_ = nullptr;
+  NotifyLifecycleUnitDestroyed(lifecycle_unit);
+  tabs_.erase(contents);
+}
+
 void TabLifecycleUnitSource::ActiveTabChanged(
     content::WebContents* old_contents,
     content::WebContents* new_contents,
@@ -180,11 +151,7 @@ void TabLifecycleUnitSource::TabReplacedAt(TabStripModel* tab_strip_model,
   DCHECK(it != tabs_.end());
   std::unique_ptr<TabLifecycleUnit> lifecycle_unit = std::move(it->second);
   lifecycle_unit->SetWebContents(new_contents);
-  web_contents_observers_.erase(old_contents);
   tabs_.erase(it);
-  web_contents_observers_.insert(std::make_pair(
-      new_contents, std::make_unique<TabLifecycleUnitWebContentsObserver>(
-                        new_contents, lifecycle_unit.get(), this)));
   tabs_[new_contents] = std::move(lifecycle_unit);
 }
 
@@ -211,15 +178,4 @@ void TabLifecycleUnitSource::OnBrowserNoLongerActive(Browser* browser) {
   UpdateFocusedTab();
 }
 
-void TabLifecycleUnitSource::OnWebContentsDestroyed(
-    content::WebContents* contents) {
-  auto it = tabs_.find(contents);
-  DCHECK(it != tabs_.end());
-  TabLifecycleUnit* lifecycle_unit = it->second.get();
-  if (focused_tab_lifecycle_unit_ == lifecycle_unit)
-    focused_tab_lifecycle_unit_ = nullptr;
-  NotifyLifecycleUnitDestroyed(lifecycle_unit);
-  tabs_.erase(contents);
-}
-
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h

@@ -53,7 +53,6 @@ class TabLifecycleUnitSource : public BrowserListObserver,
   friend class TabLifecycleUnitTest;
 
   class TabLifecycleUnit;
-  class TabLifecycleUnitWebContentsObserver;
 
   // Returns the TabStripModel of the focused browser window, if any.
   TabStripModel* GetFocusedTabStripModel() const;
@@ -72,6 +71,9 @@ class TabLifecycleUnitSource : public BrowserListObserver,
                      content::WebContents* contents,
                      int index,
                      bool foreground) override;
+  void TabClosingAt(TabStripModel* tab_strip_model,
+                    content::WebContents* contents,
+                    int index) override;
   void ActiveTabChanged(content::WebContents* old_contents,
                         content::WebContents* new_contents,
                         int index,
@@ -88,8 +90,6 @@ class TabLifecycleUnitSource : public BrowserListObserver,
   void OnBrowserSetLastActive(Browser* browser) override;
   void OnBrowserNoLongerActive(Browser* browser) override;
 
-  void OnWebContentsDestroyed(content::WebContents* contents);
-
   // Tracks the BrowserList and all TabStripModels.
   BrowserTabStripTracker browser_tab_strip_tracker_;
 
@@ -108,12 +108,6 @@ class TabLifecycleUnitSource : public BrowserListObserver,
   // changes.
   base::ObserverList<TabLifecycleObserver> tab_lifecycle_observers_;
 
-  // Map from content::WebContents to TabLifecycleUnitWebContentsObserver,
-  // used to track the WebContents destruction.
-  base::flat_map<content::WebContents*,
-                 std::unique_ptr<TabLifecycleUnitWebContentsObserver>>
-      web_contents_observers_;
-
   DISALLOW_COPY_AND_ASSIGN(TabLifecycleUnitSource);
 };
 

chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc

@@ -73,7 +73,8 @@ bool IsFocused(LifecycleUnit* lifecycle_unit) {
 class TabLifecycleUnitSourceTest : public ChromeRenderViewHostTestHarness {
  protected:
   TabLifecycleUnitSourceTest()
-      : scoped_set_tick_clock_for_testing_(&test_clock_) {}
+      : scoped_set_tick_clock_for_testing_(&test_clock_) {
+  }
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
@@ -442,22 +443,4 @@ TEST_F(TabLifecycleUnitSourceTest, CanOnlyDiscardOnce) {
 #endif
 }
 
-TEST_F(TabLifecycleUnitSourceTest, HandleWebContentsDestruction) {
-  LifecycleUnit* first_lifecycle_unit = nullptr;
-  LifecycleUnit* second_lifecycle_unit = nullptr;
-  CreateTwoTabs(true /* focus_tab_strip */, &first_lifecycle_unit,
-                &second_lifecycle_unit);
-
-  // Detach the second WebContents and manually destroy it, the
-  // observer should be notified.
-  EXPECT_CALL(source_observer_,
-              OnLifecycleUnitDestroyed(second_lifecycle_unit));
-  delete tab_strip_model_->DetachWebContentsAt(1);
-
-  testing::Mock::VerifyAndClear(&source_observer_);
-
-  EXPECT_CALL(source_observer_, OnLifecycleUnitDestroyed(first_lifecycle_unit));
-  tab_strip_model_->CloseAllTabs();
-}
-
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_manager.cc

@@ -482,7 +482,13 @@ void TabManager::PurgeBackgroundedTabsIfNeeded() {
     DCHECK(tab_lifecycle_unit_external);
     content::WebContents* content =
         tab_lifecycle_unit_external->GetWebContents();
-    DCHECK(content);
+    // TODO(fdoray): Check if TabLifecycleUnitSource should override
+    // WebContentsObserver::WebContentsDestroyed() as in some situations a
+    // WebContents might get destroyed without a call to
+    // TabStripModelObserver::TabClosingAt, in this case we'll have a
+    // TabLifecycleUnitExternal that points to a null WebContents.
+    if (content == nullptr)
+      return;
 
     content::RenderProcessHost* render_process_host =
         content->GetMainFrame()->GetProcess();