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

Issue 850428 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 2
Type: Task

Blocked on:
issue 905639

Blocking:
issue 892076



Sign in to add a comment

Resolve consents dependency cycles

Project Member Reported by vitaliii@chromium.org, Jun 7 2018

Issue description

There are multiple issues with consent dependencies:

1) There is a runtime dependency cycle UserEventService <-> ProfileSyncService. UserEventService dependency on ProfileSyncService is missing.

2) There is a runtime dependency cycle ConsentAuditor <-> ProfileSyncService. ConsentAuditor dependency on ProfileSyncService is missing.

3) On iOS ProfileSyncService runtime dependency on UserEventService is missing.

4) On iOS ConsentAuditor dependency on ProfileSyncService is missing.

5) On iOS there is build.gn dependency cycle between ConsentAuditorFactory target and ProfileSyncServiceFactory target. Note that UserEventServiceFactory is in the same target as ProfileSyncServiceFactory. ConsentAuditorFactory at the moment depends on both ProfileSyncServiceFactory and UserEventServiceFactory. Thus, this build dependency cycle can't be resolved until we delete UserEvents completely from ConsentAuditor. Note that ProfileSyncService runtime dependency on ConsentAuditor can't be added either (it requires an include, which leads to the same build time cycle).



 
Solution plan:

Step 1: wait for https://chromium-review.googlesource.com/c/chromium/src/+/1087062 (this removes UserEventService and ConsentAuditor runtime dependency on ProfileSyncService on all platforms).

This makes problems 1, 2, 4 obsolete.

Step 2: add all missing runtime dependencies of ProfileSyncService (i.e. 3).

Step 3: Wait until we launch separate datatype for consents completely and delete user events from ConsentAuditor. Fix problem 5. This could also achieved by splitting targets.


Blocking: 851433
Components: Services>Sync
Project Member

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

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

commit 0f28c2bc895daf8d39951249b2fbdc1555acd182
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Jun 12 20:22:25 2018

Propagate more context information in OnSyncStarting()

We rename ActivationContext-->DataTypeActivationResponse to avoid
confusion, and introduce a new struct (DataTypeActivationRequest) that
bundles context information that is relevant during activation of a
datatype.

In this implementation, ModelTypeController pulls those bits from
SyncService's public API, but it may make sense to instead explicitly
plumb them through DataTypeManager, in future patches.

Bug:  833483 , 820049 ,850428
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I91c5c3552dbd5db1c446dd6e00ef74389135af87
Reviewed-on: https://chromium-review.googlesource.com/1087062
Reviewed-by: Mike Pinkerton <pinkerton@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566550}
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/BUILD.gn
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/glue/sync_backend_host_impl.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/glue/sync_backend_host_impl.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/model_type_controller_unittest.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/sync_client_mock.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/sync_client_mock.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/sync_service.h
[delete] https://crrev.com/ece8228cecf913285fec932ca62516fd1f305fd8/components/sync/engine/activation_context.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_request.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_request.h
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_response.cc
[rename] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_response.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_model_type_connector.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_model_type_connector.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_sync_engine.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_sync_engine.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/model_type_configurer.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/model_type_connector.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_connector_proxy.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_connector_proxy.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_registry.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_registry.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_registry_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model/fake_model_type_controller_delegate.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model/fake_model_type_controller_delegate.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model/model_type_controller_delegate.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync_bookmarks/bookmark_model_type_processor.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync_bookmarks/bookmark_model_type_processor.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/ios/chrome/test/app/sync_test_util.mm

https://chromium-review.googlesource.com/c/chromium/src/+/1111716 fixes 

 - (1) by removing UserEventService dependency on ProfileSyncService.
 - (2) & (4) by removing ConsentAuditor dependency on ProfileSyncService.

It also unblocks (3).
Project Member

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

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

commit b4d3bf77f27e72b4448214e10205eb5216889f58
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Jun 25 14:55:08 2018

Propagate sync activation request to bridges

This struct includes authenticated_account_id, which several bridges are
interested in. This is safer and simpler to use compared to reading
from side channels, and prevents a runtime dependency cycle across some
factories.

TBR=estade@chromium.org,rockot@chromium.org

Bug: 850428, 834042 , 854446 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ib17a4503ff9d284e1a5f4e222e7eeb95b08a024f
Reviewed-on: https://chromium-review.googlesource.com/1111716
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570037}
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/chrome/browser/consent_auditor/consent_auditor_factory.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/chrome/browser/extensions/api/sessions/sessions_apitest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/chrome/browser/sync/user_event_service_factory.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/consent_auditor/consent_sync_bridge_impl.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/consent_auditor/consent_sync_bridge_impl.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/consent_auditor/consent_sync_bridge_impl_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/BUILD.gn
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/driver/model_type_controller.cc
[rename] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/data_type_activation_request.cc
[rename] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/data_type_activation_request.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_service_impl_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_sync_bridge_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync_sessions/session_sync_bridge.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/ios/chrome/browser/consent_auditor/consent_auditor_factory.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/ios/chrome/browser/sync/ios_user_event_service_factory.cc

Unfortunately (5) blocks enabling the type by default.
I will have to move consent auditor factory on iOS into the sync folder temporarily. allow_circular_includes_from did not help.
I've also considered moving user_event_service_factory out of sync folder, but this is not possible, because user_event_service_factory depends on sync service to check whether history is enabled. Thus, moving consent_auditor in seems like the only option at this stage.

The plan is to move it out once its dependency on user_event_service is deleted (i.e. USER_CONSENTS are fully launched).
FYI, we don't observe this issue not on iOS because they are already in one BUILD.gn target there (chrome/browser/BUILD.gn), but in different folders.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 18

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

commit 002e7e5bfeed31e6152641759e516a9e28bcc606
Author: vitaliii <vitaliii@chromium.org>
Date: Wed Jul 18 13:55:54 2018

[Sync::Consent::iOS] Move factory to sync folder.

Temporarily move consent_auditor_factory.{h,cc} to
ios/chrome/browser/sync.

Otherwise I cannot wire ControllerDelegateOnUIThread for USER_CONSENTS
in ios_chrome_sync_client, because consent_auditor_factory depends on
UserEventService, which is located in sync folder (i.e. this leads to
BUILD.gn cycle).

The plan is to move it back once it does not depend on UserEventService
anymore (i.e. USER_CONSENTS are completely launched).

Bug:  840357 ,850428
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I2157e913ee6d6d3adde86b3121fdb021aed3614c
Reviewed-on: https://chromium-review.googlesource.com/1141583
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Commit-Queue: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576025}
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/browser_state/BUILD.gn
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
[delete] https://crrev.com/1655037ead023cea3b18757ce10b2a98135a34ed/ios/chrome/browser/consent_auditor/BUILD.gn
[delete] https://crrev.com/1655037ead023cea3b18757ce10b2a98135a34ed/ios/chrome/browser/consent_auditor/OWNERS
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/prefs/BUILD.gn
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/sync/BUILD.gn
[rename] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/sync/DEPS
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/sync/OWNERS
[rename] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/sync/consent_auditor_factory.cc
[rename] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/sync/consent_auditor_factory.h
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/ui/authentication/BUILD.gn
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/ui/authentication/chrome_signin_view_controller.mm
[modify] https://crrev.com/002e7e5bfeed31e6152641759e516a9e28bcc606/ios/chrome/browser/ui/authentication/chrome_signin_view_controller_unittest.mm

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 18

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

commit ec63e94d1b4ffb3d08c97f5f8c7126ea5dd6d6cf
Author: vitaliii <vitaliii@chromium.org>
Date: Wed Jul 18 14:16:25 2018

[Sync::Consent::iOS] Wire controller delegate.

Bug:  840357 ,850428
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I21266676a5e097dac3d7cc75dbff8b65a2ce9d1e
Reviewed-on: https://chromium-review.googlesource.com/1141724
Commit-Queue: vitaliii <vitaliii@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576034}
[modify] https://crrev.com/ec63e94d1b4ffb3d08c97f5f8c7126ea5dd6d6cf/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/ec63e94d1b4ffb3d08c97f5f8c7126ea5dd6d6cf/ios/chrome/browser/sync/profile_sync_service_factory.cc

Sync triage ping. Is there any updates?
This is blocked on User Consents launch, thus, waiting for M69 to go to Stable.
Blocking: 892076
Blocking: -851433
Blockedon: 905639
Then we need to add UserEventServiceFactory dependency on ProfileSyncServiceFactory:
https://cs.corp.google.com/clankium/src/chrome/browser/sync/user_event_service_factory.cc?q=UserEventServiceFac&dr=CSs&l=47
Owner: ----
Status: Available (was: Assigned)
Owner: jkrcal@chromium.org
Taking it as part of fixit.
Labels: OS-Android OS-Chrome OS-iOS OS-Linux OS-Mac OS-Windows
Status: Assigned (was: Available)
Labels: sync-fixit-2018q4
Cc: treib@chromium.org
Ad #18:
I plan to fix the run-time dependency the other way around, removing ProfileSyncServiceFactory from UserEventServiceFactory (there is already no such dependency in ConsentAuditorFactory). 
CL in progress: https://chromium-review.googlesource.com/c/chromium/src/+/1349257.

Ad #17:

Getting rid of the build-time dependency on ios (i.e. reverting https://chromium-review.googlesource.com/1141583) is not that easy:
 - ProfileSyncService depends on ConsentAuditorFactory 
   (this is quite hard-wired into the whole design of sync)
 - ConsentAuditorFactory still depends on ModelTypeStoreServiceFactory.

There are a few solutions:
 - pull out ModelTypeStoreServiceFactory out of ios/chrome/browser/sync into a separate target (seems illogical)
 - move non-ios ConsentAuditorFactory into chrome/browser/sync so that the folder structure is the same as on ios
 - do nothing and accept that folder structures diverge :)

Marc, do you have an opinion here?
The plan for removing the runtime dependency SGTM and is IMO the correct fix.

I don't quite understand what the problem with the build-time dependency is. Do things work out on non-iOS only because there all of chrome/browser is a single build target?
Why does the same problem not occur for other data types? Seems like many of them should have the same dependencies (PSS factory depends on datatype-specific factory which depends on MTSS factory).
Correct, things work out on non-iOS only because there all of chrome/browser is a single build target.

I've randomly checked and the folder structure of 
   chrome/browser/[anything sync related]
is different for several factories from the folder structure of
   ios/chrome/browser/[anything sync related]

So I propose to stop caring about unifying it for ConsentAuditorFactory.
Anyone objects?

Comment 26 by jkrcal@chromium.org, Jan 21 (2 days ago)

Fixing this bug (removing the run-time dependency) is still WIP.

Sign in to add a comment