Migrate components/sync/engine/net/http_bridge.cc to network::SimpleURLLoader |
||||||||
Issue description
,
Jun 25 2018
,
Aug 8
Taking this one.
,
Aug 14
,
Aug 24
,
Aug 25
,
Aug 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf4f299d16f25abc763cb86c06a0e8ce46ecca04 commit bf4f299d16f25abc763cb86c06a0e8ce46ecca04 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Sat Aug 25 03:35:07 2018 Migrate components/sync/engine/net/http_bridge.cc to network::SimpleURLLoader URLFetcher will stop working with advent of Network Service, and SimpleURLLoader is the replacement API for most clients. This CL migrates HttpBridge and FinancialPing away from URLFetcher. Note that this CL slightly changes the flow of the HttpBridge{Factory} code. Previously, an URLRequestContextGetter instance was acquired on UI thread, passed to the Sync and IO threads. However, URLLoaderFactory and SimpleURLLoader have stricter threading restrictions than URLFetcher and URLRequestContextGetter. For instance, a SharedURLLoaderFactory instance needs to be created, used and deleted on the same thread. For this reason methods like HttpBridgeFactory::OnSignalReceived can not simply null out |url_loader_factory_| which is created on the "sync" thread. Additionally, some preexisting tests do not seem relevant for the context of network service, and got removed, eg TestUsesSameHttpNetworkSession, RequestContextGetterReleaseOrder and EarlyAbortFactory. Particularly, the last two used to deal with URLRequestContextGetter destruction. With the URLLoader migration, HttpBridge now holds on to mojo pipes instead of URLRequestContextGetter. BUG=773295, 844968 Change-Id: I185f814ba171dca69fec55b913b86a0cc9993ef1 Reviewed-on: https://chromium-review.googlesource.com/1174655 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Nicolas Zea <zea@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#586115} [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/BUILD.gn [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/driver/glue/sync_backend_host_impl_unittest.cc [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/DEPS [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/http_bridge.cc [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/http_bridge.h [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/http_bridge_network_resources.cc [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/http_bridge_network_resources.h [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/http_bridge_unittest.cc [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/engine/net/network_resources.h [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/test/DEPS [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/test/fake_server/fake_server_network_resources.cc [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/test/fake_server/fake_server_network_resources.h [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/tools/BUILD.gn [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/tools/DEPS [modify] https://crrev.com/bf4f299d16f25abc763cb86c06a0e8ce46ecca04/components/sync/tools/sync_client.cc
,
Aug 25
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7bc9bb3469e90d53d23a12913c77fff4591e631 commit f7bc9bb3469e90d53d23a12913c77fff4591e631 Author: Maks Orlovich <morlovich@chromium.org> Date: Wed Aug 29 14:12:38 2018 Sync no longer needs URLRequestContext, so remove its injection. This is possible thanks to tonikitoo@ porting it to newer APIs; desirable since users of Profile::GetRequestContext may break soon, so less users of that method is reassuring. Bug: 844968 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: I2a92cc05d40764f5a1c6358a3a571b2e3cd8f191 Reviewed-on: https://chromium-review.googlesource.com/1191462 Reviewed-by: Tatiana Gornak <melandory@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Maks Orlovich <morlovich@chromium.org> Cr-Commit-Position: refs/heads/master@{#587103} [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/chrome/browser/sync/profile_sync_service_factory.cc [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/chrome/browser/sync/profile_sync_test_util.cc [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/components/browser_sync/profile_sync_test_util.cc [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/components/browser_sync/profile_sync_test_util.h [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/ios/chrome/browser/sync/profile_sync_service_factory.cc [modify] https://crrev.com/f7bc9bb3469e90d53d23a12913c77fff4591e631/ios/web_view/internal/sync/web_view_profile_sync_service_factory.mm
,
Sep 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05b9b976f63eb1f9fcca0e01c15a6eeeb719dd69 commit 05b9b976f63eb1f9fcca0e01c15a6eeeb719dd69 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Tue Sep 11 17:10:45 2018 Fix sync logic failing with repeated instance of "network connection unavailable" This is a follow on of [1], where the sync::HttpBridge logic switched away from URLFetcher to SimpleURLLoader, as part of the network servicification effort. [1] https://crrev.com/c/1174655/ It turns out that the way sync::HttpBridge interpreted the response of an upload request changed after the migration: By default, SimpleURLLoader does not pass a valid response body in case of either HTTP or network errors. What is more, the new logic in HttpBridge::OnURLLoadComplete relied precisely on null-checking the responsebody to verify whether the upload was successful or not. This CL changes this particular check logic so that it matches the previous behavior: only actual network errors account for sync failures (which map to "network connection unavailable" errors) whereas HTTP errors (>= 400 && < 600) still account as a successful uploads. 401 specifically is handled to request expired user credentials in [2]. [2] https://cs.chromium.org/chromium/src/components/sync/engine_impl/net/sync_server_connection_manager.cc?l=84 BUG= 844968 , 878718 , 880633 Change-Id: I9d0b93b527f103149a7c78ba405dde27cae779f2 Reviewed-on: https://chromium-review.googlesource.com/1217122 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#590363} [modify] https://crrev.com/05b9b976f63eb1f9fcca0e01c15a6eeeb719dd69/components/sync/engine/net/http_bridge.cc [modify] https://crrev.com/05b9b976f63eb1f9fcca0e01c15a6eeeb719dd69/components/sync/engine/net/http_bridge_unittest.cc
,
Sep 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06d3c14ce6d8593d9d1bfe3269e6ef99f3f2d1df commit 06d3c14ce6d8593d9d1bfe3269e6ef99f3f2d1df Author: Antonio Gomes <tonikitoo@igalia.com> Date: Thu Sep 13 12:40:48 2018 Use network::TransitionalURLLoaderFactoryOwner in sync_client This is a follow up of [1], where the sync logic switched away from using URLFetcher, in favor of SimpleURLLoader (network s13n effort). [1] https://crrev.com/c/1174655 As part of this migration, sync_client (the executable) switched to use TestSharedURLLoaderFactory. However, TransitionalURLLoaderFactoryOwner was particularly implemented to be used for standalong applications, which is the case of sync_client. This CL switches sync_client (target components/sync/tools:sync_client) to use it. BUG= 844968 Change-Id: I9eaa980c3b211232b897e6aa8bb6336f5e35ec10 Reviewed-on: https://chromium-review.googlesource.com/1222508 Reviewed-by: Maks Orlovich <morlovich@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Cr-Commit-Position: refs/heads/master@{#590984} [modify] https://crrev.com/06d3c14ce6d8593d9d1bfe3269e6ef99f3f2d1df/components/sync/tools/BUILD.gn [modify] https://crrev.com/06d3c14ce6d8593d9d1bfe3269e6ef99f3f2d1df/components/sync/tools/DEPS [modify] https://crrev.com/06d3c14ce6d8593d9d1bfe3269e6ef99f3f2d1df/components/sync/tools/sync_client.cc
,
Sep 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b2d8aad00f9d4f0feedc06cb789ad65da468dbf commit 3b2d8aad00f9d4f0feedc06cb789ad65da468dbf Author: Antonio Gomes <tonikitoo@igalia.com> Date: Thu Sep 13 17:16:15 2018 Switch SyncEngine::InitParams::http_factory_getter to BindOnce As part of [1], uses of URLFetcher were replaced by SimpleURLLoader, as part of the s13n network effort. [1] https://crrev.com/c/1174655/ Given the multi-thread nature of the sync engine logic, and the fact that network::SharedURLLoaderFactory instances need to operate in the same thread it was created, it was not possible to replace URLRequestContextGetter by SharedURLLoaderFactory directly. The SharedURLLoaderFactory::Clone method was used. SharedURLLoaderFactory::Clone returns a move-only object named CrossThreadSharedURLLoaderFactoryInfo. If CrossThreadSharedURLLoaderFactoryInfo is move-only, the callback it was bound with can not be called more than once (ProfileSyncService::MakeHttpPostProviderFactoryGetter). This CL switches SyncEngine::HttpPostProviderFactoryGetter from Callback to OnceCallback. In practice, this does not bring any functional change, because if the sync engine is going to be restarted (for any reason), ProfileSyncService::StartUpSlowEngineComponents would be called again and the callback bound again with a brand new instance of CrossThreadSharedURLLoaderFactoryInfo. BUG= 844968 Change-Id: Ib45e512d22e371123e7004c386640a17f0695250 Reviewed-on: https://chromium-review.googlesource.com/1224690 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#591053} [modify] https://crrev.com/3b2d8aad00f9d4f0feedc06cb789ad65da468dbf/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/3b2d8aad00f9d4f0feedc06cb789ad65da468dbf/components/sync/driver/glue/sync_backend_host_core.cc [modify] https://crrev.com/3b2d8aad00f9d4f0feedc06cb789ad65da468dbf/components/sync/driver/glue/sync_backend_host_impl_unittest.cc [modify] https://crrev.com/3b2d8aad00f9d4f0feedc06cb789ad65da468dbf/components/sync/engine/sync_engine.h
,
Sep 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb324165da2232905e108c07ef42e288db468fac commit bb324165da2232905e108c07ef42e288db468fac Author: Antonio Gomes <tonikitoo@igalia.com> Date: Fri Sep 14 06:31:30 2018 Fix sync logic failing with repeated instance of "network connection unavailable" This is a follow on of [1], where the sync::HttpBridge logic switched away from URLFetcher to SimpleURLLoader, as part of the network servicification effort. [1] https://crrev.com/c/1174655/ It turns out that the way sync::HttpBridge interpreted the response of an upload request changed after the migration: By default, SimpleURLLoader does not pass a valid response body in case of either HTTP or network errors. What is more, the new logic in HttpBridge::OnURLLoadComplete relied precisely on null-checking the responsebody to verify whether the upload was successful or not. This CL changes this particular check logic so that it matches the previous behavior: only actual network errors account for sync failures (which map to "network connection unavailable" errors) whereas HTTP errors (>= 400 && < 600) still account as a successful uploads. 401 specifically is handled to request expired user credentials in [2]. [2] https://cs.chromium.org/chromium/src/components/sync/engine_impl/net/sync_server_connection_manager.cc?l=84 BUG= 844968 , 878718 , 880633 Change-Id: I9d0b93b527f103149a7c78ba405dde27cae779f2 Reviewed-on: https://chromium-review.googlesource.com/1217122 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#590363}(cherry picked from commit 05b9b976f63eb1f9fcca0e01c15a6eeeb719dd69) Reviewed-on: https://chromium-review.googlesource.com/1225490 Reviewed-by: Antonio Gomes <tonikitoo@igalia.com> Cr-Commit-Position: refs/branch-heads/3538@{#400} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/bb324165da2232905e108c07ef42e288db468fac/components/sync/engine/net/http_bridge.cc [modify] https://crrev.com/bb324165da2232905e108c07ef42e288db468fac/components/sync/engine/net/http_bridge_unittest.cc |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by dxie@google.com
, May 20 2018Status: Available (was: Untriaged)