Loading predictor: Allow processing of duplicate hints
Navigation predictor may send preconnect hints multiple times during
and after the page load. This CL relaxes some of the checks
in the loading predictor to allow such duplicate hints to be processed.
A duplicate hint is processed only after the first hint has completed.
Consecutive hints for the same URL would still not be processed.
Change-Id: I0101119159eead1df5c7824861d7687dc88f44c9
Bug: 919566
Reviewed-on: https://chromium-review.googlesource.com/c/1397433
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Alexandr Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621248}diff --git a/chrome/browser/predictors/loading_predictor.cc b/chrome/browser/predictors/loading_predictor.cc
index 18c6c5c..f2ae6c5 100644
--- a/chrome/browser/predictors/loading_predictor.cc
+++ b/chrome/browser/predictors/loading_predictor.cc
@@ -94,6 +94,7 @@
if (!has_preconnect_prediction)
return;
+ ++total_hints_activated_;
active_hints_.emplace(url, base::TimeTicks::Now());
if (IsPreconnectAllowed(profile_))
MaybeAddPreconnect(url, std::move(prediction.requests), origin);
@@ -250,6 +251,7 @@
return;
DCHECK(stats);
+ active_hints_.erase(stats->url);
stats_collector_->RecordPreconnectStats(std::move(stats));
}
diff --git a/chrome/browser/predictors/loading_predictor.h b/chrome/browser/predictors/loading_predictor.h
index 9f65ca2..8a64bed 100644
--- a/chrome/browser/predictors/loading_predictor.h
+++ b/chrome/browser/predictors/loading_predictor.h
@@ -80,6 +80,7 @@
void PreconnectFinished(std::unique_ptr<PreconnectStats> stats) override;
size_t GetActiveHintsSizeForTesting() { return active_hints_.size(); }
+ size_t GetTotalHintsActivatedForTesting() { return total_hints_activated_; }
size_t GetActiveNavigationsSizeForTesting() {
return active_navigations_.size();
}
@@ -131,6 +132,7 @@
std::map<GURL, base::TimeTicks> active_hints_;
std::set<NavigationID> active_navigations_;
bool shutdown_ = false;
+ size_t total_hints_activated_ = 0;
GURL last_omnibox_origin_;
base::TimeTicks last_omnibox_preconnect_time_;
@@ -148,6 +150,10 @@
FRIEND_TEST_ALL_PREFIXES(LoadingPredictorTest,
TestMainFrameRequestDoesntCancelExternalHint);
FRIEND_TEST_ALL_PREFIXES(LoadingPredictorTest,
+ TestDuplicateHintAfterPreconnectCompleteCalled);
+ FRIEND_TEST_ALL_PREFIXES(LoadingPredictorTest,
+ TestDuplicateHintAfterPreconnectCompleteNotCalled);
+ FRIEND_TEST_ALL_PREFIXES(LoadingPredictorTest,
TestDontTrackNonPrefetchableUrls);
FRIEND_TEST_ALL_PREFIXES(LoadingPredictorTest, TestDontPredictOmniboxHints);
diff --git a/chrome/browser/predictors/loading_predictor_browsertest.cc b/chrome/browser/predictors/loading_predictor_browsertest.cc
index 3ff8f2f0..9cbd307 100644
--- a/chrome/browser/predictors/loading_predictor_browsertest.cc
+++ b/chrome/browser/predictors/loading_predictor_browsertest.cc
@@ -467,10 +467,14 @@
auto observer = NavigateToURLAsync(url);
EXPECT_TRUE(observer->WaitForRequestStart());
EXPECT_EQ(1u, loading_predictor()->GetActiveNavigationsSizeForTesting());
- EXPECT_EQ(1u, loading_predictor()->GetActiveHintsSizeForTesting());
+ // Checking GetActiveHintsSizeForTesting() is racy since the active hint
+ // is removed after the preconnect finishes. Instead check for total
+ // hints activated.
+ EXPECT_EQ(1u, loading_predictor()->GetTotalHintsActivatedForTesting());
observer->WaitForNavigationFinished();
EXPECT_EQ(0u, loading_predictor()->GetActiveNavigationsSizeForTesting());
EXPECT_EQ(0u, loading_predictor()->GetActiveHintsSizeForTesting());
+ EXPECT_EQ(1u, loading_predictor()->GetTotalHintsActivatedForTesting());
}
// Tests that two concurrenct navigations are recorded correctly by the
@@ -483,11 +487,15 @@
EXPECT_TRUE(observer1->WaitForRequestStart());
EXPECT_TRUE(observer2->WaitForRequestStart());
EXPECT_EQ(2u, loading_predictor()->GetActiveNavigationsSizeForTesting());
- EXPECT_EQ(2u, loading_predictor()->GetActiveHintsSizeForTesting());
+ // Checking GetActiveHintsSizeForTesting() is racy since the active hint
+ // is removed after the preconnect finishes. Instead check for total
+ // hints activated.
+ EXPECT_EQ(2u, loading_predictor()->GetTotalHintsActivatedForTesting());
observer1->WaitForNavigationFinished();
observer2->WaitForNavigationFinished();
EXPECT_EQ(0u, loading_predictor()->GetActiveNavigationsSizeForTesting());
EXPECT_EQ(0u, loading_predictor()->GetActiveHintsSizeForTesting());
+ EXPECT_EQ(2u, loading_predictor()->GetTotalHintsActivatedForTesting());
}
// Tests that two navigations to the same URL are deduplicated.
@@ -499,11 +507,19 @@
EXPECT_TRUE(observer1->WaitForRequestStart());
EXPECT_TRUE(observer2->WaitForRequestStart());
EXPECT_EQ(2u, loading_predictor()->GetActiveNavigationsSizeForTesting());
- EXPECT_EQ(1u, loading_predictor()->GetActiveHintsSizeForTesting());
+ // Checking GetActiveHintsSizeForTesting() is racy since the active hint
+ // is removed after the preconnect finishes. Instead check for total
+ // hints activated. The total hints activated may be only 1 if the second
+ // navigation arrives before the first preconnect finishes. However, if the
+ // second navigation arrives later, then two hints may get activated.
+ EXPECT_LE(1u, loading_predictor()->GetTotalHintsActivatedForTesting());
+ EXPECT_GE(2u, loading_predictor()->GetTotalHintsActivatedForTesting());
observer1->WaitForNavigationFinished();
observer2->WaitForNavigationFinished();
EXPECT_EQ(0u, loading_predictor()->GetActiveNavigationsSizeForTesting());
EXPECT_EQ(0u, loading_predictor()->GetActiveHintsSizeForTesting());
+ EXPECT_LE(1u, loading_predictor()->GetTotalHintsActivatedForTesting());
+ EXPECT_GE(2u, loading_predictor()->GetTotalHintsActivatedForTesting());
}
// Tests that the LoadingPredictor doesn't record non-http(s) navigations.
diff --git a/chrome/browser/predictors/loading_predictor_unittest.cc b/chrome/browser/predictors/loading_predictor_unittest.cc
index 022bf606..c168cd5 100644
--- a/chrome/browser/predictors/loading_predictor_unittest.cc
+++ b/chrome/browser/predictors/loading_predictor_unittest.cc
@@ -229,6 +229,64 @@
EXPECT_EQ(start_time, it->second);
}
+TEST_F(LoadingPredictorTest, TestDuplicateHintAfterPreconnectCompleteCalled) {
+ const GURL url = GURL(kUrl);
+ const auto& active_navigations = predictor_->active_navigations_;
+ auto& active_hints = predictor_->active_hints_;
+
+ predictor_->PrepareForPageLoad(url, HintOrigin::EXTERNAL);
+ auto it = active_hints.find(url);
+ EXPECT_NE(it, active_hints.end());
+ EXPECT_TRUE(active_navigations.empty());
+
+ // To check that the hint is replaced, set the start time in the past,
+ // and check later that it changed.
+ base::TimeTicks start_time = it->second - base::TimeDelta::FromSeconds(10);
+ it->second = start_time;
+
+ std::unique_ptr<PreconnectStats> preconnect_stats =
+ std::make_unique<PreconnectStats>(url);
+ predictor_->PreconnectFinished(std::move(preconnect_stats));
+
+ predictor_->PrepareForPageLoad(url, HintOrigin::NAVIGATION_PREDICTOR);
+ it = active_hints.find(url);
+ EXPECT_NE(it, active_hints.end());
+ EXPECT_TRUE(active_navigations.empty());
+
+ // Calling PreconnectFinished() must have cleared the hint, and duplicate
+ // PrepareForPageLoad() call should be honored.
+ EXPECT_LT(start_time, it->second);
+}
+
+TEST_F(LoadingPredictorTest,
+ TestDuplicateHintAfterPreconnectCompleteNotCalled) {
+ const GURL url = GURL(kUrl);
+ const auto& active_navigations = predictor_->active_navigations_;
+ auto& active_hints = predictor_->active_hints_;
+
+ predictor_->PrepareForPageLoad(url, HintOrigin::EXTERNAL);
+ auto it = active_hints.find(url);
+ EXPECT_NE(it, active_hints.end());
+ EXPECT_TRUE(active_navigations.empty());
+
+ content::RunAllTasksUntilIdle();
+ it = active_hints.find(url);
+ EXPECT_NE(it, active_hints.end());
+
+ // To check that the hint is not replaced, set the start time in the recent
+ // past, and check later that it didn't change.
+ base::TimeTicks start_time = it->second - base::TimeDelta::FromSeconds(10);
+ it->second = start_time;
+
+ predictor_->PrepareForPageLoad(url, HintOrigin::NAVIGATION_PREDICTOR);
+ it = active_hints.find(url);
+ EXPECT_NE(it, active_hints.end());
+ EXPECT_TRUE(active_navigations.empty());
+
+ // Duplicate PrepareForPageLoad() call should not be honored.
+ EXPECT_EQ(start_time, it->second);
+}
+
TEST_F(LoadingPredictorTest, TestDontTrackNonPrefetchableUrls) {
const GURL url3 = GURL(kUrl3);
predictor_->PrepareForPageLoad(url3, HintOrigin::NAVIGATION);
