Add BackupRollbackController for starting backup/rollback

BackupRollbackController is notified when
signin/sync status changed. It invokes given backup/rollback callbacks if conditions are met.

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

[Nicolas Zea, 2014-05-06 18:54:08]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.cc (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.cc:54:
sync_prefs_->SetRemainingRollbackTries(kRollbackLimits);
Why do we have a limitted number of tries? What controls an attempt failing?

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
I'm not sure this is the class you want to use. Do you specifically care about
managed users here? Wouldn't SigningManagerBase be more appropriate?

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:31: void Reset(base::TimeDelta
delay);
This isn't actually resetting anything right? Perhaps call this Start? Also,
comment what |delay| is for.

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:48:
base::WeakPtrFactory<BackupRollbackController> weak_ptr_factory_;
disallow copy and assign

[haitaol1, 2014-05-07 21:34:17]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.cc (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.cc:54:
sync_prefs_->SetRemainingRollbackTries(kRollbackLimits);
It's not good to be stuck in rollback mode. Backend is shut down on
unrecoverable error, which counts one rollback failure. If the unrecoverable
error is due to corrupted backup DB, setting limits on rollback will prevents
stuck.

On 2014/05/06 18:54:09, Nicolas Zea wrote:
> Why do we have a limitted number of tries? What controls an attempt failing?

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
By using ManagedUserSigninManagerWrapper, backup/rollback is never started for
managed user because planpen uses a pseudo account always. This is probably the
safe thing to do.

On 2014/05/06 18:54:09, Nicolas Zea wrote:
> I'm not sure this is the class you want to use. Do you specifically care about
> managed users here? Wouldn't SigningManagerBase be more appropriate?

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:31: void Reset(base::TimeDelta
delay);
On 2014/05/06 18:54:09, Nicolas Zea wrote:
> This isn't actually resetting anything right? Perhaps call this Start? Also,
> comment what |delay| is for.

Done.

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:48:
base::WeakPtrFactory<BackupRollbackController> weak_ptr_factory_;
On 2014/05/06 18:54:09, Nicolas Zea wrote:
> disallow copy and assign

Done.

[Nicolas Zea, 2014-05-08 21:43:08]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.cc (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.cc:54:
sync_prefs_->SetRemainingRollbackTries(kRollbackLimits);
On 2014/05/07 21:34:17, haitaol1 wrote:
> It's not good to be stuck in rollback mode. Backend is shut down on
> unrecoverable error, which counts one rollback failure. If the unrecoverable
> error is due to corrupted backup DB, setting limits on rollback will prevents
> stuck.
> 
> On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > Why do we have a limitted number of tries? What controls an attempt failing?
> 

It seems odd that someone could prevent rollback by just restarting a couple
times. What's the consequence of being in a stuck rollback mode. That they can't
set up sync? That seems okay, since we stop attempting rollback once they sign
in, right?

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
On 2014/05/07 21:34:17, haitaol1 wrote:
> By using ManagedUserSigninManagerWrapper, backup/rollback is never started for
> managed user because planpen uses a pseudo account always. This is probably
the
> safe thing to do.
> 
> On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > I'm not sure this is the class you want to use. Do you specifically care
about
> > managed users here? Wouldn't SigningManagerBase be more appropriate?
> 

Is this something we want to enforce at the client though? This seems like a
strange detail to have here, and likely something that could be enforced at the
server.

[haitaol1, 2014-05-09 16:55:57]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.cc (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.cc:54:
sync_prefs_->SetRemainingRollbackTries(kRollbackLimits);
The user changes not in backup db will be constantly removed. That would be a
bad user experience.


On 2014/05/08 21:43:09, Nicolas Zea wrote:
> On 2014/05/07 21:34:17, haitaol1 wrote:
> > It's not good to be stuck in rollback mode. Backend is shut down on
> > unrecoverable error, which counts one rollback failure. If the unrecoverable
> > error is due to corrupted backup DB, setting limits on rollback will
prevents
> > stuck.
> > 
> > On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > > Why do we have a limitted number of tries? What controls an attempt
failing?
> > 
> 
> It seems odd that someone could prevent rollback by just restarting a couple
> times. What's the consequence of being in a stuck rollback mode. That they
can't
> set up sync? That seems okay, since we stop attempting rollback once they sign
> in, right?

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
How can server enforce this given that backup backend never contacts server?

On 2014/05/08 21:43:09, Nicolas Zea wrote:
> On 2014/05/07 21:34:17, haitaol1 wrote:
> > By using ManagedUserSigninManagerWrapper, backup/rollback is never started
for
> > managed user because planpen uses a pseudo account always. This is probably
> the
> > safe thing to do.
> > 
> > On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > > I'm not sure this is the class you want to use. Do you specifically care
> about
> > > managed users here? Wouldn't SigningManagerBase be more appropriate?
> > 
> 
> Is this something we want to enforce at the client though? This seems like a
> strange detail to have here, and likely something that could be enforced at
the
> server.

[haitaol1, 2014-05-13 16:23:28]

ping

[Nicolas Zea, 2014-05-13 17:39:10]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
The command to roll back in the first place comes from the server, and should
never be sent to a managed user, right?

On 2014/05/09 16:55:58, haitaol1 wrote:
> How can server enforce this given that backup backend never contacts server?
> 
> On 2014/05/08 21:43:09, Nicolas Zea wrote:
> > On 2014/05/07 21:34:17, haitaol1 wrote:
> > > By using ManagedUserSigninManagerWrapper, backup/rollback is never started
> for
> > > managed user because planpen uses a pseudo account always. This is
probably
> > the
> > > safe thing to do.
> > > 
> > > On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > > > I'm not sure this is the class you want to use. Do you specifically care
> > about
> > > > managed users here? Wouldn't SigningManagerBase be more appropriate?
> > > 
> > 
> > Is this something we want to enforce at the client though? This seems like a
> > strange detail to have here, and likely something that could be enforced at
> the
> > server.
>

[haitaol1, 2014-05-13 18:08:22]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
That's right. But we shouldn't start backup for managed users because that would
stop normal syncing. Normally if sync is on, SigninManagerBase returns non-empty
user name. But for managed user, it returns empty string (at least it used to
be, there was actually a DCHECK for that). Using SigninManagerBase, normal sync
could be trumped by backup.

On 2014/05/13 17:39:10, Nicolas Zea wrote:
> The command to roll back in the first place comes from the server, and should
> never be sent to a managed user, right?
> 
> On 2014/05/09 16:55:58, haitaol1 wrote:
> > How can server enforce this given that backup backend never contacts server?
> > 
> > On 2014/05/08 21:43:09, Nicolas Zea wrote:
> > > On 2014/05/07 21:34:17, haitaol1 wrote:
> > > > By using ManagedUserSigninManagerWrapper, backup/rollback is never
started
> > for
> > > > managed user because planpen uses a pseudo account always. This is
> probably
> > > the
> > > > safe thing to do.
> > > > 
> > > > On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > > > > I'm not sure this is the class you want to use. Do you specifically
care
> > > about
> > > > > managed users here? Wouldn't SigningManagerBase be more appropriate?
> > > > 
> > > 
> > > Is this something we want to enforce at the client though? This seems like
a
> > > strange detail to have here, and likely something that could be enforced
at
> > the
> > > server.
> > 
>

[Nicolas Zea, 2014-05-14 23:17:56]

LGTM

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
Ah, now I understand. Could you add a comment explaining this then?

On 2014/05/13 18:08:22, haitaol1 wrote:
> That's right. But we shouldn't start backup for managed users because that
would
> stop normal syncing. Normally if sync is on, SigninManagerBase returns
non-empty
> user name. But for managed user, it returns empty string (at least it used to
> be, there was actually a DCHECK for that). Using SigninManagerBase, normal
sync
> could be trumped by backup.
> 
> On 2014/05/13 17:39:10, Nicolas Zea wrote:
> > The command to roll back in the first place comes from the server, and
should
> > never be sent to a managed user, right?
> > 
> > On 2014/05/09 16:55:58, haitaol1 wrote:
> > > How can server enforce this given that backup backend never contacts
server?
> > > 
> > > On 2014/05/08 21:43:09, Nicolas Zea wrote:
> > > > On 2014/05/07 21:34:17, haitaol1 wrote:
> > > > > By using ManagedUserSigninManagerWrapper, backup/rollback is never
> started
> > > for
> > > > > managed user because planpen uses a pseudo account always. This is
> > probably
> > > > the
> > > > > safe thing to do.
> > > > > 
> > > > > On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > > > > > I'm not sure this is the class you want to use. Do you specifically
> care
> > > > about
> > > > > > managed users here? Wouldn't SigningManagerBase be more appropriate?
> > > > > 
> > > > 
> > > > Is this something we want to enforce at the client though? This seems
like
> a
> > > > strange detail to have here, and likely something that could be enforced
> at
> > > the
> > > > server.
> > > 
> > 
>

[haitaol1, 2014-05-14 23:49:26]

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
File chrome/browser/sync/backup_rollback_controller.h (right):

https://codereview.chromium.org/266233002/diff/1/chrome/browser/sync/backup_r...
chrome/browser/sync/backup_rollback_controller.h:25: const
ManagedUserSigninManagerWrapper* signin,
Commented on |signin_| in .h

On 2014/05/14 23:17:56, Nicolas Zea wrote:
> Ah, now I understand. Could you add a comment explaining this then?
> 
> On 2014/05/13 18:08:22, haitaol1 wrote:
> > That's right. But we shouldn't start backup for managed users because that
> would
> > stop normal syncing. Normally if sync is on, SigninManagerBase returns
> non-empty
> > user name. But for managed user, it returns empty string (at least it used
to
> > be, there was actually a DCHECK for that). Using SigninManagerBase, normal
> sync
> > could be trumped by backup.
> > 
> > On 2014/05/13 17:39:10, Nicolas Zea wrote:
> > > The command to roll back in the first place comes from the server, and
> should
> > > never be sent to a managed user, right?
> > > 
> > > On 2014/05/09 16:55:58, haitaol1 wrote:
> > > > How can server enforce this given that backup backend never contacts
> server?
> > > > 
> > > > On 2014/05/08 21:43:09, Nicolas Zea wrote:
> > > > > On 2014/05/07 21:34:17, haitaol1 wrote:
> > > > > > By using ManagedUserSigninManagerWrapper, backup/rollback is never
> > started
> > > > for
> > > > > > managed user because planpen uses a pseudo account always. This is
> > > probably
> > > > > the
> > > > > > safe thing to do.
> > > > > > 
> > > > > > On 2014/05/06 18:54:09, Nicolas Zea wrote:
> > > > > > > I'm not sure this is the class you want to use. Do you
specifically
> > care
> > > > > about
> > > > > > > managed users here? Wouldn't SigningManagerBase be more
appropriate?
> > > > > > 
> > > > > 
> > > > > Is this something we want to enforce at the client though? This seems
> like
> > a
> > > > > strange detail to have here, and likely something that could be
enforced
> > at
> > > > the
> > > > > server.
> > > > 
> > > 
> >

[haitaol1, 2014-05-14 23:50:43]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:53:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/266233002/30001

[commit-bot: I haz the power, 2014-05-15 02:20:37]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-05-15 02:33:44]

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

[commit-bot: I haz the power, 2014-05-15 02:33:45]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[haitaol1, 2014-05-15 16:30:19]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-15 16:33:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/266233002/50001

[commit-bot: I haz the power, 2014-05-15 19:26:19]

Change committed as 270773