Revert "Record discarded state in TabActivityWatcher."

This reverts commit 241c941f.

Reason for revert: Causes crash: crbug.com/875697

Original change's description:
> Record discarded state in TabActivityWatcher.
> 
> (1) TabLifecycleObserver is added for TabActivityWatcher to track
>     OnDiscardedStateChange event.
> 
> (2) In DidReplace(replaced_tab), the ukm_source_id and is_discarded
>     state is copied to new_tab so that we can track discarded state
>     after reload or closing.
> (3) A browser test is added.
> 
> Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
> 
> Bug: 872139
> Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
> Reviewed-on: https://chromium-review.googlesource.com/1166763
> Reviewed-by: Steven Holte <holte@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Charles . <charleszhao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#584217}

TBR=chrisha@chromium.org,jam@chromium.org,michaelpg@chromium.org,holte@chromium.org,charleszhao@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 872139
Change-Id: Ic32c319d1a75ebf3237677ccb8935744f95dfae2
Reviewed-on: https://chromium-review.googlesource.com/1181586Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584540}

chrome/browser/browser_process_impl.cc

@@ -70,7 +70,6 @@
 #include "chrome/browser/printing/print_job_manager.h"
 #include "chrome/browser/printing/print_preview_dialog_controller.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/resource_coordinator/tab_activity_watcher.h"
 #include "chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/shell_integration.h"
@@ -888,10 +887,6 @@ resource_coordinator::TabManager* BrowserProcessImpl::GetTabManager() {
             tab_manager_->intervention_policy_database(),
             tab_manager_->usage_clock());
     tab_lifecycle_unit_source_->AddObserver(tab_manager_.get());
-    // Add TabActivityWatcher to TabLifecycleObserver to track tab discards and
-    // reloads.
-    tab_lifecycle_unit_source_->AddTabLifecycleObserver(
-        resource_coordinator::TabActivityWatcher::GetInstance());
   }
   return tab_manager_.get();
 #endif  // defined(OS_ANDROID)

chrome/browser/resource_coordinator/tab_activity_watcher.cc

@@ -91,10 +91,6 @@ class TabActivityWatcher::WebContentsData
     // Copy the replaced tab's stats.
     tab_metrics_.page_metrics = replaced_tab.tab_metrics_.page_metrics;
     tab_metrics_.page_transition = replaced_tab.tab_metrics_.page_transition;
-
-    // Copy ukm_source_id_ and discarded state.
-    discarded_since_backgrounded_ = replaced_tab.discarded_since_backgrounded_;
-    previous_ukm_source_id_ = replaced_tab.ukm_source_id_;
   }
 
   // Call when the WebContents is detached from its tab. If the tab is later
@@ -173,7 +169,6 @@ class TabActivityWatcher::WebContentsData
     }
 
     backgrounded_time_ = NowTicks();
-    discarded_since_backgrounded_ = false;
     LogTabIfBackgrounded();
   }
 
@@ -186,13 +181,9 @@ class TabActivityWatcher::WebContentsData
       return;
 
     // Log the event before updating times.
-    const ukm::SourceId source_id = discarded_since_backgrounded_
-                                        ? previous_ukm_source_id_
-                                        : ukm_source_id_;
     TabActivityWatcher::GetInstance()
         ->tab_metrics_logger_->LogBackgroundTabShown(
-            source_id, NowTicks() - backgrounded_time_, GetMRUFeatures(),
-            discarded_since_backgrounded_);
+            ukm_source_id_, NowTicks() - backgrounded_time_, GetMRUFeatures());
 
     backgrounded_time_ = base::TimeTicks();
     foregrounded_time_ = NowTicks();
@@ -357,8 +348,6 @@ class TabActivityWatcher::WebContentsData
 
   // Updated when a navigation is finished.
   ukm::SourceId ukm_source_id_ = 0;
-  // Recorded when a WebContents is replaced by another.
-  ukm::SourceId previous_ukm_source_id_ = 0;
 
   // When the tab was created.
   base::TimeTicks creation_time_;
@@ -394,9 +383,6 @@ class TabActivityWatcher::WebContentsData
   // event is logged.
   tab_ranker::MRUFeatures mru_features_;
 
-  // Whether this tab is currently in discarded state.
-  bool discarded_since_backgrounded_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(WebContentsData);
 };
 
@@ -549,28 +535,15 @@ void TabActivityWatcher::OnTabClosed(WebContentsData* web_contents_data) {
       NowTicks() - web_contents_data->navigation_time_);
 
   // Log ForegroundedOrClosed event.
-  const ukm::SourceId source_id =
-      web_contents_data->discarded_since_backgrounded_
-          ? web_contents_data->previous_ukm_source_id_
-          : web_contents_data->ukm_source_id_;
   if (!web_contents_data->backgrounded_time_.is_null()) {
     tab_metrics_logger_->LogBackgroundTabClosed(
-        source_id, NowTicks() - web_contents_data->backgrounded_time_,
-        web_contents_data->GetMRUFeatures(),
-        web_contents_data->discarded_since_backgrounded_);
+        web_contents_data->ukm_source_id_,
+        NowTicks() - web_contents_data->backgrounded_time_,
+        web_contents_data->GetMRUFeatures());
   }
 
   // Erase the pointer in |all_closing_tabs_| only when all logging finished.
   all_closing_tabs_.erase(web_contents_data);
 }
 
-void TabActivityWatcher::OnDiscardedStateChange(content::WebContents* contents,
-                                                bool is_discarded) {
-  WebContentsData::FromWebContents(contents)->discarded_since_backgrounded_ |=
-      is_discarded;
-}
-
-void TabActivityWatcher::OnAutoDiscardableStateChange(
-    content::WebContents* contents,
-    bool is_auto_discardable) {}
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_activity_watcher.h

@@ -10,7 +10,6 @@
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/optional.h"
-#include "chrome/browser/resource_coordinator/tab_lifecycle_observer.h"
 #include "chrome/browser/resource_coordinator/tab_ranker/tab_score_predictor.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/browser_tab_strip_tracker.h"
@@ -26,8 +25,7 @@ namespace resource_coordinator {
 // events to determine the end state of each background tab.
 class TabActivityWatcher : public BrowserListObserver,
                            public TabStripModelObserver,
-                           public BrowserTabStripTrackerDelegate,
-                           public TabLifecycleObserver {
+                           public BrowserTabStripTrackerDelegate {
  public:
   TabActivityWatcher();
   ~TabActivityWatcher() override;
@@ -77,12 +75,6 @@ class TabActivityWatcher : public BrowserListObserver,
   // BrowserTabStripTrackerDelegate:
   bool ShouldTrackBrowser(Browser* browser) override;
 
-  // TabLifecycleObserver:
-  void OnDiscardedStateChange(content::WebContents* contents,
-                              bool is_discarded) override;
-  void OnAutoDiscardableStateChange(content::WebContents* contents,
-                                    bool is_auto_discardable) override;
-
   // Resets internal state.
   void ResetForTesting();
 

chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc

@@ -8,8 +8,6 @@
 
 #include "base/macros.h"
 #include "base/test/simple_test_tick_clock.h"
-#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 "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -118,7 +116,6 @@ class TabActivityWatcherUkmTest : public TabActivityWatcherTest {
     TabActivityWatcherTest::PreRunTestOnMainThread();
 
     ukm_entry_checker_ = std::make_unique<UkmEntryChecker>();
-    test_ukm_recorder_ = std::make_unique<ukm::TestAutoSetUkmRecorder>();
   }
 
   void SetUpOnMainThread() override {
@@ -152,41 +149,7 @@ class TabActivityWatcherUkmTest : public TabActivityWatcherTest {
     ukm_entry_checker_->ExpectNewEntry(kFOCEntryName, url, expected_metrics);
   }
 
-  // Uses test_ukm_recorder_ to check new event metrics including:
-  // (1) the number of UkmEntry with given event_name should equals to |size|.
-  // (2) the newest entry should have source_url equals to |url|.
-  // (3) the newest entry should have source_id equals to |source_id| if
-  //     |source_id| is not 0 (skip for the case of 0).
-  // (4) the newest entry should contain all metrics in |expected_metrics|.
-  // Also returns the source_id of the newest entry.
-  ukm::SourceId ExpectedNewEntryWithSourceId(
-      const GURL& url,
-      const std::string& event_name,
-      size_t size,
-      const UkmMetricMap& expected_metrics,
-      ukm::SourceId source_id = 0) {
-    const std::vector<const ukm::mojom::UkmEntry*> entries =
-        test_ukm_recorder_->GetEntriesByName(event_name);
-    // Check size.
-    EXPECT_EQ(entries.size(), size);
-    const ukm::mojom::UkmEntry* entry = entries.back();
-    // Check source_url.
-    test_ukm_recorder_->ExpectEntrySourceHasUrl(entry, url);
-    // Check source_id.
-    if (source_id != 0) {
-      EXPECT_EQ(source_id, entry->source_id);
-    }
-    // Check expected_metrics.
-    for (const auto& metric : expected_metrics) {
-      test_ukm_recorder_->ExpectEntryMetric(entry, metric.first,
-                                            *metric.second);
-    }
-    // return entry source_id.
-    return entry->source_id;
-  }
-
   std::unique_ptr<UkmEntryChecker> ukm_entry_checker_;
-  std::unique_ptr<ukm::TestAutoSetUkmRecorder> test_ukm_recorder_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TabActivityWatcherUkmTest);
@@ -366,70 +329,4 @@ IN_PROC_BROWSER_TEST_F(TabActivityWatcherUkmTest, TabDrag) {
   EXPECT_EQ(0, ukm_entry_checker_->NumNewEntriesRecorded(kFOCEntryName));
 }
 
-// Tests discarded tab is recorded correctly.
-IN_PROC_BROWSER_TEST_F(TabActivityWatcherUkmTest,
-                       DiscardedTabGetsPreviousSourceId) {
-  ukm::SourceId ukm_source_id_for_tab_0 = 0;
-  ukm::SourceId ukm_source_id_for_tab_1 = 0;
-
-  ui_test_utils::NavigateToURL(browser(), test_urls_[0]);
-  EXPECT_EQ(0u, ukm_entry_checker_->NumEntries(kEntryName));
-
-  // Adding a new foreground tab logs the previously active tab.
-  AddTabAtIndex(1, test_urls_[1], ui::PAGE_TRANSITION_LINK);
-  {
-    SCOPED_TRACE("");
-    ukm_entry_checker_->ExpectNewEntry(
-        kEntryName, test_urls_[0],
-        {{TabManager_TabMetrics::kNavigationEntryCountName, 2}});
-
-    ukm_source_id_for_tab_0 = ExpectedNewEntryWithSourceId(
-        test_urls_[0], kEntryName, 1,
-        {{TabManager_TabMetrics::kNavigationEntryCountName, 2}});
-  }
-
-  // Discard the first tab.
-  content::WebContents* first_contents =
-      browser()->tab_strip_model()->GetWebContentsAt(0);
-  resource_coordinator::TabLifecycleUnitSource::GetInstance()
-      ->GetTabLifecycleUnitExternal(first_contents)
-      ->DiscardTab();
-
-  // Switching to first tab logs a forgrounded event for test_urls_[0]
-  // and a backgrounded event for test_urls_[1].
-  browser()->tab_strip_model()->ActivateTabAt(0, kIsUserGesture);
-  {
-    SCOPED_TRACE("");
-    ukm_entry_checker_->ExpectNewEntry(kEntryName, test_urls_[1],
-                                       kBasicMetricValues);
-
-    ukm_source_id_for_tab_1 = ExpectedNewEntryWithSourceId(
-        test_urls_[1], kEntryName, 2, kBasicMetricValues);
-
-    ExpectedNewEntryWithSourceId(
-        test_urls_[0], kFOCEntryName, 1,
-        {{TabManager_Background_ForegroundedOrClosed::kIsForegroundedName, 1},
-         {TabManager_Background_ForegroundedOrClosed::kIsDiscardedName, 1}},
-        ukm_source_id_for_tab_0);
-  }
-
-  // Discard the second tab.
-  content::WebContents* second_content =
-      browser()->tab_strip_model()->GetWebContentsAt(1);
-  resource_coordinator::TabLifecycleUnitSource::GetInstance()
-      ->GetTabLifecycleUnitExternal(second_content)
-      ->DiscardTab();
-
-  CloseBrowserSynchronously(browser());
-  {
-    SCOPED_TRACE("");
-
-    ExpectedNewEntryWithSourceId(
-        test_urls_[1], kFOCEntryName, 2,
-        {{TabManager_Background_ForegroundedOrClosed::kIsForegroundedName, 0},
-         {TabManager_Background_ForegroundedOrClosed::kIsDiscardedName, 1}},
-        ukm_source_id_for_tab_1);
-  }
-}
-
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_metrics_logger.cc

@@ -59,7 +59,6 @@ void LogBackgroundTabForegroundedOrClosed(
     base::TimeDelta inactive_duration,
     const tab_ranker::MRUFeatures& mru_features,
     bool is_foregrounded,
-    bool is_discarded,
     int sequence_id) {
   if (!ukm_source_id)
     return;
@@ -70,7 +69,6 @@ void LogBackgroundTabForegroundedOrClosed(
       .SetMRUIndex(mru_features.index)
       .SetTimeFromBackgrounded(inactive_duration.InMilliseconds())
       .SetTotalTabCount(mru_features.total)
-      .SetIsDiscarded(is_discarded)
       .Record(ukm::UkmRecorder::Get());
 }
 
@@ -219,21 +217,19 @@ void TabMetricsLogger::LogBackgroundTab(ukm::SourceId ukm_source_id,
 void TabMetricsLogger::LogBackgroundTabShown(
     ukm::SourceId ukm_source_id,
     base::TimeDelta inactive_duration,
-    const tab_ranker::MRUFeatures& mru_features,
-    const bool is_discarded) {
+    const tab_ranker::MRUFeatures& mru_features) {
   LogBackgroundTabForegroundedOrClosed(ukm_source_id, inactive_duration,
                                        mru_features, true /* is_shown */,
-                                       is_discarded, ++sequence_id_);
+                                       ++sequence_id_);
 }
 
 void TabMetricsLogger::LogBackgroundTabClosed(
     ukm::SourceId ukm_source_id,
     base::TimeDelta inactive_duration,
-    const tab_ranker::MRUFeatures& mru_features,
-    const bool is_discarded) {
+    const tab_ranker::MRUFeatures& mru_features) {
   LogBackgroundTabForegroundedOrClosed(ukm_source_id, inactive_duration,
                                        mru_features, false /* is_shown */,
-                                       is_discarded, ++sequence_id_);
+                                       ++sequence_id_);
 }
 
 void TabMetricsLogger::LogTabLifetime(ukm::SourceId ukm_source_id,

chrome/browser/resource_coordinator/tab_metrics_logger.h

@@ -66,15 +66,13 @@ class TabMetricsLogger {
   // shown after being inactive.
   void LogBackgroundTabShown(ukm::SourceId ukm_source_id,
                              base::TimeDelta inactive_duration,
-                             const tab_ranker::MRUFeatures& mru_metrics,
-                             const bool is_discarded);
+                             const tab_ranker::MRUFeatures& mru_metrics);
 
   // Logs TabManager.Background.ForegroundedOrClosed UKM for a tab that was
   // closed after being inactive.
   void LogBackgroundTabClosed(ukm::SourceId ukm_source_id,
                               base::TimeDelta inactive_duration,
-                              const tab_ranker::MRUFeatures& mru_metrics,
-                              const bool is_discarded);
+                              const tab_ranker::MRUFeatures& mru_metrics);
 
   // Logs TabManager.TabLifetime UKM for a closed tab.
   void LogTabLifetime(ukm::SourceId ukm_source_id,

chrome/browser/resource_coordinator/tab_metrics_logger_unittest.cc

@@ -160,11 +160,10 @@ class TabMetricsLoggerUKMTest : public ::testing::Test {
 TEST_F(TabMetricsLoggerUKMTest, LogBackgroundTabShown) {
   const tab_ranker::MRUFeatures& mru_metrics{4, 7};
   const int64_t inactive_duration_ms = 1234;
-  const bool is_discarded = false;
 
   GetLogger()->LogBackgroundTabShown(
       GetSourceId(), base::TimeDelta::FromMilliseconds(inactive_duration_ms),
-      mru_metrics, is_discarded);
+      mru_metrics);
 
   // Checks that the size is logged correctly.
   EXPECT_EQ(1U, GetTestUkmRecorder()->sources_count());
@@ -183,19 +182,15 @@ TEST_F(TabMetricsLoggerUKMTest, LogBackgroundTabShown) {
                                           inactive_duration_ms);
   GetTestUkmRecorder()->ExpectEntryMetric(entries[0], "TotalTabCount",
                                           mru_metrics.total);
-  GetTestUkmRecorder()->ExpectEntryMetric(entries[0], "IsDiscarded",
-                                          is_discarded);
 }
 
 // Checks the closed event is logged correctly.
 TEST_F(TabMetricsLoggerUKMTest, LogBackgroundTabClosed) {
   const tab_ranker::MRUFeatures& mru_metrics{4, 7};
   const int64_t inactive_duration_ms = 1234;
-  const bool is_discarded = true;
-
   GetLogger()->LogBackgroundTabClosed(
       GetSourceId(), base::TimeDelta::FromMilliseconds(inactive_duration_ms),
-      mru_metrics, is_discarded);
+      mru_metrics);
 
   // Checks that the size is logged correctly.
   EXPECT_EQ(1U, GetTestUkmRecorder()->sources_count());
@@ -214,18 +209,15 @@ TEST_F(TabMetricsLoggerUKMTest, LogBackgroundTabClosed) {
                                           inactive_duration_ms);
   GetTestUkmRecorder()->ExpectEntryMetric(entries[0], "TotalTabCount",
                                           mru_metrics.total);
-  GetTestUkmRecorder()->ExpectEntryMetric(entries[0], "IsDiscarded",
-                                          is_discarded);
 }
 
 // Checks the sequence id is logged as sequentially incremental sequence across
 // different events.
 TEST_F(TabMetricsLoggerUKMTest, SequenceIdShouldBeLoggedSequentially) {
-  const bool is_discarded = false;
   GetLogger()->LogBackgroundTabShown(GetSourceId(), base::TimeDelta(),
-                                     tab_ranker::MRUFeatures(), is_discarded);
+                                     tab_ranker::MRUFeatures());
   GetLogger()->LogBackgroundTabClosed(GetSourceId(), base::TimeDelta(),
-                                      tab_ranker::MRUFeatures(), is_discarded);
+                                      tab_ranker::MRUFeatures());
 
   EXPECT_EQ(2U, GetTestUkmRecorder()->sources_count());
   EXPECT_EQ(2U, GetTestUkmRecorder()->entries_count());

tools/metrics/ukm/ukm.xml

@@ -3632,12 +3632,6 @@ be describing additional metrics about the same event.
     Collects the duration in MS from when the tab is backgrounded to when it is
     brought to foreground or closed.
   </summary>
-  <metric name="IsDiscarded">
-    <summary>
-      Boolean value indicating whether or not the tab is discarded since it was
-      backgrounded.
-    </summary>
-  </metric>
   <metric name="IsForegrounded">
     <summary>
       Boolean value indicating whether or not it's triggered because the tab is