[Sync] InvalidationService shouldn't CHECK when registering ObjectId for more than one handler

[Sync] InvalidationService shouldn't CHECK when registering ObjectId for more than one handler

Before this change InvalidationService CHECKs that object id is only
registered with one handler. This causes browser crash when object id is
received from network already registered.

Solution is to let each component that uses InvalidationService decide
how to react to duplicate registration.
In this change:
- InvalidationService::UpdateRegisteredInvalidationIds doesn't CHECK
  inside, but returns boolean that indicates if update was successful.
- All places that call this function do CHECK to preserve existing
  behavior.
- Internal implementations and tests are updated accordingly.

BUG=475941
R=maniscalco@chromium.org

Committed: https://crrev.com/8131d2b4b0c296269c8f1bb8e0ed9dfbd5e852ba
Cr-Commit-Position: refs/heads/master@{#330654}

[maniscalco, 2015-05-18 15:42:28]

Just a few comments.

What's the plan for updating the call sites to do something smarter than CHECK
the result?  Do you plan to notify the OWNERS or were you going to update the
call sites?

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
File components/invalidation/invalidation_service_test_template.h (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
components/invalidation/invalidation_service_test_template.h:414: // {PAV} Add
test for false
Remove?

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
File components/invalidation/invalidator_registrar.cc (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
components/invalidation/invalidator_registrar.cc:50: DVLOG(0) << "Duplicate
registration: trying to register "
Can you walk me through the thought process on the log level / severity to use
here?  This case is definitely an error of some kind, right?  Would LOG(ERROR) 
more appropriate?

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/tic...
File components/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/tic...
components/invalidation/ticl_invalidation_service.cc:131:
CHECK(invalidator_->UpdateRegisteredIds(
I see this method (TiclInvalidationService::UpdateRegisteredInvalidationIds) can
now return a boolean result.  Should we be returning false here instead of
CHECKing?

[pavely, 2015-05-18 18:38:05]

PTAL.

> What's the plan for updating the call sites to do something smarter than CHECK
the result?  Do you plan to notify the OWNERS or were you going to update the
call sites?

I will notify bartfab@ that he can change cloud_policy_invalidator.cc to report
duplicates.
For the rest of call sites CHECK is appropriate, registering for ObjectId twice
is not expected.

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
File components/invalidation/invalidation_service_test_template.h (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
components/invalidation/invalidation_service_test_template.h:414: // {PAV} Add
test for false
On 2015/05/18 15:42:28, maniscalco wrote:
> Remove?

Thank you.

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
File components/invalidation/invalidator_registrar.cc (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
components/invalidation/invalidator_registrar.cc:50: DVLOG(0) << "Duplicate
registration: trying to register "
On 2015/05/18 15:42:28, maniscalco wrote:
> Can you walk me through the thought process on the log level / severity to use
> here?  This case is definitely an error of some kind, right?  Would LOG(ERROR)

> more appropriate?

I think LOG(ERROR) and DVLOG(0) have the same effect, but LOG(ERROR) is more
widely used and more descriptive so LOG(ERROR) it is.

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/tic...
File components/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/tic...
components/invalidation/ticl_invalidation_service.cc:131:
CHECK(invalidator_->UpdateRegisteredIds(
On 2015/05/18 15:42:28, maniscalco wrote:
> I see this method (TiclInvalidationService::UpdateRegisteredInvalidationIds)
can
> now return a boolean result.  Should we be returning false here instead of
> CHECKing?

invalidator_ is private to TiclInvalidationService, the only handler possible
for it is TiclInvalidationService itself. Situation when there are two handlers
registering for the same ObjectId is invalid here. If it happens, we should
catch it early.

[maniscalco, 2015-05-18 18:56:06]

LGTM

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
File components/invalidation/invalidator_registrar.cc (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/inv...
components/invalidation/invalidator_registrar.cc:50: DVLOG(0) << "Duplicate
registration: trying to register "
On 2015/05/18 18:38:04, pavely wrote:
> On 2015/05/18 15:42:28, maniscalco wrote:
> > Can you walk me through the thought process on the log level / severity to
use
> > here?  This case is definitely an error of some kind, right?  Would
LOG(ERROR)
> 
> > more appropriate?
> 
> I think LOG(ERROR) and DVLOG(0) have the same effect, but LOG(ERROR) is more
> widely used and more descriptive so LOG(ERROR) it is.

Cool.  As we discussed offline, the V in DVLOG stands for verbose so DVLOG(0) is
actually an "INFO".

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/tic...
File components/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/1146533005/diff/1/components/invalidation/tic...
components/invalidation/ticl_invalidation_service.cc:131:
CHECK(invalidator_->UpdateRegisteredIds(
On 2015/05/18 18:38:04, pavely wrote:
> On 2015/05/18 15:42:28, maniscalco wrote:
> > I see this method (TiclInvalidationService::UpdateRegisteredInvalidationIds)
> can
> > now return a boolean result.  Should we be returning false here instead of
> > CHECKing?
> 
> invalidator_ is private to TiclInvalidationService, the only handler possible
> for it is TiclInvalidationService itself. Situation when there are two
handlers
> registering for the same ObjectId is invalid here. If it happens, we should
> catch it early.

SGTM

[pavely, 2015-05-18 18:57:12]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2015-05-18 18:58:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146533005/20001

[commit-bot: I haz the power, 2015-05-18 20:12:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-18 20:12:26]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[pavely, 2015-05-18 20:51:27]

pavely@chromium.org changed reviewers:
+ bartfab@chromium.org, kinaba@chromium.org

[pavely, 2015-05-18 20:53:49]

Adding reviewers for call sites. Guys, PTAL.
kinaba@: chrome/browser/drive/drive_notification_manager.cc
bartfab@: chrome/browser/policy/cloud/*

[kinaba, 2015-05-19 01:59:10]

> kinaba@: chrome/browser/drive/drive_notification_manager.cc
lgtm

[bartfab (slow), 2015-05-19 10:44:43]

https://codereview.chromium.org/1146533005/diff/60001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/1146533005/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:311:
CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));
Nit 1: #include "base/logging.h"
Nit 2: I think a LOG(ERROR) is more appropriate than a CHECK(). If we receive
invalid data from the cloud, invalidation will not work - no need to crash the
whole browser because of that.

https://codereview.chromium.org/1146533005/diff/60001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/remote_commands_invalidator.cc (right):

https://codereview.chromium.org/1146533005/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/cloud/remote_commands_invalidator.cc:141:
CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));
Nit: As in the other file, LOG(ERROR) would be sufficient here.

[pavely, 2015-05-19 17:54:56]

https://codereview.chromium.org/1146533005/diff/60001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/1146533005/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:311:
CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));
On 2015/05/19 10:44:42, bartfab wrote:
> Nit 1: #include "base/logging.h"
> Nit 2: I think a LOG(ERROR) is more appropriate than a CHECK(). If we receive
> invalid data from the cloud, invalidation will not work - no need to crash the
> whole browser because of that.

#1: Done

#2: I wanted this change to preserve existing behavior, but allow call sites to
do something smarter than just crash. LOG(ERROR) would work but would not allow
us to see issues: invalid data would cause degraded experience and we'll never
find out about it.
You proposed earlier to report UMA histogram in such situations, this is the
place to do it. Let's land this change as it is and you'll add appropriate
reaction.

[bartfab (slow), 2015-05-19 22:19:13]

policy lgtm

[pavely, 2015-05-19 22:51:22]

The CQ bit was checked by pavely@chromium.org

[pavely, 2015-05-19 22:51:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
maniscalco@chromium.org, kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/1146533005/#ps80001
(title: "Addressed comments from Bartosz.")

[commit-bot: I haz the power, 2015-05-19 22:52:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146533005/80001

[commit-bot: I haz the power, 2015-05-20 00:27:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-20 00:28:48]

Patchset 5 (id:??) landed as
https://crrev.com/8131d2b4b0c296269c8f1bb8e0ed9dfbd5e852ba
Cr-Commit-Position: refs/heads/master@{#330654}