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

Issue 686172 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

[USS] Write integration tests for USS autocomplete

Project Member Reported by s...@chromium.org, Jan 27 2017

Issue description

This includes adding the bot config! Don't forget.
 

Comment 1 by s...@chromium.org, Feb 1 2017

1. out/Default/chrome --enable-features=SyncUSSAutocomplete
2. Sign in, enable syncing of autofill
3. Disable syncing of autofill

It seems the ModelTypeController is calling GetSyncBridgeForModelType in preparation for calling DisableSync. But GetSyncBridgeForModelType is called on the UI thread which autocomplete/AutofillWebDataService does not like. Looks like we mostly worked around this in https://codereview.chromium.org/2508263003/diff/100001/components/sync/driver/model_type_controller.cc but this particular call sight was missed. This is blocking integ tests, as pretty much every integ test that enables autofill hits this during cleanup. See https://codereview.chromium.org/2672493002/#ps1

[39991:39991:0201/100321.264469:FATAL:autofill_webdata_service.cc(273)] Check failed: db_thread_->BelongsToCurrentThread(). 
#0 0x7fdc5f296297 base::debug::StackTrace::StackTrace()
#1 0x7fdc5f2ba38b logging::LogMessage::~LogMessage()
#2 0x7fdc60a266d5 autofill::AutofillWebDataService::GetDBUserData()
#3 0x7fdc60a031c5 autofill::AutocompleteSyncBridge::FromWebDataService()
#4 0x7fdc603e3745 browser_sync::ChromeSyncClient::GetSyncBridgeForModelType()
#5 0x7fdc618ace8a syncer::ModelTypeController::Stop()
#6 0x7fdc618a8c6d syncer::ModelAssociationManager::StopDisabledTypes()
#7 0x7fdc618a8a2a syncer::ModelAssociationManager::Initialize()
#8 0x7fdc6189aeba syncer::DataTypeManagerImpl::Restart()
#9 0x7fdc6189a0da syncer::DataTypeManagerImpl::ConfigureImpl()
#10 0x7fdc61899ef6 syncer::DataTypeManagerImpl::Configure()
#11 0x7fdc60cd3825 browser_sync::ProfileSyncService::ConfigureDataTypeManager()
#12 0x7fdc60cdb92d browser_sync::ProfileSyncService::ReconfigureDatatypeManager()
#13 0x7fdc60cdc023 browser_sync::ProfileSyncService::OnSetupInProgressHandleDestroyed()
#14 0x7fdc5fe0eeaa _ZN4base8internal13FunctorTraitsIMN11google_apis19UrlFetchRequestBaseEFvvEvE6InvokeIRKNS_7WeakPtrINS2_5drive30SingleBatchableDelegateRequestEEEJEEEvS5_OT_DpOT0_
#15 0x7fdc606eb3a1 syncer::SyncSetupInProgressHandle::~SyncSetupInProgressHandle()
#16 0x7fdc60f570d0 SyncSetupHandler::CloseSyncSetup()
#17 0x7fdc5ce7d5c0 content::WebUIImpl::ProcessWebUIMessage()
#18 0x7fdc60f82b66 UberUI::OverrideHandleWebUIMessage()
#19 0x7fdc5ce7d593 content::WebUIImpl::ProcessWebUIMessage()
#20 0x7fdc5ce7c678 content::WebUIImpl::OnWebUISend()
#21 0x7fdc5ce7c493 _ZN3IPC8MessageTI26ViewHostMsg_WebUISend_MetaSt5tupleIJ4GURLSsN4base9ListValueEEEvE8DispatchIN7content9WebUIImplESA_vMSA_FvRKS3_RKSsRKS5_EEEbPKNS_7MessageEPT_PT0_PT1_T2_
#22 0x7fdc5ce7c3a7 content::WebUIImpl::OnMessageReceived()
#23 0x7fdc5ce4edf0 content::WebContentsImpl::OnMessageReceived()
#24 0x7fdc5cd3e2cb content::RenderViewHostImpl::OnMessageReceived()
#25 0x7fdc5cd43233 content::RenderWidgetHostImpl::OnMessageReceived()
#26 0x7fdc5cd31ace content::RenderProcessHostImpl::OnMessageReceived()
#27 0x7fdc5f6da2b5 IPC::ChannelProxy::Context::OnDispatchMessage()
#28 0x7fdc5f6dd25a _ZN4base8internal7InvokerINS0_9BindStateIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEJ13scoped_refptrIS5_ES6_EEEFvvEE3RunEPNS0_13BindStateBaseE
#29 0x7fdc5f296d5e base::debug::TaskAnnotator::RunTask()
#30 0x7fdc5f2c7dcd base::MessageLoop::RunTask()
#31 0x7fdc5f2c8766 base::MessageLoop::DoWork()
#32 0x7fdc5f2caa1a base::(anonymous namespace)::WorkSourceDispatch()
#33 0x7fdc58426e04 g_main_context_dispatch
#34 0x7fdc58427048 <unknown>
#35 0x7fdc584270ec g_main_context_iteration
#36 0x7fdc5f2ca776 base::MessagePumpGlib::Run()
#37 0x7fdc5f2c7b25 base::MessageLoop::RunHandler()
#38 0x7fdc5f2fc58c base::RunLoop::Run()
#39 0x7fdc604a0b1a ChromeBrowserMainParts::MainMessageLoopRun()
#40 0x7fdc5ca18ef9 content::BrowserMainLoop::RunMainMessageLoopParts()
#41 0x7fdc5ca1cc77 content::BrowserMainRunnerImpl::Run()
#42 0x7fdc5ca1400e content::BrowserMain()
#43 0x7fdc5d202fb7 content::RunNamedProcessTypeMain()
#44 0x7fdc5d203a06 content::ContentMainRunnerImpl::Run()
#45 0x7fdc5d202330 content::ContentMain()
#46 0x7fdc5fdb9c81 ChromeMain
#47 0x7fdc55814f45 __libc_start_main
#48 0x7fdc5fdb9b19 <unknown>

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 2 2017

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

commit 08754c122f848c02be04f9858a623ec706fbca34
Author: skym <skym@chromium.org>
Date: Thu Feb 02 16:38:55 2017

[Sync] Incorporate value into USS AUTOFILL non_unique_name.

The directory based AUTOFILL implemented used to set
autofill_entry|name|value as the non_unique_name. During the USS
rewrite this was changed to just be the name of the form field. This is
less helpful when looking at sync-internals, you may have many entries
for the same form with different values. It would be nice if this name
distinguished them.

Thinking about the cost of increasing non_unique_name values, it
directly impacts bandwidth usage, being part of commits and GUs, though
it does get compressed. We don't seem to keep this value in memory at
runtime when steady state, which is really the most important scenario.

BUG= 686172 

Review-Url: https://codereview.chromium.org/2663423002
Cr-Commit-Position: refs/heads/master@{#447776}

[modify] https://crrev.com/08754c122f848c02be04f9858a623ec706fbca34/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 2 2017

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

commit c3389b63ea8217144db344e29451b9ee3bbd05e3
Author: skym <skym@chromium.org>
Date: Thu Feb 02 16:56:58 2017

[Sync] Only get bridge on model thread for ModelTypeController.

AutocompleteSyncBridge cannot be fetched on the UI thread, because of
the way AutofillWebDataService provides it. This can only be done on
the model thread. Previously the ModelTypeController was updated to
mostly respect this in https://codereview.chromium.org/2508263003/ .
However, we seem to have missed the DisableSync() case. This CL copies
the existing pattern for this scenario.

BUG= 686172 

Review-Url: https://codereview.chromium.org/2668323002
Cr-Commit-Position: refs/heads/master@{#447779}

[modify] https://crrev.com/c3389b63ea8217144db344e29451b9ee3bbd05e3/components/sync/driver/model_type_controller.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 3 2017

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

commit d09c38982b3b999ecfa67acfb8018d0b198bc4bd
Author: skym <skym@chromium.org>
Date: Fri Feb 03 22:09:02 2017

[Sync] Make EnableDisableSingleClientTest more robust.

EnableDisableSingleClientTest's tests enable or disable a type, and
then go verify that the root node was or was not present. This doesn't
work for USS, as USS doesn't create root nodes. So ever time we migrate
a USS type we have to update this file. However, this was a difficult
task because of the way it was set up. Full or hard coded rules about
relationships, and dependent on specific orderings of ModelType. The
most egregious violation being it would call enable/disable on non-user
selectable types, this would no-op, and then it'd verify it had the
correct affect, because typically these grouped types are preceded by
their corresponding user selectable type, and this test went through
the types in order.

To fix this, we start by making the test harness return false (failure)
if you try to enable/disable a non-selectable type. This in turns means
that EnableDisableSingleClientTest's cases need to understand some of
this type group mapping. So SyncPrefs was updated to expose
ResolvePrefGroups to allow intelligent tests. Not only does this allow
us to remove most hard coded type rules, but we are also able to test
on a more precise level, knowing typically exactly which types should
change as a result of an enable/disable. The main caveat being types
that are mapped to by multiple selectable types, which we're slightly
less strictly testing when they change.

Also updated test to guess that any new type of syncer::PRINTERS or
later will be USS. This strives to reduce the pain of adding new types,
although it is a risk in that the logic is fragile.

BUG= 686172 

Review-Url: https://codereview.chromium.org/2670903002
Cr-Commit-Position: refs/heads/master@{#448085}

[modify] https://crrev.com/d09c38982b3b999ecfa67acfb8018d0b198bc4bd/chrome/browser/sync/test/integration/enable_disable_test.cc
[modify] https://crrev.com/d09c38982b3b999ecfa67acfb8018d0b198bc4bd/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
[modify] https://crrev.com/d09c38982b3b999ecfa67acfb8018d0b198bc4bd/components/sync/base/sync_prefs.h

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 15 2017

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

commit 6261bc1e8181556af9c67ad860cca8ebfc3469e0
Author: skym <skym@chromium.org>
Date: Wed Feb 15 17:26:58 2017

[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill would consistently crash here by posting a task to
the model thread, and before that gets resolved, the SyncClient is
destroyed. Presumably all the other posts to model thread were unsafe
as well, though hitting the race condition was more unlikely.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG= 686172 

Review-Url: https://codereview.chromium.org/2672493002
Cr-Commit-Position: refs/heads/master@{#450736}

[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/chrome/browser/sync/test/integration/enable_disable_test.cc
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/BUILD.gn
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/autocomplete_sync_bridge.h
[add] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/web_data_model_type_controller.cc
[add] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/autofill/core/browser/webdata/web_data_model_type_controller.h
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/model/model_type_debug_info.cc
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/components/sync/model/model_type_debug_info.h
[modify] https://crrev.com/6261bc1e8181556af9c67ad860cca8ebfc3469e0/testing/variations/fieldtrial_testing_config.json

Comment 6 by s...@chromium.org, Feb 22 2017

Status: Fixed (was: Started)
This is done, marking a such.
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 10 2017

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

commit 903ebf3235f4057351f7734c0e148e7938009dbd
Author: skym <skym@chromium.org>
Date: Mon Apr 10 17:59:38 2017

[Sync] Post back to UI thread for USS GetAllNodes.

When running GetAllNodes, USS types post to the model thread to access
their processor/bridge. This means after they've gathered the nodes,
they need to return to the UI thread to give the results back to the
GetAllNodesRequestHelper.

Unfortunately I removed a wrapping BindToCurrentThread call in
https://codereview.chromium.org/2672493002/diff/80001/components/sync/driver/model_type_controller.cc
that did the jump back to the UI thread. This meant that non-UI USS
types (namely AUTOFILL) were updating GetAllNodesRequestHelper from a
different thread, through very non-thread safe code. To fix simply
added the BindToCurrentThread back.

Also added a added a thread checker to GetAllNodesRequestHelper in
hopes of stopping similar bugs from occurring again.

BUG= 686172 

Review-Url: https://codereview.chromium.org/2808113003
Cr-Commit-Position: refs/heads/master@{#463320}

[modify] https://crrev.com/903ebf3235f4057351f7734c0e148e7938009dbd/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/903ebf3235f4057351f7734c0e148e7938009dbd/components/sync/driver/model_type_controller.cc

Sign in to add a comment