[ch-r] Parse ACCEPT_CH H2/3 frame and restart with new headers if needed

Part of Client Hints Reliability:
https://github.com/WICG/client-hints-infrastructure/blob/master/reliability.md

 * `accept_ch_frame` in TransportInfo gets parsed and (if headers are
   found that weren't already in the request) sent to the browser
   process (via NetworkServiceClient::OnConnected) to generate header
   values and check browser-level requirements for sending client hints
   (e.g. JS is enabled)

 * If there are new headers to add, they get sent back (through
   URLLoader to make sure no unsafe headers were sent) to URLRequest,
   where the headers are added and the request is restarted. If the
   request isn't restarted, the callback from HttpNetworkTransaction is
   called to get the event loop started again.

Bug: 1168489
Change-Id: I79cca460824ee2ed3f3447898940da71d2444125
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2733595
Commit-Queue: Aaron Tagliaboschi <aarontag@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#869055}

chrome/browser/client_hints/client_hints_browsertest.cc

@@ -48,9 +48,11 @@
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "services/network/public/cpp/client_hints.h"
 #include "services/network/public/cpp/cors/cors.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/network_switches.h"
+#include "services/network/public/mojom/web_client_hints_types.mojom-shared.h"
 #include "third_party/blink/public/common/client_hints/client_hints.h"
 #include "third_party/blink/public/common/web_preferences/web_preferences.h"
 
@@ -260,7 +262,9 @@ class ClientHintsBrowserTest : public policy::PolicyTest,
   virtual std::unique_ptr<base::FeatureList> EnabledFeatures() {
     std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
     feature_list->InitializeFromCommandLine(
-        "UserAgentClientHint,LangClientHintHeader", "");
+        "UserAgentClientHint,LangClientHintHeader,CriticalClientHint,"
+        "AcceptCHFrame",
+        "");
     return feature_list;
   }
 
@@ -2141,3 +2145,107 @@ IN_PROC_BROWSER_TEST_F(ClientHintsWebHoldbackBrowserTest,
   EXPECT_EQ(0u, third_party_request_count_seen());
   EXPECT_EQ(0u, third_party_client_hints_count_seen());
 }
+
+class AcceptCHFrameObserverInterceptor {
+ public:
+  AcceptCHFrameObserverInterceptor()
+      : interceptor_(base::BindRepeating(
+            &AcceptCHFrameObserverInterceptor::InterceptURLRequest,
+            base::Unretained(this))) {}
+
+  void set_accept_ch_frame(
+      std::vector<network::mojom::WebClientHintsType> frame) {
+    accept_ch_frame_ = frame;
+  }
+
+ private:
+  bool InterceptURLRequest(
+      content::URLLoaderInterceptor::RequestParams* params) {
+    if (!accept_ch_frame_ || !params->url_request.trusted_params ||
+        !params->url_request.trusted_params->accept_ch_frame_observer) {
+      return false;
+    }
+
+    std::vector<network::mojom::WebClientHintsType> hints;
+    for (auto hint : accept_ch_frame_.value()) {
+      std::string header =
+          network::kClientHintsNameMapping[static_cast<int>(hint)];
+      if (!params->url_request.headers.HasHeader(header))
+        hints.push_back(hint);
+    }
+
+    if (hints.empty())
+      return false;
+
+    mojo::Remote<network::mojom::AcceptCHFrameObserver> remote(std::move(
+        params->url_request.trusted_params->accept_ch_frame_observer));
+    remote->OnAcceptCHFrameReceived(params->url_request.url, hints,
+                                    base::DoNothing::Once<int>());
+    // At this point it's expected that either the remote's callback will be
+    // called or the URLLoader will be destroyed to make way for a new one.
+    // As this is essentially unobservable, RunUntilIdle must be used.
+    base::RunLoop().RunUntilIdle();
+    return false;
+  }
+
+  content::URLLoaderInterceptor interceptor_;
+  base::Optional<std::vector<network::mojom::WebClientHintsType>>
+      accept_ch_frame_;
+};
+
+// Replace the request interceptor with an AcceptCHFrameObserverInterceptor.
+class ClientHintsAcceptCHFrameObserverBrowserTest
+    : public ClientHintsBrowserTest {
+ public:
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    accept_ch_frame_observer_interceptor_ =
+        std::make_unique<AcceptCHFrameObserverInterceptor>();
+  }
+
+  void TearDownOnMainThread() override {
+    accept_ch_frame_observer_interceptor_.reset();
+  }
+
+  void set_accept_ch_frame(
+      std::vector<network::mojom::WebClientHintsType> frame) {
+    accept_ch_frame_observer_interceptor_->set_accept_ch_frame(frame);
+  }
+
+  std::vector<network::mojom::WebClientHintsType> all_client_hints_types() {
+    std::vector<network::mojom::WebClientHintsType> hints;
+    for (size_t i = 0; i < blink::kClientHintsMappingsCount; i++) {
+      hints.push_back(static_cast<network::mojom::WebClientHintsType>(i));
+    }
+
+    return hints;
+  }
+
+ private:
+  std::unique_ptr<AcceptCHFrameObserverInterceptor>
+      accept_ch_frame_observer_interceptor_;
+};
+
+// Ensure that client hints are sent when the ACCEPT_CH frame observer is
+// notified.
+IN_PROC_BROWSER_TEST_F(ClientHintsAcceptCHFrameObserverBrowserTest,
+                       AcceptCHFrame) {
+  const GURL gurl = without_accept_ch_without_lifetime_url();
+  set_accept_ch_frame(all_client_hints_types());
+  SetClientHintExpectationsOnMainFrame(true);
+  SetClientHintExpectationsOnSubresources(false);
+  ui_test_utils::NavigateToURL(browser(), gurl);
+}
+
+// Ensure that client hints are *not* sent when the observer is notified but
+// client hints would normally not be sent (e.g. when JS is disabled for the
+// frame).
+IN_PROC_BROWSER_TEST_F(ClientHintsAcceptCHFrameObserverBrowserTest,
+                       AcceptCHFrameJSDisabled) {
+  const GURL gurl = without_accept_ch_without_lifetime_url();
+  set_accept_ch_frame(all_client_hints_types());
+  SetJsEnabledForActiveView(false);
+  SetClientHintExpectationsOnMainFrame(false);
+  SetClientHintExpectationsOnSubresources(false);
+  ui_test_utils::NavigateToURL(browser(), gurl);
+}

components/client_hints/browser/client_hints.cc

@@ -52,6 +52,8 @@ void ClientHints::GetAllowedClientHintsFromSource(
                                        &client_hints_rules);
   client_hints::GetAllowedClientHintsFromSource(url, client_hints_rules,
                                                 client_hints);
+  for (auto hint : additional_hints_)
+    client_hints->SetIsEnabled(hint, true);
 }
 
 bool ClientHints::IsJavaScriptAllowed(const GURL& url) {
@@ -149,4 +151,13 @@ void ClientHints::PersistClientHints(
   UMA_HISTOGRAM_COUNTS_100("ClientHints.UpdateSize", client_hints.size());
 }
 
+void ClientHints::SetAdditionalClientHints(
+    const std::vector<network::mojom::WebClientHintsType>& hints) {
+  additional_hints_ = hints;
+}
+
+void ClientHints::ClearAdditionalClientHints() {
+  additional_hints_.clear();
+}
+
 }  // namespace client_hints

components/client_hints/browser/client_hints.h

@@ -48,12 +48,18 @@ class ClientHints : public KeyedService,
       const std::vector<network::mojom::WebClientHintsType>& client_hints,
       base::TimeDelta expiration_duration) override;
 
+  void SetAdditionalClientHints(
+      const std::vector<network::mojom::WebClientHintsType>&) override;
+
+  void ClearAdditionalClientHints() override;
+
  private:
   content::BrowserContext* context_ = nullptr;
   network::NetworkQualityTracker* network_quality_tracker_ = nullptr;
   HostContentSettingsMap* settings_map_ = nullptr;
   blink::UserAgentMetadata user_agent_metadata_;
   PrefService* pref_service_;
+  std::vector<network::mojom::WebClientHintsType> additional_hints_;
 
   DISALLOW_COPY_AND_ASSIGN(ClientHints);
 };

content/browser/client_hints/critical_client_hints_throttle.cc

@@ -5,17 +5,19 @@
 #include "content/browser/client_hints/critical_client_hints_throttle.h"
 
 #include "base/feature_list.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_functions.h"
 #include "content/browser/client_hints/client_hints.h"
 #include "content/browser/renderer_host/frame_tree_node.h"
 #include "content/browser/renderer_host/navigation_request.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/client_hints_controller_delegate.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_features.h"
 #include "net/http/http_util.h"
 #include "services/network/public/cpp/client_hints.h"
+#include "services/network/public/cpp/features.h"
 #include "services/network/public/mojom/parsed_headers.mojom-forward.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
@@ -24,7 +26,11 @@
 namespace {
 
 void LogCriticalCHStatus(CriticalCHRestart status) {
-  UMA_HISTOGRAM_ENUMERATION("ClientHints.CriticalCHRestart", status);
+  base::UmaHistogramEnumeration("ClientHints.CriticalCHRestart", status);
+}
+
+void LogAcceptCHFrameStatus(AcceptCHFrameRestart status) {
+  base::UmaHistogramEnumeration("ClientHints.AcceptCHFrame", status);
 }
 
 }  // namespace
@@ -45,8 +51,10 @@ void CriticalClientHintsThrottle::WillProcessResponse(
     const GURL& response_url,
     network::mojom::URLResponseHead* response_head,
     bool* defer) {
-  if (redirected_)
+  if (critical_redirect_ ||
+      !base::FeatureList::IsEnabled(features::kCriticalClientHint)) {
     return;
+  }
 
   if (!response_head->parsed_headers ||
       !response_head->parsed_headers->accept_ch ||
@@ -69,25 +77,60 @@ void CriticalClientHintsThrottle::WillProcessResponse(
 
   LogCriticalCHStatus(CriticalCHRestart::kHeaderPresent);
 
+  net::HttpRequestHeaders modified_headers;
+  if (ShouldRestartWithHints(response_url, critical_hints, modified_headers)) {
+    critical_redirect_ = true;
+    LogCriticalCHStatus(CriticalCHRestart::kNavigationRestarted);
+    delegate_->RestartWithModifiedHeadersNow(modified_headers);
+  }
+}
+
+void CriticalClientHintsThrottle::HandleAcceptCHFrameReceived(
+    const GURL& url,
+    const std::vector<network::mojom::WebClientHintsType>& accept_ch_frame) {
+  if (accept_ch_frame_redirect_ ||
+      !base::FeatureList::IsEnabled(network::features::kAcceptCHFrame)) {
+    return;
+  }
+
+  LogAcceptCHFrameStatus(AcceptCHFrameRestart::kFramePresent);
+
+  net::HttpRequestHeaders modified_headers;
+
+  if (ShouldRestartWithHints(url, accept_ch_frame, modified_headers)) {
+    accept_ch_frame_redirect_ = true;
+    LogAcceptCHFrameStatus(AcceptCHFrameRestart::kNavigationRestarted);
+    delegate_->RestartWithModifiedHeadersNow(modified_headers);
+  }
+}
+
+bool CriticalClientHintsThrottle::ShouldRestartWithHints(
+    const GURL& response_url,
+    const std::vector<network::mojom::WebClientHintsType>& hints,
+    net::HttpRequestHeaders& modified_headers) {
   FrameTreeNode* frame_tree_node =
       FrameTreeNode::GloballyFindByID(frame_tree_node_id_);
 
-  if (AreCriticalHintsMissing(response_url, frame_tree_node,
-                              client_hint_delegate_, critical_hints)) {
-    redirected_ = true;
-    auto parsed = ParseAndPersistAcceptCHForNagivation(
-        response_url, response_head->parsed_headers, context_,
-        client_hint_delegate_, frame_tree_node);
+  if (!AreCriticalHintsMissing(response_url, frame_tree_node,
+                               client_hint_delegate_, hints)) {
+    return false;
+  }
 
-    net::HttpRequestHeaders modified_headers;
+  client_hint_delegate_->SetAdditionalClientHints(hints);
+  // TODO(crbug.com/1195034): If the frame tree node doesn't have an associated
+  // navigation_request (e.g. a service worker request) it might not override
+  // the user agent correctly.
+  if (frame_tree_node) {
     AddNavigationRequestClientHintsHeaders(
         response_url, &modified_headers, context_, client_hint_delegate_,
         frame_tree_node->navigation_request()->GetIsOverridingUserAgent(),
         frame_tree_node);
-
-    LogCriticalCHStatus(CriticalCHRestart::kNavigationRestarted);
-
-    delegate_->RestartWithModifiedHeadersNow(modified_headers);
+  } else {
+    AddPrefetchNavigationRequestClientHintsHeaders(
+        response_url, &modified_headers, context_, client_hint_delegate_,
+        /*is_ua_override_on=*/false, /*is_javascript_enabled=*/true);
   }
+  client_hint_delegate_->ClearAdditionalClientHints();
+  return true;
 }
 }  // namespace content

content/browser/client_hints/critical_client_hints_throttle.h

@@ -19,6 +19,14 @@ enum class CriticalCHRestart {
   kMaxValue = kNavigationRestarted,
 };
 
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class AcceptCHFrameRestart {
+  kFramePresent = 0,
+  kNavigationRestarted = 1,
+  kMaxValue = kNavigationRestarted,
+};
+
 }  // namespace
 
 namespace content {
@@ -39,15 +47,35 @@ class CriticalClientHintsThrottle : public blink::URLLoaderThrottle {
                            network::mojom::URLResponseHead* response_head,
                            bool* defer) override;
 
+  void HandleAcceptCHFrameReceived(
+      const GURL& url,
+      const std::vector<network::mojom::WebClientHintsType>& accept_ch_frame)
+      override;
+
  private:
+  // Returns true if the extra `hints` are both not stored in the client hints
+  // preferences and allowed to be sent to the given URL (from the given frame
+  // tree node and so on). If returning true, the new name and values are added
+  // to |modified_headers|.
+  bool ShouldRestartWithHints(
+      const GURL& url,
+      const std::vector<network::mojom::WebClientHintsType>& hints,
+      net::HttpRequestHeaders& modified_headers);
+
   BrowserContext* context_;
   ClientHintsControllerDelegate* client_hint_delegate_;
   int frame_tree_node_id_;
-  // This ensures the navigation doesn't turn into an infinite loop (this
-  // object should stay alive until the navigation is committed). On finding a
-  // critical client hint is missing, the throttle instigates an internal
-  // redirect.
-  bool redirected_ = false;
+
+  // Both the ACCEPT_CH frame and the Critical-CH header should only restart a
+  // navigation once. Once a redirect is triggered, the `*_redirect_` flag for
+  // the feature is set to true. These ensure the navigation doesn't turn into
+  // an infinite loop for their respective features (this object should stay
+  // alive until the navigation is committed).
+  //
+  // Redirect flag for the Critical-CH header.
+  bool critical_redirect_ = false;
+  // Redirect flag for the ACCEPT_CH h2/3 frame.
+  bool accept_ch_frame_redirect_ = false;
 };
 
 }  // namespace content

content/browser/loader/navigation_url_loader_impl_unittest.cc

@@ -107,7 +107,8 @@ class TestNavigationLoaderInterceptor : public NavigationLoaderInterceptor {
         nullptr /* trust_token_helper */, kEmptyOriginAccessList,
         mojo::NullRemote() /* cookie_observer */,
         mojo::NullRemote() /* url_loader_network_observer */,
-        /*devtools_observer=*/mojo::NullRemote());
+        /*devtools_observer=*/mojo::NullRemote(),
+        /*accept_ch_frame_observer=*/mojo::NullRemote());
   }
 
   bool MaybeCreateLoaderForResponse(

content/browser/loader/url_loader_throttles.cc

@@ -20,6 +20,7 @@
 #include "net/http/http_request_headers.h"
 #include "net/http/http_util.h"
 #include "services/network/public/cpp/client_hints.h"
+#include "services/network/public/cpp/features.h"
 #include "services/network/public/mojom/parsed_headers.mojom-forward.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
@@ -47,7 +48,8 @@ CreateContentBrowserURLLoaderThrottles(
   ClientHintsControllerDelegate* client_hint_delegate =
       browser_context->GetClientHintsControllerDelegate();
   if (base::FeatureList::IsEnabled(features::kFeaturePolicyForClientHints) &&
-      base::FeatureList::IsEnabled(features::kCriticalClientHint) &&
+      (base::FeatureList::IsEnabled(features::kCriticalClientHint) ||
+       base::FeatureList::IsEnabled(network::features::kAcceptCHFrame)) &&
       request.is_main_frame && net::HttpUtil::IsMethodSafe(request.method) &&
       client_hint_delegate &&
       ShouldAddClientHints(request.url,

content/public/browser/client_hints_controller_delegate.h

@@ -51,7 +51,21 @@ class CONTENT_EXPORT ClientHintsControllerDelegate {
       const std::vector<network::mojom::WebClientHintsType>& client_hints,
       base::TimeDelta expiration_duration) = 0;
 
+  // Optionally implemented by implementations used in tests. Clears all hints
+  // that would have been returned by GetAllowedClientHintsFromSource(),
+  // regardless of whether they were added via PersistClientHints() or
+  // SetAdditionalHints().
   virtual void ResetForTesting() {}
+
+  // Sets additional `hints` that this delegate should add to the
+  // blink::WebEnabledClientHints object affected by
+  // |GetAllowedClientHintsFromSource|. This is for when there are additional
+  // client hints to be added to a request that are not in storage.
+  virtual void SetAdditionalClientHints(
+      const std::vector<network::mojom::WebClientHintsType>&) = 0;
+
+  // Clears the additional hints set by |SetAdditionalHints|.
+  virtual void ClearAdditionalClientHints() = 0;
 };
 
 }  // namespace content