New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 841309 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug
Proj-Servicification

Blocking:
issue 840359



Sign in to add a comment

Make removal of variations headers on redirect work with Network Service

Project Member Reported by morlovich@chromium.org, May 9 2018

Issue description

Blocking: 840359

Comment 2 by dxie@chromium.org, May 14 2018

Labels: -Pri-3 Proj-Servicification-Canary OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
Status: Available (was: Untriaged)

Comment 3 by mmenke@chromium.org, May 16 2018

Components: Internals>Metrics>Variations

Comment 4 by juncai@chromium.org, May 18 2018

Owner: juncai@chromium.org
Status: Assigned (was: Available)

Comment 5 by juncai@chromium.org, May 18 2018

Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ddbd3c1db32d386dc1a3b51d9b890e2ab4855e3c

commit ddbd3c1db32d386dc1a3b51d9b890e2ab4855e3c
Author: Jun Cai <juncai@chromium.org>
Date: Tue Jun 12 17:24:13 2018

Network Service: Add VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromSubresourceRequest

This CL adds variations http headers browser test from subresources.

Bug:  841309 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I41c07393d6ba1e214bceb6b8e3ffa31c503d377f
Reviewed-on: https://chromium-review.googlesource.com/1096299
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566479}
[modify] https://crrev.com/ddbd3c1db32d386dc1a3b51d9b890e2ab4855e3c/chrome/browser/net/variations_http_headers_browsertest.cc
[modify] https://crrev.com/ddbd3c1db32d386dc1a3b51d9b890e2ab4855e3c/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/605ff0e70d6ab6dd97b6c0df236b12d66c51037c

commit 605ff0e70d6ab6dd97b6c0df236b12d66c51037c
Author: Jun Cai <juncai@chromium.org>
Date: Tue Jun 12 22:29:20 2018

Network Service: Fix VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromResourceRequest

Some major changes in this CL:
1. Adds an extra parameter "array<string>? to_be_removed_request_headers" to
URLLoader::FollowRedirect() so that the browser can pass in this information.
2. Adds a ModifyHeaderURLLoaderThrottle class which can add http headers through WillStartRequest()
and remove http headers through WillRedirectRequest().
3. Adds an extra parameter "std::vector<std::sgring>* to_be_removed_headers" to the
WillRedirectRequest() so that subclass can pass in the http headers to be removed.
4. Adds a member variable |to_be_removed_request_headers_| to ThrottlingURLLoader so that
ThrottlingURLLoader::OnReceiveRedirect() can store the to be removed headers there, and
ThrottlingURLLoader::FollowRedirect() can use that to remove headers by calling
URLLoader::FollowRedirect().

The design doc is at:
https://docs.google.com/document/d/1XAa0V_lKmiZWW7eBCcqdr4DCJSrPjhnvuqispxBGkUw/edit?pli=1

Bug:  841309 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I99b80811986714c21f17833b9ccb438f175d9939
Reviewed-on: https://chromium-review.googlesource.com/1086219
Commit-Queue: Jun Cai <juncai@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566607}
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/browser/extensions/chrome_url_request_util.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/browser/offline_pages/offline_page_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/browser/offline_pages/offline_page_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/common/BUILD.gn
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/common/DEPS
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/common/prerender_url_loader_throttle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/common/prerender_url_loader_throttle.h
[add] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/common/variations_header_url_loader_throttle.cc
[add] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/chrome/common/variations_header_url_loader_throttle.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/download/internal/common/resource_downloader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/safe_browsing/browser/base_parallel_resource_throttle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/safe_browsing/browser/browser_url_loader_throttle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/safe_browsing/browser/browser_url_loader_throttle.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/safe_browsing/renderer/renderer_url_loader_throttle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/safe_browsing/renderer/renderer_url_loader_throttle.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/subresource_filter/content/common/ad_delay_throttle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/components/subresource_filter/content/common/ad_delay_throttle.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/appcache/appcache_url_loader_job.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/appcache/appcache_url_loader_job.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/devtools/devtools_url_loader_interceptor.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/file_url_loader_factory.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/fileapi/file_system_url_loader_factory.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/navigation_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/navigation_url_loader_impl.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/navigation_url_loader_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/prefetch_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/loader/prefetch_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_installed_script_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_installed_script_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_navigation_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/service_worker/service_worker_new_script_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/shared_worker/shared_worker_script_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/shared_worker/shared_worker_script_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/web_package/signed_exchange_cert_fetcher_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/web_package/web_package_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/browser/web_package/web_package_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/common/throttling_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/common/throttling_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/common/throttling_url_loader_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/public/common/url_loader_throttle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/public/common/url_loader_throttle.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/renderer/fetchers/resource_fetcher_impl.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/renderer/loader/child_url_loader_factory_bundle.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/test/test_navigation_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/content/test/test_navigation_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/cors/cors_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/cors/cors_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/network_service_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/public/cpp/simple_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/public/cpp/simple_url_loader_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/public/mojom/url_loader.mojom
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/test/test_url_loader_factory.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/services/network/url_loader_unittest.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/storage/browser/blob/blob_url_loader.cc
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/storage/browser/blob/blob_url_loader.h
[modify] https://crrev.com/605ff0e70d6ab6dd97b6c0df236b12d66c51037c/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/627fd97ad671f7157584a311c6ad3f2cf49c4afe

commit 627fd97ad671f7157584a311c6ad3f2cf49c4afe
Author: Jun Cai <juncai@chromium.org>
Date: Mon Jun 18 19:32:46 2018

Network Service: Add VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromRequestUsingSimpleURLLoader

This CL adds variations http headers browser which uses
SimpleURLLoader.

Bug:  841309 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I16bc257c18b76f0b2ba6b9ba721b87f4e3fb44a7
Reviewed-on: https://chromium-review.googlesource.com/1100106
Commit-Queue: Jun Cai <juncai@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568110}
[modify] https://crrev.com/627fd97ad671f7157584a311c6ad3f2cf49c4afe/chrome/browser/net/variations_http_headers_browsertest.cc
[modify] https://crrev.com/627fd97ad671f7157584a311c6ad3f2cf49c4afe/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

On https://chromium-review.googlesource.com/1103451, there was an observation that variations headers appending does not seem to work with network service (i.e. there are no headers added).

I don't know the details of the network service, but I can offer some discussion.

First, these headers represent the field trial state known to the browser process. So if the code is now running in a different process, that state won't be available. The code may still *run* but because no variations ids have gotten registered with the singleton, nothing is appended.

So in such a case, we need some kind of cross-process solution. For example, a mojo call to update the singleton from the browser process when variations ids are registered. Or vice versa - for the service to query the browser for the ids when it needs them.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/420501d523ef6bb6ea6917879a2644d9caad1867

commit 420501d523ef6bb6ea6917879a2644d9caad1867
Author: Jun Cai <juncai@chromium.org>
Date: Tue Jun 26 18:31:04 2018

Network Service: Recategorize VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromInternalRequest

This CL recategorizes VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromInternalRequest
to a URLFetcher related bug.

Bug:  841309 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ie90d7c364358da31e5e66472c54a80a6d05ab074
Reviewed-on: https://chromium-review.googlesource.com/1113943
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570473}
[modify] https://crrev.com/420501d523ef6bb6ea6917879a2644d9caad1867/chrome/browser/net/variations_http_headers_browsertest.cc
[modify] https://crrev.com/420501d523ef6bb6ea6917879a2644d9caad1867/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6260d6caff3f410586afb03191f721b308357941

commit 6260d6caff3f410586afb03191f721b308357941
Author: Jun Cai <juncai@chromium.org>
Date: Tue Jun 26 23:29:08 2018

Network Service: Fix VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromRequestUsingSimpleURLLoader*

This CL adds a SimpleURLLoader wrapper CreateSimpleURLLoaderForVariations()
so that it can add and remove headers for variations.

Bug:  841309 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I45dcfa0d820168365cc834ae7af2cb37adfe0449
Reviewed-on: https://chromium-review.googlesource.com/1107208
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570580}
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/android/customtabs/detached_resource_request.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/android/customtabs/detached_resource_request.h
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/media/router/discovery/dial/dial_url_fetcher.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/media/router/discovery/dial/dial_url_fetcher.h
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/net/variations_http_headers_browsertest.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/ssl/common_name_mismatch_handler.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/chrome/browser/ssl/common_name_mismatch_handler.h
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/net_log/net_export_file_writer_unittest.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/payments/core/payment_manifest_downloader.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/payments/core/payment_manifest_downloader.h
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/payments/core/payment_manifest_downloader_unittest.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/variations/net/BUILD.gn
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/variations/net/DEPS
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/variations/net/variations_http_headers.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/components/variations/net/variations_http_headers.h
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/services/network/cors/preflight_controller.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/services/network/public/cpp/simple_url_loader.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/services/network/public/cpp/simple_url_loader.h
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/services/network/public/cpp/simple_url_loader_unittest.cc
[modify] https://crrev.com/6260d6caff3f410586afb03191f721b308357941/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8

commit db3f93cc5849efa94d4568bb4dbb3faf1dd776d8
Author: Jun Cai <juncai@chromium.org>
Date: Mon Jul 09 21:54:55 2018

Network Service: Fix VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromSubresourceRequest

This CL adds a VariationsHeaderURLLoaderThrottle on renderer to fix
the VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromSubresourceRequest.

Bug:  841309 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I6120ca42d7aeee9fe721a5af7c93c4047416225b
Reviewed-on: https://chromium-review.googlesource.com/1103451
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573465}
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/browser/BUILD.gn
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/browser/chrome_content_browser_client.cc
[add] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/browser/signin/signin_updater.cc
[add] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/browser/signin/signin_updater.h
[add] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/browser/signin/signin_updater_factory.cc
[add] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/browser/signin/signin_updater_factory.h
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/common/renderer_configuration.mojom
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/common/variations_header_url_loader_throttle.cc
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/common/variations_header_url_loader_throttle.h
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/renderer/chrome_render_thread_observer.cc
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/renderer/chrome_render_thread_observer.h
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/chrome/renderer/url_loader_throttle_provider_impl.cc
[modify] https://crrev.com/db3f93cc5849efa94d4568bb4dbb3faf1dd776d8/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99424620f8a042b4a2b06978f77bc98731f3d09f

commit 99424620f8a042b4a2b06978f77bc98731f3d09f
Author: Jun Cai <juncai@chromium.org>
Date: Wed Jul 11 21:58:47 2018

Network Service: Remove VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromNetworkService

This CL removes VariationsHttpHeadersBrowserTest.TestStrippingHeadersFromNetworkService
since the consumers are required to add headers directly for the initial
request.

Bug:  841309 
Change-Id: Ic84eeef7fcdbc6f1693aa7e56972cd05bf311719
Reviewed-on: https://chromium-review.googlesource.com/1120955
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574356}
[modify] https://crrev.com/99424620f8a042b4a2b06978f77bc98731f3d09f/chrome/browser/net/variations_http_headers_browsertest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3aba7f968ecb147e59a5460bd168e6085425848

commit f3aba7f968ecb147e59a5460bd168e6085425848
Author: Jun Cai <juncai@chromium.org>
Date: Thu Jul 12 16:52:40 2018

Network Service: Document how adding/removing http headers for web content requests are implemented

This is a follow-up CL per comments at:
https://chromium-review.googlesource.com/c/chromium/src/+/1120955

This CL adds comments to //components/variations/variations_http_header_provider.cc
to document how adding/removing http headers for web content requests are
implemented differently when Network Service is enabled or not enabled.

Bug:  841309 
Change-Id: I6b1e7ac776415131f6f549253f8a69a4d0992117
Reviewed-on: https://chromium-review.googlesource.com/1134408
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574602}
[modify] https://crrev.com/f3aba7f968ecb147e59a5460bd168e6085425848/components/variations/variations_http_header_provider.cc

Status: Fixed (was: Started)
Cc: lizeb@chromium.org arthurso...@chromium.org
AFAIU, this is not implemented without the NetworkService. With the NetworkService, it is partially implemented, I mean, |to_be_removed_header| will be ignored when URLLoader::FollowRedirect is called from the NavigationRequest and target the NavigationURLLoaderImpl.

I have a project requiring remove a header on a navigation. How hard do you think it is to implement it?
Making it work with the NetworkService when FollowRedirect is called from the NavigationRequest looks straightforward. For the non-Network service case, I may encounter surprise. juncai@ Do you have idea how hard it would be for me to implement it?
For the network service, this feature used to be missing, so this bug tracks the implementation of this feature. Before it is implemented with network service, this feature already exists, but goes through a different code path using ChromeNetworkDelegate::OnBeforeRedirect():
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_network_delegate.cc?targetos=chromium&l=262

I see thanks!

To make it working from NavigationURLLoaderImpl::FollowRedirect(removed_header, modifed_header), I need to do more plumbing.

I started something here:
https://chromium-review.googlesource.com/c/chromium/src/+/1344053
It is just enough to make it work in my tests, but there are still holes in a few places I need to fill.

Sign in to add a comment