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

Issue 844968 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: ----

Blocking:
issue 773295
issue 877740



Sign in to add a comment

Migrate components/sync/engine/net/http_bridge.cc to network::SimpleURLLoader

Project Member Reported by dxie@google.com, May 20 2018

Issue description


 

Comment 1 by dxie@google.com, May 20 2018

Labels: Proj-Servicification-Canary Proj-Servicification OS-Windows OS-Linux OS-Mac OS-Chrome Proj-Servicification-network-url OS-Android
Status: Available (was: Untriaged)
Blocking: 773295
Components: Services>Sync
Summary: Migrate components/sync/engine/net/http_bridge.cc to network::SimpleURLLoader (was: Migrate components/sync/engine/net/http_bridge.cc)
Owner: toniki...@chromium.org
Status: Assigned (was: Available)
Taking this one.
Status: Started (was: Assigned)
Labels: Pri-1
Blocking: 877740
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 14

Labels: merge-merged-3538
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