Buffer UI navigation events in DataUseTabModel until rule fetch

Matching rules are fetched when Chrome starts and can take in the order
of 100-200ms. UI navigations that happen before these rules are fetched
will not start the data use tracking. This race condition is fixed by
buffering the navigation events until rule fetch is complete.

BUG=586235

Review URL: https://codereview.chromium.org/1837223002

Cr-Commit-Position: refs/heads/master@{#383817}

chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java

@@ -112,8 +112,9 @@ public class ExternalDataUseObserver {
     }
 
     /**
-    * Fetches matching rules and returns them via {@link #fetchMatchingRulesDone}. While the
-    * fetch is underway, it is illegal to make calls to this method.
+     * Fetches matching rules and returns them via {@link #fetchMatchingRulesDone}. subsequent
+     * calls to this method While the fetch is underway, may cause the {@link
+     * #fetchMatchingRulesDone} callback to be missed for the subsequent call.
      */
     @CalledByNative
     protected void fetchMatchingRules() {

chrome/browser/android/data_usage/data_use_tab_model.cc

@@ -40,6 +40,10 @@ const uint32_t kDefaultOpenTabExpirationDurationSeconds =
 const uint32_t kDefaultMatchingRuleExpirationDurationSeconds =
     60 * 60 * 24;  // 24 hours.
 
+// Default maximum limit imposed for the initial UI navigation event buffer.
+// Once this limit is reached, the buffer will be cleared.
+const uint32_t kDefaultMaxNavigationEventsBuffered = 100;
+
 const char kUMAExpiredInactiveTabEntryRemovalDurationHistogram[] =
     "DataUsage.TabModel.ExpiredInactiveTabEntryRemovalDuration";
 const char kUMAExpiredActiveTabEntryRemovalDurationHistogram[] =
@@ -133,6 +137,7 @@ DataUseTabModel::DataUseTabModel()
       closed_tab_expiration_duration_(GetClosedTabExpirationDuration()),
       open_tab_expiration_duration_(GetOpenTabExpirationDuration()),
       is_control_app_installed_(false),
+      data_use_ui_navigations_(new std::vector<DataUseUINavigationEvent>()),
       weak_factory_(this) {
   // Detach from current thread since rest of DataUseTabModel lives on the UI
   // thread and the current thread may not be UI thread..
@@ -166,6 +171,19 @@ void DataUseTabModel::OnNavigationEvent(SessionID::id_type tab_id,
                                         const std::string& package) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsValidTabID(tab_id));
+
+  if (data_use_ui_navigations_) {
+    // Buffer the navigation event for later processing.
+    data_use_ui_navigations_->push_back(
+        DataUseUINavigationEvent(tab_id, transition, url, package));
+
+    if (data_use_ui_navigations_->size() >=
+        kDefaultMaxNavigationEventsBuffered) {
+      ProcessBufferedNavigationEvents();
+    }
+    return;
+  }
+
   std::string current_label, new_label;
   bool is_package_match;
 
@@ -258,6 +276,7 @@ void DataUseTabModel::RegisterURLRegexes(
     return;
   data_use_matcher_->RegisterURLRegexes(app_package_name, domain_path_regex,
                                         label);
+  ProcessBufferedNavigationEvents();
 }
 
 void DataUseTabModel::OnControlAppInstallStateChange(
@@ -269,8 +288,9 @@ void DataUseTabModel::OnControlAppInstallStateChange(
   is_control_app_installed_ = is_control_app_installed;
   std::vector<std::string> empty;
   if (!is_control_app_installed_) {
-    // Clear rules.
+    // Clear rules, and process the buffered UI navigation events.
     data_use_matcher_->RegisterURLRegexes(empty, empty, empty);
+    ProcessBufferedNavigationEvents();
   } else {
     // Fetch the matching rules when the app is installed.
     data_use_matcher_->FetchMatchingRules();
@@ -459,6 +479,19 @@ void DataUseTabModel::CompactTabEntries() {
   }
 }
 
+void DataUseTabModel::ProcessBufferedNavigationEvents() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!data_use_ui_navigations_)
+    return;
+  // Move the ownership of vector managed by |data_use_ui_navigations_| and
+  // release it, so that navigation events will be processed immediately.
+  const auto tmp_data_use_ui_navigations_ = std::move(data_use_ui_navigations_);
+  for (const auto& ui_event : *tmp_data_use_ui_navigations_.get()) {
+    OnNavigationEvent(ui_event.tab_id, ui_event.transition_type, ui_event.url,
+                      ui_event.package);
+  }
+}
+
 }  // namespace android
 
 }  // namespace chrome

chrome/browser/android/data_usage/data_use_tab_model.h

@@ -187,15 +187,41 @@ class DataUseTabModel {
   FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest,
                            MultipleObserverMultipleStartEndEvents);
   FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest, ObserverStartEndEvents);
+  FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest,
+                           ProcessBufferedNavigationEventsAfterRuleFetch);
   FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest, TabCloseEvent);
   FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest, TabCloseEventEndsTracking);
   FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest,
                            UnexpiredTabEntryRemovaltimeHistogram);
   FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
                            MatchingRuleFetchOnControlAppInstall);
+  FRIEND_TEST_ALL_PREFIXES(
+      ExternalDataUseObserverTest,
+      ProcessBufferedNavigationEventsAfterControlAppNotInstalled);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+                           ProcessBufferedNavigationEventsAfterRuleFetch);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+                           ProcessBufferedNavigationEventsAfterMaxLimit);
 
   typedef base::hash_map<SessionID::id_type, TabDataUseEntry> TabEntryMap;
 
+  // Contains the details of a single UI navigation event.
+  struct DataUseUINavigationEvent {
+    DataUseUINavigationEvent(SessionID::id_type tab_id,
+                             TransitionType transition_type,
+                             GURL url,
+                             std::string package)
+        : tab_id(tab_id),
+          transition_type(transition_type),
+          url(url),
+          package(package) {}
+
+    const SessionID::id_type tab_id;
+    const TransitionType transition_type;
+    const GURL url;
+    const std::string package;
+  };
+
   // Gets the current label of a tab, and the new label if a navigation event
   // occurs in the tab. |tab_id| is the source tab of the generated event,
   // |transition| indicates the type of the UI event/transition,  |url| is the
@@ -231,6 +257,11 @@ class DataUseTabModel {
   // size is |kMaxTabEntries|.
   void CompactTabEntries();
 
+  // Processes the UI navigation events buffered in |data_use_ui_navigations_|
+  // and deletes the vector in |data_use_ui_navigations_| so that navigation
+  // events will not be buffered any more.
+  void ProcessBufferedNavigationEvents();
+
   // Collection of observers that receive tracking session start and end
   // notifications. Notifications are posted on UI thread.
   base::ObserverList<TabDataUseObserver> observers_;
@@ -258,6 +289,15 @@ class DataUseTabModel {
   // True if the external control app is installed.
   bool is_control_app_installed_;
 
+  // Buffer of UI navigation events that occurred until the first rule fetch is
+  // complete or the control app not installed callback was received or until
+  // |kDefaultMaxNavigationEventsBuffered| navigation events were buffered,
+  // whichever occurs first. Existence of the vector in scoped_ptr indicates if
+  // the UI navigation events need to be buffered. If the scoped_ptr contains a
+  // vector all navigation events will be added to it. Otherwise all navigation
+  // events will be processed immediately.
+  scoped_ptr<std::vector<DataUseUINavigationEvent>> data_use_ui_navigations_;
+
   base::ThreadChecker thread_checker_;
 
   base::WeakPtrFactory<DataUseTabModel> weak_factory_;

chrome/browser/android/data_usage/data_use_tab_model_unittest.cc

@@ -920,6 +920,42 @@ TEST_F(DataUseTabModelTest, MatchingRuleClearedOnControlAppUninstall) {
   EXPECT_FALSE(data_use_tab_model_->data_use_matcher_->HasValidRules());
 }
 
+// Tests that UI navigation events are buffered until matching rules are fetched
+// and then processed to start data use tracking.
+TEST_F(DataUseTabModelTest, ProcessBufferedNavigationEventsAfterRuleFetch) {
+  MockTabDataUseObserver mock_observer;
+  std::vector<std::string> app_package_names, domain_regexes, labels;
+
+  app_package_names.push_back(kPackageFoo);
+  domain_regexes.push_back(kURLFoo);
+  labels.push_back(kTestLabel1);
+  data_use_tab_model_->AddObserver(&mock_observer);
+
+  // Navigation event should get buffered, and tracking should not start.
+  EXPECT_CALL(mock_observer, NotifyTrackingStarting(kTabID1)).Times(0);
+  EXPECT_CALL(mock_observer, NotifyTrackingEnding(kTabID1)).Times(0);
+  data_use_tab_model_->OnNavigationEvent(
+      kTabID1, DataUseTabModel::TRANSITION_OMNIBOX_SEARCH, GURL(kURLFoo),
+      std::string());
+  EXPECT_EQ(1U, data_use_tab_model_->data_use_ui_navigations_->size());
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+
+  // Once matching rules are fetched, data use tracking should start.
+  EXPECT_CALL(mock_observer, NotifyTrackingStarting(kTabID1)).Times(1);
+  EXPECT_CALL(mock_observer, NotifyTrackingEnding(kTabID1)).Times(0);
+  RegisterURLRegexes(app_package_names, domain_regexes, labels);
+  EXPECT_FALSE(data_use_tab_model_->data_use_ui_navigations_.get());
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+
+  // Data use tracking should end.
+  EXPECT_CALL(mock_observer, NotifyTrackingStarting(kTabID1)).Times(0);
+  EXPECT_CALL(mock_observer, NotifyTrackingEnding(kTabID1)).Times(1);
+  data_use_tab_model_->OnNavigationEvent(kTabID1,
+                                         DataUseTabModel::TRANSITION_BOOKMARK,
+                                         GURL(std::string()), std::string());
+  EXPECT_FALSE(data_use_tab_model_->data_use_ui_navigations_.get());
+}
+
 }  // namespace android
 
 }  // namespace chrome

chrome/browser/android/data_usage/external_data_use_observer.h

@@ -108,6 +108,13 @@ class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
   FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, MultipleMatchingRules);
   FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
                            PeriodicFetchMatchingRules);
+  FRIEND_TEST_ALL_PREFIXES(
+      ExternalDataUseObserverTest,
+      ProcessBufferedNavigationEventsAfterControlAppNotInstalled);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+                           ProcessBufferedNavigationEventsAfterRuleFetch);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+                           ProcessBufferedNavigationEventsAfterMaxLimit);
   FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
                            RegisteredAsDataUseObserver);
   FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, ReportsMergedCorrectly);

chrome/browser/android/data_usage/external_data_use_observer_unittest.cc

@@ -390,7 +390,8 @@ TEST_F(ExternalDataUseObserverTest, PeriodicFetchMatchingRules) {
 // fetched.
 TEST_F(ExternalDataUseObserverTest, MatchingRuleFetchOnControlAppInstall) {
   {
-    // Matching rules not fetched on navigation if control app is not installed.
+    // Matching rules not fetched on navigation if control app is not installed,
+    // and navigation events will be buffered.
     external_data_use_observer()->last_matching_rules_fetch_time_ =
         base::TimeTicks();
     external_data_use_observer()
@@ -401,6 +402,10 @@ TEST_F(ExternalDataUseObserverTest, MatchingRuleFetchOnControlAppInstall) {
         std::string());
     base::RunLoop().RunUntilIdle();
     histogram_tester.ExpectTotalCount("DataUsage.MatchingRulesCount.Valid", 0);
+    EXPECT_EQ(1U, external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_->size());
+    external_data_use_observer()
+        ->data_use_tab_model_->data_use_ui_navigations_->clear();
   }
 
   {
@@ -410,6 +415,8 @@ TEST_F(ExternalDataUseObserverTest, MatchingRuleFetchOnControlAppInstall) {
         ->data_use_tab_model_->OnControlAppInstallStateChange(true);
     base::RunLoop().RunUntilIdle();
     histogram_tester.ExpectTotalCount("DataUsage.MatchingRulesCount.Valid", 1);
+    EXPECT_FALSE(external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_.get());
   }
 
   {
@@ -638,6 +645,72 @@ TEST_F(ExternalDataUseObserverTest, DataUseReportTimedOut) {
   histogram_tester.ExpectTotalCount(kUMAReportSubmissionDurationHistogram, 0);
 }
 
+// Tests that UI navigation events are buffered until control app not installed
+// callback is received.
+TEST_F(ExternalDataUseObserverTest,
+       ProcessBufferedNavigationEventsAfterControlAppNotInstalled) {
+  external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+      kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+      std::string());
+  external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+      kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+      std::string());
+  EXPECT_EQ(2U, external_data_use_observer()
+                   ->data_use_tab_model_->data_use_ui_navigations_->size());
+  external_data_use_observer()
+      ->data_use_tab_model_->OnControlAppInstallStateChange(false);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(external_data_use_observer()
+                   ->data_use_tab_model_->data_use_ui_navigations_.get());
+}
+
+// Tests that UI navigation events are buffered until control app is installed
+// and matching rules are fetched.
+TEST_F(ExternalDataUseObserverTest,
+       ProcessBufferedNavigationEventsAfterRuleFetch) {
+  external_data_use_observer()->data_use_tab_model_->is_control_app_installed_ =
+      false;
+  base::HistogramTester histogram_tester;
+  external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+      kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+      std::string());
+  external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+      kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+      std::string());
+  EXPECT_EQ(2U, external_data_use_observer()
+                   ->data_use_tab_model_->data_use_ui_navigations_->size());
+  external_data_use_observer()
+      ->data_use_tab_model_->OnControlAppInstallStateChange(true);
+  base::RunLoop().RunUntilIdle();
+  histogram_tester.ExpectTotalCount("DataUsage.MatchingRulesCount.Valid", 3);
+  EXPECT_FALSE(external_data_use_observer()
+                   ->data_use_tab_model_->data_use_ui_navigations_.get());
+}
+
+// Tests that UI navigation events are buffered until the buffer reaches a
+// maximum imposed limit.
+TEST_F(ExternalDataUseObserverTest,
+       ProcessBufferedNavigationEventsAfterMaxLimit) {
+  const uint32_t kDefaultMaxNavigationEventsBuffered = 1000;
+  // Expect that the max imposed limit will be reached, and the buffer will be
+  // cleared.
+  for (size_t count = 0; count < kDefaultMaxNavigationEventsBuffered; ++count) {
+    external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+        kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+        std::string());
+    if (!external_data_use_observer()
+             ->data_use_tab_model_->data_use_ui_navigations_) {
+      // The max limit is reached.
+      break;
+    }
+    EXPECT_EQ(count+1,
+              external_data_use_observer()
+                  ->data_use_tab_model_->data_use_ui_navigations_->size());
+  }
+  EXPECT_FALSE(external_data_use_observer()
+                   ->data_use_tab_model_->data_use_ui_navigations_.get());
+}
+
 }  // namespace android
 
 }  // namespace chrome