Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(8)

Issue 398423002: Sync: Refactoring of DEVICE_INFO syncable type - Part 1 (Closed)

Created:
6 years, 5 months ago by stanisc
Modified:
6 years, 5 months ago
CC:
chromium-reviews, tim+watch_chromium.org, extensions-reviews_chromium.org, zea+watch_chromium.org, haitaol+watch_chromium.org, chromium-apps-reviews_chromium.org, maniscalco+watch_chromium.org, maniscalco, sky, Nicolas Zea
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Sync: Refactoring of DEVICE_INFO syncable type - Part 1 This reintroduces change 283461 (issue 367153005) that has been reverted due to memory leaks in some unit tests. The memory leak was in a mock implementation of ProfileSyncComponentsFactory. The factory is responsible for creating an instance of LocalDeviceInfoProvider, which it does on demand and immediately transfer ownership of LocalDeviceInfoProvider to the caller. But due to the way mocking was implemented in ProfileSyncComponentsFactoryMock a mock instance of LocalDeviceInfoProvider was created in the constructor and if a test never called CreateLocalDeviceInfoProvider(), it would be leaked. In this change I moved away from using ON_CALL mocking mechanism that resulted in the leak to a custom implementation that avoids that problem. See original changes in the patch #1 and the fix in the patch #2. The original description copied from issue 367153005: This change introduces a new class LocalDeviceInfoProvider that is responsible for providing the local device specific DeviceInfo/cache_guid. It initializes the data asynchronously and allows consumers to get notified when the data becomes available. LocalDeviceInfoProvider will allow to remove these responsibilities from SyncedDeviceTracker / ProfileSyncService and loose coupling between ProfileSyncService and a number of SyncableService and DataTypeController derived classes. LocalDeviceInfoProvider is hosted on the frontend thread. Since it needs cache_guid to initialize DeviceInfo, which is currently available only on the backend, I updated SyncBackendHostCore and SyncBackendHostImpl to pass cache_guid to the frontend via OnBackendInitialized. For the time being SyncedDeviceTracker remains unchanged and continues to initialize the local device info too. The entire class will be removed in the Part 2 of the change which will move SyncedDeviceTracker functionality to a new SyncableService. LocalDeviceInfoProvider also replaces SessionsSyncManager::SyncInternalApiDelegate which was previously used to decouple SessionsSyncManager from the specifics of of the local device info implementation. SessionsSyncManager is changed to consume LocalDeviceInfoProvider instead of SessionsSyncManager::SyncInternalApiDelegate. SessionDataTypeController now also consumes LocalDeviceInfoProvider to make sure that SESSIONS type model doesn't start before local device info becomes available. A very similar approach will be used in the upcoming second part of the change for DEVICE_INFO DataTypeController and SyncableType. The change includes new unit tests for LocalDeviceInfoProvider and SessionDataTypeController. TBR=maniscalco@chromium.org,sky@chromium.org,zea@chromium.org BUG=395349 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284561

Patch Set 1 #

Patch Set 2 : ProfileSyncComponentsFactoryMock memory leak fix #

Total comments: 1

Patch Set 3 : Merged with Bug 382968 changes (Pass signin_scoped_device_id to DeviceInfoSpecifics). #

Patch Set 4 : Removed wrong signin_scoped_device_id assert from LocalDeviceInfoProvider. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+928 lines, -194 lines) Patch
M chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/sessions/sessions_apitest.cc View 1 2 5 chunks +28 lines, -11 lines 0 comments Download
M chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc View 3 chunks +6 lines, -1 line 0 comments Download
A chrome/browser/sync/glue/local_device_info_provider.h View 1 2 1 chunk +50 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/local_device_info_provider_impl.h View 1 2 1 chunk +41 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/local_device_info_provider_impl.cc View 1 2 3 1 chunk +61 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/local_device_info_provider_mock.h View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/local_device_info_provider_mock.cc View 1 2 1 chunk +61 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/local_device_info_provider_unittest.cc View 1 2 1 chunk +85 lines, -0 lines 0 comments Download
M chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_host_core.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/sync/glue/sync_backend_host_impl.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/sync/glue/sync_backend_host_impl.cc View 1 2 3 3 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_host_mock.cc View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_components_factory.h View 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_components_factory_impl.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_components_factory_impl.cc View 1 2 3 3 chunks +12 lines, -1 line 0 comments Download
M chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc View 2 chunks +14 lines, -12 lines 0 comments Download
M chrome/browser/sync/profile_sync_components_factory_mock.h View 1 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_components_factory_mock.cc View 1 3 chunks +18 lines, -2 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.h View 1 2 3 7 chunks +12 lines, -15 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.cc View 1 2 3 8 chunks +25 lines, -23 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_factory.cc View 1 2 1 chunk +7 lines, -5 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_mock.h View 1 2 3 3 chunks +4 lines, -7 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_mock.cc View 3 chunks +15 lines, -6 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_startup_unittest.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/sync/sessions/session_data_type_controller.h View 3 chunks +21 lines, -1 line 0 comments Download
M chrome/browser/sync/sessions/session_data_type_controller.cc View 6 chunks +42 lines, -5 lines 0 comments Download
A chrome/browser/sync/sessions/session_data_type_controller_unittest.cc View 1 2 1 chunk +234 lines, -0 lines 0 comments Download
M chrome/browser/sync/sessions/sessions_sync_manager.h View 4 chunks +6 lines, -15 lines 0 comments Download
M chrome/browser/sync/sessions/sessions_sync_manager.cc View 6 chunks +26 lines, -8 lines 0 comments Download
M chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc View 1 2 6 chunks +19 lines, -18 lines 0 comments Download
M chrome/browser/sync/test_profile_sync_service.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/test_profile_sync_service.cc View 2 chunks +9 lines, -9 lines 0 comments Download
M chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm View 1 2 5 chunks +16 lines, -18 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_helper_unittest.cc View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc View 1 2 4 chunks +12 lines, -20 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 chunks +6 lines, -2 lines 0 comments Download
M components/sync_driver/sync_frontend.h View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
stanisc
Richard, please review the change between the original patch (Patch 1) and the memory leak ...
6 years, 5 months ago (2014-07-18 01:14:13 UTC) #1
rlarocque
Thanks for making the changes clear. LGTM. You'll need some extra approvals from owners of ...
6 years, 5 months ago (2014-07-18 01:19:35 UTC) #2
stanisc
I wanted to add that I verified that unit_test memory leaks are gone in "linux_asan" ...
6 years, 5 months ago (2014-07-18 01:19:41 UTC) #3
stanisc
I've added original reviewers from the previews change that has been reverted. Please note that ...
6 years, 5 months ago (2014-07-18 01:29:11 UTC) #4
sky
Files you asked me to review LGTM https://codereview.chromium.org/398423002/diff/20001/chrome/browser/extensions/api/sessions/sessions_apitest.cc File chrome/browser/extensions/api/sessions/sessions_apitest.cc (right): https://codereview.chromium.org/398423002/diff/20001/chrome/browser/extensions/api/sessions/sessions_apitest.cc#newcode123 chrome/browser/extensions/api/sessions/sessions_apitest.cc:123: nit: no ...
6 years, 5 months ago (2014-07-18 15:37:18 UTC) #5
stanisc
The CQ bit was checked by stanisc@chromium.org
6 years, 5 months ago (2014-07-18 22:37:48 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stanisc@chromium.org/398423002/20001
6 years, 5 months ago (2014-07-18 22:40:33 UTC) #7
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium ...
6 years, 5 months ago (2014-07-18 23:42:58 UTC) #8
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-18 23:55:16 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chromeos_clang_dbg/builds/30942)
6 years, 5 months ago (2014-07-18 23:55:17 UTC) #10
stanisc
The CQ bit was checked by stanisc@chromium.org
6 years, 5 months ago (2014-07-21 23:20:03 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stanisc@chromium.org/398423002/60001
6 years, 5 months ago (2014-07-21 23:21:20 UTC) #12
commit-bot: I haz the power
6 years, 5 months ago (2014-07-22 00:36:29 UTC) #13
Message was sent while issue was closed.
Change committed as 284561

Powered by Google App Engine
This is Rietveld 408576698