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

Issue 923285 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 923287



Sign in to add a comment

Find new home for authoritative sync cache GUID

Project Member Reported by mastiz@chromium.org, Jan 18 (4 days ago)

Issue description

We are deprecating sync's Directory in order to ultimately dismantle the old sync code.

One central concept that is "owned" and persisted by the Directory today is the "cache GUID", a random ID that is bound to a sync session (i.e. gets cleared when the user disables sync).

We should find a new home for this ID, before considering the removal of the Directory. Depending on the exact implementation, we may want to do this some milestones in advance in order to make sure all users have "migrated" this piece of information.

 

Comment 1 by mastiz@chromium.org, Jan 18 (4 days ago)

Blocking: 923287
Project Member

Comment 2 by bugdroid1@chromium.org, Yesterday (41 hours ago)

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

commit c031c01b33a63540fe2debc5ce07ff07ff8e4d6e
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Jan 21 13:11:08 2019

Save various sync Directory fields also to prefs

This patch simply copies the authoritative values (still in Directory)
into prefs, as a future-proof step in preparation for ultimately
migrating users away from the Directory and dismantling it.

Bug: 923285
Change-Id: I8229efcd0820a20a8b0ba044cd6d9ff7cee1167d
Reviewed-on: https://chromium-review.googlesource.com/c/1421098
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624574}
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/chrome/browser/sync/test/integration/enable_disable_test.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/base/pref_names.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/base/pref_names.h
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/base/sync_prefs.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/base/sync_prefs.h
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/driver/glue/sync_backend_host_impl.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/driver/glue/sync_backend_host_impl.h
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/driver/glue/sync_backend_host_impl_unittest.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/engine/fake_sync_engine.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/engine/sync_engine_host.h
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/engine/sync_engine_host_stub.cc
[modify] https://crrev.com/c031c01b33a63540fe2debc5ce07ff07ff8e4d6e/components/sync/engine/sync_engine_host_stub.h

Comment 3 by isandrk@chromium.org, Today (14 hours ago)

Hi mastiz@, I'm getting a crash on ChromeOS that is probably caused by this change (doesn't crash at revision T-1):

[118141:118141:0122/163052.601144:FATAL:values.cc(155)] Check failed: IsStringUTF8(string_value_). 
#0 0x7f853295810f base::debug::StackTrace::StackTrace()
#1 0x7f85328860ba logging::LogMessage::~LogMessage()
#2 0x7f853294ab16 base::Value::Value()
#3 0x7f853294abbe base::Value::Value()
#4 0x7f852d521caf PrefService::SetString()
#5 0x55ecae0726b1 syncer::SyncPrefs::SetBagOfChips()
#6 0x55ecae886e00 browser_sync::ProfileSyncService::OnSyncCycleCompleted()
#7 0x55ecaf6fdc01 syncer::SyncBackendHostImpl::HandleSyncCycleCompletedOnFrontendLoop()
#8 0x55ecace0f860 base::internal::Invoker<>::Run()
#9 0x7f853286a3c1 base::debug::TaskAnnotator::RunTask()
#10 0x7f85328952cf base::MessageLoopImpl::RunTask()
#11 0x7f8532895953 base::MessageLoopImpl::DoWork()
#12 0x7f853297b3a9 base::MessagePumpLibevent::Run()
#13 0x7f8532894e78 base::MessageLoopImpl::Run()
#14 0x7f85328c60f5 base::RunLoop::Run()
#15 0x55ecadaf8b0b ChromeBrowserMainParts::MainMessageLoopRun()
#16 0x7f852f7a41b4 content::BrowserMainLoop::RunMainMessageLoopParts()
#17 0x7f852f7a6876 content::BrowserMainRunnerImpl::Run()
#18 0x7f852f7a0ddc content::BrowserMain()
#19 0x7f853027e80c content::ContentMainRunnerImpl::RunServiceManager()
#20 0x7f853027e411 content::ContentMainRunnerImpl::Run()
#21 0x7f85225cc4e3 service_manager::Main()
#22 0x7f853027c874 content::ContentMain()
#23 0x55ecacc7d873 ChromeMain
#24 0x7f8522b442b1 __libc_start_main
#25 0x55ecacc7d6ea _start

Comment 4 by isandrk@chromium.org, Today (14 hours ago)

Cc: isandrk@chromium.org

Comment 5 by mastiz@chromium.org, Today (13 hours ago)

Thanks for reaching out and sorry for the inconvenience: I'll land a patch soon.
Project Member

Comment 6 by bugdroid1@chromium.org, Today (12 hours ago)

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

commit c79fd2419a918afd184dfb25c7c8a004b5295944
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Jan 22 17:58:22 2019

Fix kSyncBagOfChips writing non-utf8 to prefs

https://chromium-review.googlesource.com/c/1421098 recently introduced
this pref with writes that include non-utf8 strings, causing DCHECK
failures.

Let's use base64 encoding when saving this into prefs.

Bug: 923285,924144
Change-Id: I13c5c6913050fa16d9b4d90e9dfcaf5c364fa9f9
Reviewed-on: https://chromium-review.googlesource.com/c/1426782
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624829}
[modify] https://crrev.com/c79fd2419a918afd184dfb25c7c8a004b5295944/components/sync/base/sync_prefs.cc

Comment 7 by mastiz@chromium.org, Today (12 hours ago)

DCHECK should be fixed by now.

Sign in to add a comment