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 <[email protected]> Reviewed-by: Chris Hamilton <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#541540}(cherry picked from commit 70546dc) Reviewed-on: https://chromium-review.googlesource.com/953367 Reviewed-by: Sébastien Marchand <[email protected]> Cr-Commit-Position: refs/branch-heads/3359@{#74} Cr-Branched-From: 66afc5e-refs/heads/master@{#540276}
nwjs · Mar 7, 2018 · ce49bf6 · ce49bf6
1 parent 74361db
commit ce49bf6
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 36 deletions.
diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc b/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)
-    : content::WebContentsObserver(web_contents),
-      observers_(observers),
+    : observers_(observers),
+      web_contents_(web_contents),
       tab_strip_model_(tab_strip_model) {
   DCHECK(observers_);
-  DCHECK(GetWebContents());
+  DCHECK(web_contents_);
   DCHECK(tab_strip_model_);
 }
 
@@ -48,7 +48,7 @@ void TabLifecycleUnitSource::TabLifecycleUnit::SetTabStripModel(
 void TabLifecycleUnitSource::TabLifecycleUnit::SetWebContents(
     content::WebContents* web_contents) {
   DCHECK(web_contents);
-  Observe(web_contents);
+  web_contents_ = web_contents;
 }
 
 void TabLifecycleUnitSource::TabLifecycleUnit::SetFocused(bool focused) {
@@ -264,7 +264,7 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::Discard(
 
 content::WebContents* TabLifecycleUnitSource::TabLifecycleUnit::GetWebContents()
     const {
-  return web_contents();
+  return web_contents_;
 }
 
 bool TabLifecycleUnitSource::TabLifecycleUnit::IsMediaTab() const {
@@ -324,7 +324,7 @@ TabLifecycleUnitSource::TabLifecycleUnit::GetRenderProcessHost() const {
   return GetWebContents()->GetMainFrame()->GetProcess();
 }
 
-void TabLifecycleUnitSource::TabLifecycleUnit::DidStartLoading() {
+void TabLifecycleUnitSource::TabLifecycleUnit::OnDidStartLoading() {
   if (GetState() == State::DISCARDED) {
     SetState(State::LOADED);
     OnDiscardedStateChange();

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.h b/chrome/browser/resource_coordinator/tab_lifecycle_unit.h
@@ -12,7 +12,6 @@
 #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;
 
@@ -36,8 +35,7 @@ static constexpr base::TimeDelta kTabFocusedProtectionTime =
 // Represents a tab.
 class TabLifecycleUnitSource::TabLifecycleUnit
     : public LifecycleUnitBase,
-      public TabLifecycleUnitExternal,
-      public content::WebContentsObserver {
+      public TabLifecycleUnitExternal {
  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
@@ -69,6 +67,10 @@ 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;
@@ -96,13 +98,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_;
 

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc
@@ -14,6 +14,7 @@
 #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 {
 
@@ -63,6 +64,41 @@ 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_;
@@ -113,6 +149,11 @@ 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.
@@ -122,18 +163,6 @@ 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,
@@ -151,7 +180,11 @@ 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);
 }
 
@@ -178,4 +211,15 @@ 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
diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h b/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h
@@ -53,6 +53,7 @@ class TabLifecycleUnitSource : public BrowserListObserver,
   friend class TabLifecycleUnitTest;
 
   class TabLifecycleUnit;
+  class TabLifecycleUnitWebContentsObserver;
 
   // Returns the TabStripModel of the focused browser window, if any.
   TabStripModel* GetFocusedTabStripModel() const;
@@ -71,9 +72,6 @@ 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,
@@ -90,6 +88,8 @@ 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,6 +108,12 @@ 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);
 };
 

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc
@@ -73,8 +73,7 @@ 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();
@@ -443,4 +442,22 @@ 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
diff --git a/chrome/browser/resource_coordinator/tab_manager.cc b/chrome/browser/resource_coordinator/tab_manager.cc
@@ -482,13 +482,7 @@ void TabManager::PurgeBackgroundedTabsIfNeeded() {
     DCHECK(tab_lifecycle_unit_external);
     content::WebContents* content =
         tab_lifecycle_unit_external->GetWebContents();
-    // 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;
+    DCHECK(content);
 
     content::RenderProcessHost* render_process_host =
         content->GetMainFrame()->GetProcess();