Disable tab syncing when history saving is disabled.

Disable tab syncing when history saving is disabled.

The history page shows the current tabs of synced devices. This works by
having the client upload its open tabs to the server, which feeds this
data into the history backend.

If history saving is disabled by policy then the opens tabs should not
be uploaded to the server. This also prevents them from showing up in
the history page.

BUG=292976, 372108, 393051
Committed: https://crrev.com/e3fcbab8a44333d2f10c5dd73cce9b879e44bd2b
Cr-Commit-Position: refs/heads/master@{#292123}

[Joao da Silva, 2014-07-21 08:42:28]

PTAL:

@tim: git history says you're a good reviewer for
session_data_type_controller.cc, can you have a look?

@engedy: as discussed offline. Let me know if you have any concerns.

Thanks!

[Joao da Silva, 2014-08-20 07:25:55]

+atwilson

[Joao da Silva, 2014-08-21 12:46:14]

Just finished manual verification that this works as intended, incl. when the
policy changes at runtime.

Drew, can you have a look? Thanks!

[Andrew T Wilson (Slow), 2014-08-21 14:10:38]

+zea
Mostly LG, although i'd like to understand what the right thing is to do in
NonFrontendDataTypeController wrt which threads are allowed to generate
unrecoverable errors (important in the case that the backend thread no longer
exists).

Would also like to see if the code in LoadModels() in SessionDataTypeController
is required. If not, maybe we can remove from TypedUrlDataTypeController too?

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (left):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:295:
DCHECK(IsOnBackendThread());
Nicolas there are a few things we noticed here:

1) TypedUrlDataTypeController is a NonFrontendDataTypeController. However, if
the history service gets disabled, *there is no more backend thread*, but we
need to generate an unrecoverable error. The only thread we can do this on is
the UI thread, but it hits this DCHECK.

We can either remove this DCHECK(), or else we can call
RecordUnrecoverableError() and DisableImpl() directly from
TypedUrlDataTypeController(). What is the right way to go?

2) It looks like RecordUnrecoverableError() can be called from any thread,
because it's called both from here (backend thread) and also from StartDone().
This means that the error_callback_ can be invoked from any thread. Is this
correct/intended?

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_components_factory_impl.cc:229:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
 Move this inside the if(pref) below?

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:50:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
OK to land this bit as-is, but ping Nicolas Zea whether we can just remove this
since we should never get LoadModels() if the policy is set during startup, and
if the policy changes at runtime, OnSavingBrowserHistoryPrefChanged() should
handle generating errors/stopping sync for this type.

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.h (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.h:38: virtual void
LoadModels(const ModelLoadCallback& model_load_callback)
Please don't change visibility of APIs in subclasses.

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller_unittest.cc:122:
sync_driver::UIDataTypeController* controller = controller_;
Remove this.

https://codereview.chromium.org/394223002/diff/80001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/394223002/diff/80001/components/policy/resour...
components/policy/resources/policy_templates.json:520: If this setting is
enabled, browsing history is not saved. This setting also disables tab syncing,
since synced tabs are also sent to the history database.
"also sent to the history db" is not strictly true. It's synced to the server.
Maybe just say "also disables tab syncing." and that's it.

[Joao da Silva, 2014-08-26 16:06:52]

Addressed Drew's comments.

Nicolas, can you have a look at Drew's comments?

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_components_factory_impl.cc:229:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
On 2014/08/21 14:10:37, Andrew T Wilson wrote:
>  Move this inside the if(pref) below?

Done.

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:50:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/21 14:10:37, Andrew T Wilson wrote:
> OK to land this bit as-is, but ping Nicolas Zea whether we can just remove
this
> since we should never get LoadModels() if the policy is set during startup,
and
> if the policy changes at runtime, OnSavingBrowserHistoryPrefChanged() should
> handle generating errors/stopping sync for this type.

I don't fully understand how the sync data type controllers work with the sync
backends; this snippet was taken from TypedUrlDataTypeController::LoadModels, so
maybe that should be fixed too.

Nicolas, ping :-)

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.h (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.h:38: virtual void
LoadModels(const ModelLoadCallback& model_load_callback)
On 2014/08/21 14:10:37, Andrew T Wilson wrote:
> Please don't change visibility of APIs in subclasses.

Done.

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller_unittest.cc:122:
sync_driver::UIDataTypeController* controller = controller_;
On 2014/08/21 14:10:38, Andrew T Wilson wrote:
> Remove this.

Done.

https://codereview.chromium.org/394223002/diff/80001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/394223002/diff/80001/components/policy/resour...
components/policy/resources/policy_templates.json:520: If this setting is
enabled, browsing history is not saved. This setting also disables tab syncing,
since synced tabs are also sent to the history database.
On 2014/08/21 14:10:38, Andrew T Wilson wrote:
> "also sent to the history db" is not strictly true. It's synced to the server.
> Maybe just say "also disables tab syncing." and that's it.

Done.

[Nicolas Zea, 2014-08-26 17:59:30]

zea@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2014-08-26 17:59:30]

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (left):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:295:
DCHECK(IsOnBackendThread());
On 2014/08/21 14:10:37, Andrew T Wilson wrote:
> Nicolas there are a few things we noticed here:
> 
> 1) TypedUrlDataTypeController is a NonFrontendDataTypeController. However, if
> the history service gets disabled, *there is no more backend thread*, but we
> need to generate an unrecoverable error. The only thread we can do this on is
> the UI thread, but it hits this DCHECK.
> 
> We can either remove this DCHECK(), or else we can call
> RecordUnrecoverableError() and DisableImpl() directly from
> TypedUrlDataTypeController(). What is the right way to go?
> 
> 2) It looks like RecordUnrecoverableError() can be called from any thread,
> because it's called both from here (backend thread) and also from StartDone().
> This means that the error_callback_ can be invoked from any thread. Is this
> correct/intended?

I think you should be fine just calling DisableImpl. RecordUnrecoverableError is
just for UMA tracking. Theoretically (last I checked) a specific UMA histogram
callsite is not thread safe (it uses some weird static variables under the
hood), so I'd just avoid calling it altogether for policy errors. Plus, they're
errors by design, rather than bugs/unknown issues.

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:50:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/26 16:06:51, Joao da Silva wrote:
> On 2014/08/21 14:10:37, Andrew T Wilson wrote:
> > OK to land this bit as-is, but ping Nicolas Zea whether we can just remove
> this
> > since we should never get LoadModels() if the policy is set during startup,
> and
> > if the policy changes at runtime, OnSavingBrowserHistoryPrefChanged() should
> > handle generating errors/stopping sync for this type.
> 
> I don't fully understand how the sync data type controllers work with the sync
> backends; this snippet was taken from TypedUrlDataTypeController::LoadModels,
so
> maybe that should be fixed too.
> 
> Nicolas, ping :-)

There's actually a new method you can override, ReadyForStart, which is better
for this kind of check. TypedURLDataTypeController should probably be using that
too. It's called on every configuration, and stops the datatype if it's not
true.

ExtensionBackedDataTypeController is an example that uses both ReadyForStar and
OnSingleDataTypeUnrecoverableError for a similar purpose (disabling a datatype
if an extension is not installed).

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:55:
syncer::SyncError::DATATYPE_ERROR,
nit: this should be DATATYPE_POLICY_ERROR (same in Typed url data type
controller)

[Joao da Silva, 2014-08-26 18:43:03]

Thanks for the pointers. Everything still works as expected after implementing
ReadyForStart() instead of LoadModels().

PTAL, thanks!

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (left):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:295:
DCHECK(IsOnBackendThread());
On 2014/08/26 17:59:30, Nicolas Zea wrote:
> On 2014/08/21 14:10:37, Andrew T Wilson wrote:
> > Nicolas there are a few things we noticed here:
> > 
> > 1) TypedUrlDataTypeController is a NonFrontendDataTypeController. However,
if
> > the history service gets disabled, *there is no more backend thread*, but we
> > need to generate an unrecoverable error. The only thread we can do this on
is
> > the UI thread, but it hits this DCHECK.
> > 
> > We can either remove this DCHECK(), or else we can call
> > RecordUnrecoverableError() and DisableImpl() directly from
> > TypedUrlDataTypeController(). What is the right way to go?
> > 
> > 2) It looks like RecordUnrecoverableError() can be called from any thread,
> > because it's called both from here (backend thread) and also from
StartDone().
> > This means that the error_callback_ can be invoked from any thread. Is this
> > correct/intended?
> 
> I think you should be fine just calling DisableImpl. RecordUnrecoverableError
is
> just for UMA tracking. Theoretically (last I checked) a specific UMA histogram
> callsite is not thread safe (it uses some weird static variables under the
> hood), so I'd just avoid calling it altogether for policy errors. Plus,
they're
> errors by design, rather than bugs/unknown issues.

Thanks for clarifying, done.

Unrelated to this CL: note that RecordUnrecoverableError is called from 2
places:

- OnSingleDataTypeUnrecoverableError, on the backend thread
- StartDone, on the UI thread

And then RecordUnrecoverableError invokes the error_callback_ on either of those
threads; shouldn't the callback be invoked in the same thread in both cases?

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:50:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/26 17:59:30, Nicolas Zea wrote:
> On 2014/08/26 16:06:51, Joao da Silva wrote:
> > On 2014/08/21 14:10:37, Andrew T Wilson wrote:
> > > OK to land this bit as-is, but ping Nicolas Zea whether we can just remove
> > this
> > > since we should never get LoadModels() if the policy is set during
startup,
> > and
> > > if the policy changes at runtime, OnSavingBrowserHistoryPrefChanged()
should
> > > handle generating errors/stopping sync for this type.
> > 
> > I don't fully understand how the sync data type controllers work with the
sync
> > backends; this snippet was taken from
TypedUrlDataTypeController::LoadModels,
> so
> > maybe that should be fixed too.
> > 
> > Nicolas, ping :-)
> 
> There's actually a new method you can override, ReadyForStart, which is better
> for this kind of check. TypedURLDataTypeController should probably be using
that
> too. It's called on every configuration, and stops the datatype if it's not
> true.
> 
> ExtensionBackedDataTypeController is an example that uses both ReadyForStar
and
> OnSingleDataTypeUnrecoverableError for a similar purpose (disabling a datatype
> if an extension is not installed).

Done.

https://codereview.chromium.org/394223002/diff/80001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:55:
syncer::SyncError::DATATYPE_ERROR,
On 2014/08/26 17:59:30, Nicolas Zea wrote:
> nit: this should be DATATYPE_POLICY_ERROR (same in Typed url data type
> controller)

Done.

[Nicolas Zea, 2014-08-26 19:37:50]

sync LGTM

And yeah, I suspect RecordUnrecoverableError is being called incorrectly. I've
filed 407764 to investigate.

[Andrew T Wilson (Slow), 2014-08-27 08:06:31]

lgtm

[Joao da Silva, 2014-08-27 08:20:04]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-08-27 08:20:46]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/394223002/120001

[commit-bot: I haz the power, 2014-08-27 09:07:13]

Committed patchset #7 (120001) as dafb8f27eabb86be48f11b87ed7d648bbc64d6c5

[commit-bot: I haz the power, 2014-09-10 02:50:39]

Patchset 7 (id:??) landed as
https://crrev.com/e3fcbab8a44333d2f10c5dd73cce9b879e44bd2b
Cr-Commit-Position: refs/heads/master@{#292123}