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

Issue 855010 link

Starred by 1 user
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Aug 2
Cc:
mastiz@chromium.org
treib@chromium.org
jkrcal@chromium.org
mamir@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 867801
issue 866814



Sign in to add a comment

[USS] Simplify multi-threading by creating a proxy between ModelTypeController and its delegate

Project Member Reported by jkrcal@chromium.org, Jun 21 2018 
ModelTypeController lives on the UI thread, its delegate may live on a different thread (model thread). Currently, the cross thread posting is implemented in the controller (and/or its subclasses).

We should create a proxy class that implements the ModelTypeControllerDelegate interface that takes care of cross-thread posting.
 - the default implementation for data types that live on the UI thread could be super-simple (maybe we even do not need any proxy in this case; maybe we need it to post the tasks so they do execute asynchronously).
 - there could be a different proxy implementation that actually does cross-thread posting, with weak-ptrs, etc.

As part of this bug, we should heavily simplify unit-tests for ModelTypeController.

Comment 1 by mastiz@chromium.org, Jul 3

Labels: sync-fixit-2018q3

Comment 2 by mastiz@chromium.org, Jul 24

Blocking: 866814

Comment 3 by mastiz@chromium.org, Jul 24

Owner: mastiz@chromium.org
Status: Started (was: Available)

Comment 4 by mastiz@chromium.org, Jul 26

Blocking: 867801
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 26 

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

commit c6436dc053c393458420e41d49fd5d90c978213c
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Jul 26 12:00:26 2018

Introduce proxy ModelTypeControllerDelegate

This moves away any cross-sequence posting logic from
ModelTypeController, into a newly introduced class specialized in this
problem.

The goal is that ModelTypeController doesn't grab the delegate
dependency in a subtle way from SyncClient, and instead it gets
injected as dependency. Follow-up patches will leverage the
advantanges of this, such as not posting tasks when the datatype lives
in the UI thread.

There is one behavioral change that was hard to avoid here, and seems
overall desirable: TYPED_URL migrates away from HistoryService's
ScheduleDBTask() API and instead uses PostTask() directly to the
backend task runner, which is achieved by instantiating the
proxy delegate within HistoryService (since the task runner doesn't
get exposed publicly).

Bug:  855010 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id626b0b7e9b7febd7596b9cc53e01b32c1ce23a0
Reviewed-on: https://chromium-review.googlesource.com/1148568
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578266}
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/autofill/core/browser/BUILD.gn
[delete] https://crrev.com/bf243bd2656da518db74dfbfc469b6ee8fab677c/components/autofill/core/browser/webdata/web_data_model_type_controller.cc
[delete] https://crrev.com/bf243bd2656da518db74dfbfc469b6ee8fab677c/components/autofill/core/browser/webdata/web_data_model_type_controller.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/browser_sync/profile_sync_components_factory_impl.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/browsing_history_service.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/history_backend.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/history_backend_unittest.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/history_service.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/history_service.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/typed_url_model_type_controller.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/history/core/browser/typed_url_model_type_controller.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/omnibox/browser/in_memory_url_index.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/omnibox/browser/url_index_private_data.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/BUILD.gn
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/driver/DEPS
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/model/model_type_change_processor.h
[add] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/model_impl/proxy_model_type_controller_delegate.cc
[add] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync/model_impl/proxy_model_type_controller_delegate.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync_sessions/session_model_type_controller.cc
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/components/sync_sessions/session_model_type_controller.h
[modify] https://crrev.com/c6436dc053c393458420e41d49fd5d90c978213c/ios/chrome/browser/sync/ios_chrome_sync_client.mm
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 2 

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

commit f85f820a29239c4e2896b7250c289a0e4585b0ce
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Aug 02 15:24:28 2018

Avoid thread hopping in ModelTypeController tests

We do this for the control methods to the delegate to simplify tests,
by migrating away from ProxyModelTypeControllerDelegate.

Instead, ForwardingModelTypeControllerDelegate is introduced, currently
used in tests only but with the goal to be adopted more broadly (see
crbug.com/867801).

A proxy processor is still in use, and that still requires some nested
loops, but that's left out for future patches.

Bug:  855010 ,867801
Change-Id: I1933355fee2093bf006830cde334c644f4391020
Reviewed-on: https://chromium-review.googlesource.com/1160642
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580189}
[modify] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/BUILD.gn
[modify] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/driver/model_type_controller_unittest.cc
[add] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/model_impl/forwarding_model_type_controller_delegate.cc
[add] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/model_impl/forwarding_model_type_controller_delegate.h

Comment 7 by mastiz@chromium.org, Aug 2

Status: Fixed (was: Started)
Tests can be simplified further, but all work related to the delegate is now completed.
►

Sign in to add a comment