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

Issue 855375 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 854446



Sign in to add a comment

Sync metadata can leak if sync stopped while starting

Project Member Reported by mastiz@chromium.org, Jun 22 2018

Issue description

The simplest scenario where this can occur is a busy I/O thread causing a slow startup process for a datatype. If sync is disabled meanwhile, ModelTypeController is not supposed to just ignore the event.
 

Comment 1 by mastiz@chromium.org, Jun 22 2018

Labels: -Pri-2 Sync-Triaged Pri-1
Adjusting prio due to blocking dependency.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 22 2018

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

commit 39396141ab32683696bfbda5887b26792034dd75
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Jun 22 11:39:57 2018

Refactor ModelTypeController state transitions

More explicit state handling is introduced with a switch, and some dead
code is replaced with a DCHECK.

Bug:  855375 
Change-Id: I644d26e63bf51bf0cb56362afa0fa990f75e8be7
Reviewed-on: https://chromium-review.googlesource.com/1111708
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569579}
[modify] https://crrev.com/39396141ab32683696bfbda5887b26792034dd75/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/39396141ab32683696bfbda5887b26792034dd75/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/39396141ab32683696bfbda5887b26792034dd75/components/sync/driver/model_type_controller_unittest.cc

Project Member

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

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

commit 7059913e9dfc552ed5c85b0b626a6364517e8679
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Jun 25 19:43:26 2018

Enable more EnableDisableSingleClientTest tests

These tests were believed to be failing or be flaky, but I could
not reproduce the failures locally, presumably due to recent
improvements in https://chromium-review.googlesource.com/1111716

As it seems, DataTypeManagerImpl's mechanism to defer
reconfigurations via |needs_reconfigure_| is sufficient to handle
fast enable-disable-enable sequences.

Bug:  855375 
Change-Id: Iec677202f9db15787da1a62345e09852e84a9f79
Reviewed-on: https://chromium-review.googlesource.com/1111844
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570137}
[modify] https://crrev.com/7059913e9dfc552ed5c85b0b626a6364517e8679/chrome/browser/sync/test/integration/enable_disable_test.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 4

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

commit 23919ce6813da7b70e2d5cf488345f5f7f42fa1d
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Jul 04 14:12:58 2018

Wait until STOPPING finishes during configuration

The process of stopping a datatype is asynchronous by nature, most
prominently in USS where we want to take special care about sync
metadata being cleared.

Prior to this patch, ModelTypeController could silently drop stop
events, which could lead to the DataTypeManager (effectively the UI)
think a datatype is stopped, while in reality it could remain
running. This is problematic in multiple ways, most importantly due
to the risk of leaking sync metadata or even data.

Instead, what we want is that calls to OnSyncStarting() always have a
corresponding StopSync(), and guarantee that OnSyncStarting() is not
called twice without a StopSync() call in between.

In order to do that, ModelTypeController now enters state STOPPING
if the Stop() cannot be immediately executed, because it's currently
STARTING. Once the start is finalized, stop is immediately issued and
the corresponding callback informed, which allows
ModelAssociationManager to continue its job.

Bug:  855375 
Change-Id: I6e22940fcb56816c43181c34e04c4a6e53c70a21
Reviewed-on: https://chromium-review.googlesource.com/1122863
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572555}
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/directory_data_type_controller.cc
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/directory_data_type_controller.h
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/model_association_manager.h
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/proxy_data_type_controller.cc
[modify] https://crrev.com/23919ce6813da7b70e2d5cf488345f5f7f42fa1d/components/sync/driver/proxy_data_type_controller.h

Status: Fixed (was: Started)

Sign in to add a comment