[bfcache] Do not track bfcached navigations in PageLoadMetricsObservers.

As a first step of supporting back-forward cached navigations for
PageLoadMetricsObservers, do not track them at all.

Design doc:
https://docs.google.com/document/d/1VxjalqgiDvoAxQBGetBQi2m179UmiUWqftqq1XBRSWs/edit#heading=h.lacw135tkjlp
R=bmcquade@chromium.org,alexmos@chromium.org
BUG=1014174

Change-Id: Ic96914a5c72d7a2619948fb1f39e388f4311f004
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1860416Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705895}

components/page_load_metrics/browser/metrics_web_contents_observer.cc

@@ -776,6 +776,10 @@ bool MetricsWebContentsObserver::ShouldTrackNavigation(
       return false;
   }
 
+  // TODO(crbug.com/1014174): Ignore back-forward cached navigations for now.
+  if (navigation_handle->IsServedFromBackForwardCache())
+    return false;
+
   return true;
 }
 

components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc

@@ -10,17 +10,20 @@
 #include "base/memory/weak_ptr.h"
 #include "base/process/kill.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
 #include "base/timer/mock_timer.h"
 #include "components/page_load_metrics/browser/page_load_metrics_test_content_browser_client.h"
 #include "components/page_load_metrics/browser/page_load_tracker.h"
 #include "components/page_load_metrics/browser/test_metrics_web_contents_observer_embedder.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/resource_load_info.mojom.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/public/test/web_contents_tester.h"
 #include "net/base/net_errors.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::NavigationSimulator;
@@ -1475,4 +1478,50 @@ TEST_F(MetricsWebContentsObserverTest, RecordFeatureUsageNoObserver) {
   MetricsWebContentsObserver::RecordFeatureUsage(main_rfh(), features);
 }
 
+class MetricsWebContentsObserverBackForwardCacheTest
+    : public MetricsWebContentsObserverTest {
+ public:
+  MetricsWebContentsObserverBackForwardCacheTest() {
+    feature_list_.InitWithFeaturesAndParameters(
+        {{features::kBackForwardCache,
+          {{"TimeToLiveInBackForwardCacheInSeconds", "3600"}}}},
+        {});
+  }
+
+  ~MetricsWebContentsObserverBackForwardCacheTest() override {}
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(MetricsWebContentsObserverBackForwardCacheTest,
+       RecordFeatureUsageWithBackForwardCache) {
+  content::WebContentsTester* web_contents_tester =
+      content::WebContentsTester::For(web_contents());
+  web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
+  ASSERT_EQ(main_rfh()->GetLastCommittedURL().spec(), GURL(kDefaultTestUrl));
+
+  std::vector<blink::mojom::WebFeature> web_features1{
+      blink::mojom::WebFeature::kHTMLMarqueeElement};
+  mojom::PageLoadFeatures features1(web_features1, {}, {});
+  MetricsWebContentsObserver::RecordFeatureUsage(main_rfh(), features1);
+
+  web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl2));
+  content::NavigationSimulator::GoBack(web_contents());
+
+  std::vector<blink::mojom::WebFeature> web_features2{
+      blink::mojom::WebFeature::kFormAttribute};
+  mojom::PageLoadFeatures features2(web_features2, {}, {});
+  MetricsWebContentsObserver::RecordFeatureUsage(main_rfh(), features2);
+
+  std::vector<std::vector<blink::mojom::WebFeature>> features;
+  for (const auto& observation : observed_features()) {
+    features.push_back(observation.features);
+  }
+
+  // For now back-forward cached navigations are not tracked and the events
+  // after the history navigation are not tracked.
+  EXPECT_THAT(features, testing::ElementsAre(web_features1));
+}
+
 }  // namespace page_load_metrics

content/browser/frame_host/back_forward_cache_metrics.cc

@@ -193,7 +193,7 @@ void BackForwardCacheMetrics::RecordMetricsForHistoryNavigationCommit(
   // TODO(hajimehoshi): Use kNotCachedDueToExperimentCondition if the
   // experiment condition does not match.
   HistoryNavigationOutcome outcome = HistoryNavigationOutcome::kNotCached;
-  if (navigation->is_served_from_back_forward_cache()) {
+  if (navigation->IsServedFromBackForwardCache()) {
     outcome = HistoryNavigationOutcome::kRestored;
 
     UMA_HISTOGRAM_ENUMERATION(
@@ -207,7 +207,7 @@ void BackForwardCacheMetrics::RecordMetricsForHistoryNavigationCommit(
   // have a value.
   if (evicted_reason_.has_value() ||
       last_committed_main_frame_navigation_id_ == -1) {
-    DCHECK(!navigation->is_served_from_back_forward_cache());
+    DCHECK(!navigation->IsServedFromBackForwardCache());
     outcome = HistoryNavigationOutcome::kEvicted;
   }
   UMA_HISTOGRAM_ENUMERATION("BackForwardCache.HistoryNavigationOutcome",

content/browser/frame_host/navigation_request.cc

@@ -1155,7 +1155,7 @@ void NavigationRequest::BeginNavigation() {
   if (!NeedsUrlLoader()) {
     // The types of pages that don't need a URL Loader should never get served
     // from the BackForwardCache.
-    DCHECK(!is_served_from_back_forward_cache());
+    DCHECK(!IsServedFromBackForwardCache());
 
     // There is no need to make a network request for this navigation, so commit
     // it immediately.
@@ -1641,7 +1641,7 @@ void NavigationRequest::OnResponseStarted(
   }
 
   // Select an appropriate renderer to commit the navigation.
-  if (is_served_from_back_forward_cache()) {
+  if (IsServedFromBackForwardCache()) {
     NavigationControllerImpl* controller =
         static_cast<NavigationControllerImpl*>(
             frame_tree_node_->navigator()->GetController());
@@ -2141,7 +2141,7 @@ void NavigationRequest::OnStartChecksComplete(
           OriginPolicyThrottle::ShouldRequestOriginPolicy(common_params_->url)),
       std::move(navigation_ui_data), service_worker_handle_.get(),
       appcache_handle_.get(), std::move(prefetched_signed_exchange_cache_),
-      this, is_served_from_back_forward_cache(), std::move(interceptor));
+      this, IsServedFromBackForwardCache(), std::move(interceptor));
 
   DCHECK(!render_frame_host_);
 }
@@ -2367,7 +2367,7 @@ void NavigationRequest::CommitNavigation() {
   DCHECK(!common_params_->url.SchemeIs(url::kJavaScriptScheme));
   DCHECK(!IsRendererDebugURL(common_params_->url));
 
-  if (is_served_from_back_forward_cache()) {
+  if (IsServedFromBackForwardCache()) {
     NavigationControllerImpl* controller =
         static_cast<NavigationControllerImpl*>(
             frame_tree_node_->navigator()->GetController());
@@ -3771,6 +3771,10 @@ GlobalFrameRoutingId NavigationRequest::GetPreviousRenderFrameHostId() {
   return previous_render_frame_host_id_;
 }
 
+bool NavigationRequest::IsServedFromBackForwardCache() {
+  return rfh_restored_from_back_forward_cache_ != nullptr;
+}
+
 // static
 NavigationRequest* NavigationRequest::From(NavigationHandle* handle) {
   return static_cast<NavigationRequest*>(handle);

content/browser/frame_host/navigation_request.h

@@ -251,6 +251,7 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   void RegisterSubresourceOverride(
       mojom::TransferrableURLLoaderPtr transferrable_loader) override;
   GlobalFrameRoutingId GetPreviousRenderFrameHostId() override;
+  bool IsServedFromBackForwardCache() override;
 
   // Called on the UI thread by the Navigator to start the navigation.
   // The NavigationRequest can be deleted while BeginNavigation() is called.
@@ -465,11 +466,6 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
     response_headers_for_testing_ = response_headers;
   }
 
-  // Whether this navigation was served from the back-forward cache.
-  bool is_served_from_back_forward_cache() {
-    return rfh_restored_from_back_forward_cache_ != nullptr;
-  }
-
   RenderFrameHostImpl* rfh_restored_from_back_forward_cache() {
     return rfh_restored_from_back_forward_cache_;
   }

content/browser/frame_host/render_frame_host_manager.cc

@@ -655,7 +655,7 @@ void RenderFrameHostManager::ClearWebUIInstances() {
 
 void RenderFrameHostManager::DidCreateNavigationRequest(
     NavigationRequest* request) {
-  if (request->is_served_from_back_forward_cache()) {
+  if (request->IsServedFromBackForwardCache()) {
     // Since the frame from the back-forward cache is being committed to the
     // SiteInstance we already have, it is treated as current.
     request->set_associated_site_instance_type(

content/public/browser/navigation_handle.h

@@ -163,6 +163,9 @@ class CONTENT_EXPORT NavigationHandle {
   // handlers.
   virtual bool IsExternalProtocol() = 0;
 
+  // Whether the navigation is restoring a page from back-forward cache.
+  virtual bool IsServedFromBackForwardCache() = 0;
+
   // Navigation control flow --------------------------------------------------
 
   // The net error code if an error happened prior to commit. Otherwise it will

content/public/test/mock_navigation_handle.h

@@ -37,6 +37,7 @@ class MockNavigationHandle : public NavigationHandle {
   bool IsRendererInitiated() override { return true; }
   MOCK_METHOD0(GetFrameTreeNodeId, int());
   MOCK_METHOD0(GetPreviousRenderFrameHostId, GlobalFrameRoutingId());
+  bool IsServedFromBackForwardCache() override { return false; }
   RenderFrameHost* GetParentFrame() override {
     return render_frame_host_ ? render_frame_host_->GetParent() : nullptr;
   }