Use correct UKM source ID in WebRequestProxyingURLLoaderFactory

Currently we get a UKM source ID from the frame in
WebRequestAPI::MaybeProxyURLLoaderFactory but it's too early - the frame
has not been fully committed yet, and the UKM source ID for the previous
URL is returned.

Get the UKM source ID in
WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart, to work around
the problem.

Bug: 1119320
Change-Id: I1fdd8d301aad916b11eef50e521d7f42087ed41c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362071Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799948}

extensions/browser/api/web_request/web_request_api.cc

@@ -744,15 +744,13 @@ bool WebRequestAPI::MaybeProxyURLLoaderFactory(
          (browser_context->IsOffTheRecord() &&
           ExtensionsBrowserClient::Get()->GetOriginalContext(browser_context) ==
               browser_context_));
-  const ukm::SourceId ukm_source_id =
-      web_contents ? ukm::GetSourceIdForWebContentsDocument(web_contents)
-                   : ukm::kInvalidSourceId;
+  const int frame_id = frame ? frame->GetRoutingID() : MSG_ROUTING_NONE;
   WebRequestProxyingURLLoaderFactory::StartProxying(
-      browser_context, is_navigation ? -1 : render_process_id,
+      browser_context, is_navigation ? -1 : render_process_id, frame_id,
       &request_id_generator_, std::move(navigation_ui_data),
       std::move(navigation_id), std::move(proxied_receiver),
       std::move(target_factory_remote), std::move(header_client_receiver),
-      proxies_.get(), type, ukm_source_id);
+      proxies_.get(), type);
   return true;
 }
 

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

@@ -14,10 +14,12 @@
 #include "base/strings/stringprintf.h"
 #include "base/task/post_task.h"
 #include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
+#include "components/ukm/content/source_url_recorder.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/global_request_id.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/common/url_utils.h"
 #include "extensions/browser/api/web_request/permission_helper.h"
 #include "extensions/browser/extension_navigation_ui_data.h"
@@ -1094,6 +1096,7 @@ bool WebRequestProxyingURLLoaderFactory::InProgressRequest::IsRedirectSafe(
 WebRequestProxyingURLLoaderFactory::WebRequestProxyingURLLoaderFactory(
     content::BrowserContext* browser_context,
     int render_process_id,
+    int frame_id,
     WebRequestAPI::RequestIDGenerator* request_id_generator,
     std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data,
     base::Optional<int64_t> navigation_id,
@@ -1102,16 +1105,15 @@ WebRequestProxyingURLLoaderFactory::WebRequestProxyingURLLoaderFactory(
     mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient>
         header_client_receiver,
     WebRequestAPI::ProxySet* proxies,
-    content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type,
-    ukm::SourceId ukm_source_id)
+    content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type)
     : browser_context_(browser_context),
       render_process_id_(render_process_id),
+      frame_id_(frame_id),
       request_id_generator_(request_id_generator),
       navigation_ui_data_(std::move(navigation_ui_data)),
       navigation_id_(std::move(navigation_id)),
       proxies_(proxies),
-      loader_factory_type_(loader_factory_type),
-      ukm_source_id_(ukm_source_id) {
+      loader_factory_type_(loader_factory_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // base::Unretained is safe here because the callback will be
   // canceled when |shutdown_notifier_| is destroyed, and |proxies_|
@@ -1138,6 +1140,7 @@ WebRequestProxyingURLLoaderFactory::WebRequestProxyingURLLoaderFactory(
 void WebRequestProxyingURLLoaderFactory::StartProxying(
     content::BrowserContext* browser_context,
     int render_process_id,
+    int frame_id,
     WebRequestAPI::RequestIDGenerator* request_id_generator,
     std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data,
     base::Optional<int64_t> navigation_id,
@@ -1146,16 +1149,14 @@ void WebRequestProxyingURLLoaderFactory::StartProxying(
     mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient>
         header_client_receiver,
     WebRequestAPI::ProxySet* proxies,
-    content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type,
-    ukm::SourceId ukm_source_id) {
+    content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   auto proxy = std::make_unique<WebRequestProxyingURLLoaderFactory>(
-      browser_context, render_process_id, request_id_generator,
+      browser_context, render_process_id, frame_id, request_id_generator,
       std::move(navigation_ui_data), std::move(navigation_id),
       std::move(loader_receiver), std::move(target_factory_remote),
-      std::move(header_client_receiver), proxies, loader_factory_type,
-      ukm_source_id);
+      std::move(header_client_receiver), proxies, loader_factory_type);
 
   proxies->AddProxy(std::move(proxy));
 }
@@ -1170,6 +1171,19 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart(
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  if (!ukm_source_id_) {
+    auto* frame =
+        content::RenderFrameHost::FromID(render_process_id_, frame_id_);
+    using FactoryType = content::ContentBrowserClient::URLLoaderFactoryType;
+    if (loader_factory_type_ == FactoryType::kDocumentSubResource ||
+        loader_factory_type_ == FactoryType::kWorkerSubResource) {
+      ukm_source_id_ =
+          frame ? frame->GetPageUkmSourceId() : ukm::kInvalidSourceId;
+    } else {
+      ukm_source_id_ = ukm::kInvalidSourceId;
+    }
+  }
+
   // Make sure we are not proxying a browser initiated non-navigation
   // request except for loading service worker scripts.
   DCHECK(render_process_id_ != -1 || navigation_ui_data_ ||
@@ -1198,7 +1212,7 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart(
   auto result = requests_.emplace(
       web_request_id, std::make_unique<InProgressRequest>(
                           this, web_request_id, request_id, routing_id, options,
-                          ukm_source_id_, request, traffic_annotation,
+                          *ukm_source_id_, request, traffic_annotation,
                           std::move(loader_receiver), std::move(client)));
   result.first->second->Restart();
 }

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h

@@ -238,6 +238,7 @@ class WebRequestProxyingURLLoaderFactory
   WebRequestProxyingURLLoaderFactory(
       content::BrowserContext* browser_context,
       int render_process_id,
+      int frame_id,
       WebRequestAPI::RequestIDGenerator* request_id_generator,
       std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data,
       base::Optional<int64_t> navigation_id,
@@ -247,14 +248,14 @@ class WebRequestProxyingURLLoaderFactory
       mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient>
           header_client_receiver,
       WebRequestAPI::ProxySet* proxies,
-      content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type,
-      ukm::SourceId ukm_source_id);
+      content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type);
 
   ~WebRequestProxyingURLLoaderFactory() override;
 
   static void StartProxying(
       content::BrowserContext* browser_context,
       int render_process_id,
+      int frame_id,
       WebRequestAPI::RequestIDGenerator* request_id_generator,
       std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data,
       base::Optional<int64_t> navigation_id,
@@ -264,8 +265,7 @@ class WebRequestProxyingURLLoaderFactory
       mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient>
           header_client_receiver,
       WebRequestAPI::ProxySet* proxies,
-      content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type,
-      ukm::SourceId ukm_source_id);
+      content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type);
 
   // network::mojom::URLLoaderFactory:
   void CreateLoaderAndStart(
@@ -313,6 +313,7 @@ class WebRequestProxyingURLLoaderFactory
 
   content::BrowserContext* const browser_context_;
   const int render_process_id_;
+  const int frame_id_;
   WebRequestAPI::RequestIDGenerator* const request_id_generator_;
   std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data_;
   base::Optional<int64_t> navigation_id_;
@@ -325,9 +326,13 @@ class WebRequestProxyingURLLoaderFactory
 
   const content::ContentBrowserClient::URLLoaderFactoryType
       loader_factory_type_;
-  // A UKM source ID associated with the content::WebContents of the initiator
-  // frame.
-  const ukm::SourceId ukm_source_id_;
+  // A UKM source ID associated with the initiator frame.
+  // This is non-const and optional, because at the construction timing the
+  // frame hasn't been fully committed yet. See
+  // https://groups.google.com/a/chromium.org/forum/#!msg/navigation-dev/zsEzPKm-COk/N7k58Fa8BgAJ.
+  // TODO(yhirano): Make this const non-optional when it is fixed.
+  // This is calculated correctly only for factories for subresource requests.
+  base::Optional<ukm::SourceId> ukm_source_id_;
 
   // Mapping from our own internally generated request ID to an
   // InProgressRequest instance.