Migrate invalidator client IDs out of sync

Migrate invalidator client IDs out of sync

We want to store invalidator client IDs somewhere other than the sync
DB.  Unfortunately, changing in the ID would require us to clear any
existing client state, which is something we'd prefer to avoid.

This commit makes the SyncInvalidationListener copy its sync-assigned
client ID to the InvalidationStateTracker when it is initialized.  When
we decide to fully separate the sync and notifier IDs, the clients that
have run through this code path will find that their notifier client ID
has already been initialized correctly.  This allows us to avoid
clearing their client state.

It's impossible to guarantee that all clients will hit this code path.
Fortunately, we don't have to.  The goal of this patch is to avoid
deleting the state of too many clients all at once.  It's not a big
problem if we have to clear the state of a few stragglers that haven't
hit the migration path yet.

BUG=124142
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180907

[rlarocque, 2013-02-01 23:26:46]

This is modeled after the bootstrap data migration.  Let's hope this works
equally well.  

For the curious, the follow-up to this patch is mostly written and can be found
at https://codereview.chromium.org/12092091/.

Tim: Please review.

[tim (not reviewing), 2013-02-05 20:04:33]

The CL to migrate invalidation state also included the code that would read from
the new storage and initialize the "proper" way if possible.  This allowed
additional early coverage of the newer code path as well as a more definitive
way to measure whether clients had actually passed through the state
successfully (since not needing to use the value from the old storage is the
strongest signal that migration occurred and worked). I didn't dig too deeply,
but is that not feasible here?

https://codereview.chromium.org/12094113/diff/2001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/12094113/diff/2001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:269:
pref_service_->GetString(prefs::kInvalidatorInvalidatorClientId) : "";
nit - prefer std::string() to ""

https://codereview.chromium.org/12094113/diff/2001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/12094113/diff/2001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:1717: "invalidator.invalidator_client_id";
nit- just invalidator.client_id seems like it'd be sufficient.

https://codereview.chromium.org/12094113/diff/2001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/12094113/diff/2001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:99: // TODO(rlarocque, 124142): This
call exists as part of an effort to move the
nit - ooh, newfangled todo format? otherwise I'd stick to TODO(rlarocque) and
mention the bug in the comment, since most people will be trained to grep for
that...

[rlarocque, 2013-02-05 21:35:56]

On 2013/02/05 20:04:33, timsteele wrote:
> The CL to migrate invalidation state also included the code that would read
from
> the new storage and initialize the "proper" way if possible.  This allowed
> additional early coverage of the newer code path as well as a more definitive
> way to measure whether clients had actually passed through the state
> successfully (since not needing to use the value from the old storage is the
> strongest signal that migration occurred and worked). I didn't dig too deeply,
> but is that not feasible here?
> 

I'd prefer not to.  The ID we send up to the server on every commit for
reflection-blocking is still the one provided by sync DB.  We can't get that
wrong; otherwise reflection blocking won't work.  There are some edge cases (ie.
SyncData.sqlite3 corrupted) where we could get it wrong if we're not careful.  

I think it's safer to maintain the existing logic as much as possible in this
patch.  

>
https://codereview.chromium.org/12094113/diff/2001/chrome/browser/sync/invali...
> File chrome/browser/sync/invalidations/invalidator_storage.cc (right):
> 
>
https://codereview.chromium.org/12094113/diff/2001/chrome/browser/sync/invali...
> chrome/browser/sync/invalidations/invalidator_storage.cc:269:
> pref_service_->GetString(prefs::kInvalidatorInvalidatorClientId) : "";
> nit - prefer std::string() to ""
> 
> https://codereview.chromium.org/12094113/diff/2001/chrome/common/pref_names.cc
> File chrome/common/pref_names.cc (right):
> 
>
https://codereview.chromium.org/12094113/diff/2001/chrome/common/pref_names.c...
> chrome/common/pref_names.cc:1717: "invalidator.invalidator_client_id";
> nit- just invalidator.client_id seems like it'd be sufficient.
> 
>
https://codereview.chromium.org/12094113/diff/2001/sync/notifier/sync_invalid...
> File sync/notifier/sync_invalidation_listener.cc (right):
> 
>
https://codereview.chromium.org/12094113/diff/2001/sync/notifier/sync_invalid...
> sync/notifier/sync_invalidation_listener.cc:99: // TODO(rlarocque, 124142):
This
> call exists as part of an effort to move the
> nit - ooh, newfangled todo format? otherwise I'd stick to TODO(rlarocque) and
> mention the bug in the comment, since most people will be trained to grep for
> that...

[rlarocque, 2013-02-05 21:36:56]

https://codereview.chromium.org/12094113/diff/2001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/12094113/diff/2001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:269:
pref_service_->GetString(prefs::kInvalidatorInvalidatorClientId) : "";
On 2013/02/05 20:04:33, timsteele wrote:
> nit - prefer std::string() to ""

Done.

https://codereview.chromium.org/12094113/diff/2001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/12094113/diff/2001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:1717: "invalidator.invalidator_client_id";
On 2013/02/05 20:04:33, timsteele wrote:
> nit- just invalidator.client_id seems like it'd be sufficient. 

Done.

https://codereview.chromium.org/12094113/diff/2001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/12094113/diff/2001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:99: // TODO(rlarocque, 124142): This
call exists as part of an effort to move the
On 2013/02/05 20:04:33, timsteele wrote:
> nit - ooh, newfangled todo format? otherwise I'd stick to TODO(rlarocque) and
> mention the bug in the comment, since most people will be trained to grep for
> that...

I thought I had seen that before... Maybe I just made it up.

Fixed.

[tim (not reviewing), 2013-02-05 22:05:28]

LGTM % nits.

https://codereview.chromium.org/12094113/diff/10001/chrome/browser/sync/inval...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/12094113/diff/10001/chrome/browser/sync/inval...
chrome/browser/sync/invalidations/invalidator_storage.cc:270: std::string();
nit - indent 4 spaces from pref_service_.

https://codereview.chromium.org/12094113/diff/10001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/12094113/diff/10001/chrome/common/pref_names....
chrome/common/pref_names.cc:1719: "invalidator.client_id";
nit - think this fits on the line above?

https://codereview.chromium.org/12094113/diff/10001/sync/notifier/sync_invali...
File sync/notifier/sync_invalidation_listener_unittest.cc (right):

https://codereview.chromium.org/12094113/diff/10001/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener_unittest.cc:308: // TODO(rlarocque,
124142): This is necessary for the deferred write of the
todo fixup here too (and elsewhere in patch) :)

[rlarocque, 2013-02-05 22:16:45]

https://codereview.chromium.org/12094113/diff/10001/chrome/browser/sync/inval...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/12094113/diff/10001/chrome/browser/sync/inval...
chrome/browser/sync/invalidations/invalidator_storage.cc:270: std::string();
On 2013/02/05 22:05:28, timsteele wrote:
> nit - indent 4 spaces from pref_service_.

Done.

https://codereview.chromium.org/12094113/diff/10001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/12094113/diff/10001/chrome/common/pref_names....
chrome/common/pref_names.cc:1719: "invalidator.client_id";
On 2013/02/05 22:05:28, timsteele wrote:
> nit - think this fits on the line above?

Done.

https://codereview.chromium.org/12094113/diff/10001/sync/notifier/sync_invali...
File sync/notifier/sync_invalidation_listener_unittest.cc (right):

https://codereview.chromium.org/12094113/diff/10001/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener_unittest.cc:308: // TODO(rlarocque,
124142): This is necessary for the deferred write of the
On 2013/02/05 22:05:28, timsteele wrote:
> todo fixup here too (and elsewhere in patch) :)

Done.  I think I got them all this time.

[commit-bot: I haz the power, 2013-02-05 22:42:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/12094113/11011

[commit-bot: I haz the power, 2013-02-05 23:32:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-06 00:12:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/12094113/4005

[commit-bot: I haz the power, 2013-02-06 06:51:39]

Change committed as 180907