Abort sync cycles when download step fails

Abort sync cycles when download step fails

This change causes the syncer to exit from its download then commit
function if the download fails.  This helps prevent the creation of
server-side conflicts, which would be more likely to happen if a
download failed but the following commit succeeded.

The main changes are as follows:
- Rather than proceed to the next step when a download updates failure
  is detected, the syncer will exit.  This should cause the
SyncScheduler to schedule a retry at a later time.  
- The definition of a download updates failure is now based on the
  return code of the download attempt, rather than checking the contents
of the (possible non-existent) returned protobuf.  This makes the error
detection logic used here more closely match the logic used to decide
whether or not to schedule retries.

This implementation has a side-effect on configure sync cycles.  The old
behaviour was to handle failures by applying whatever updates we had
downloaded at that point.  The new behaviour will leave updates
unapplied if any error occurs.  This better matches a nearby comment's
assertion which states that we should not attempt to apply updates until
we have downloaded all of them.  I believe the author of that comment
would approve of this change.

This change moves around some of the ExtensionActivityMonitor logic.
It's important that we not take the data from the extensions acitivity
monitor at the start of the cycle.  Restoring that data is handled in
the commit building and response code, which might not be executed if we
need to break out early.  This also fixes issue 123270.

Most of the diffs for this change are concentrated in the unit tests:
- Expose more of the SyncScheduler to the SyncerTest test harness.
- Add functions to SyncerTest for testing specific types of sync jobs.
- Add some functions that allow us to better control failures in
MockConnectionManager.
- Added tests for configure job success and failure.
- Added tests for update then commit job success and failure.
- Removed some redundant tests.

BUG=122033, 123270
TEST=sync_unit_tests, specifically:
SyncerTest.UpdateThenCommit,
SyncerTest.UpdateFailsThenDontCommit,
SyncerTest.ConfigureDownloadsTwoBatchesSuccess,
SyncerTest.ConfigureFailsDontApplyUpdates

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132455

[rlarocque, 2012-04-06 20:35:09]

Nicolas: More than anything else, it's the interactions between these changes
and encryption that scare me.  Please review the non-test changes.

Tim: I ended up modifying some very old unit test code when adding the unit
tests.  Please review those changes.

http://codereview.chromium.org/10006046/diff/2001/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10006046/diff/2001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.cc:686: void SyncScheduler::SetSyncerStepsForPurpose(
This function has moved, but is otherwise mostly unmodified.  

The one change is that it no longer DCHECKs based on which thread we're on.  I
think that's acceptable, considering that this function is entirely thread-safe.

http://codereview.chromium.org/10006046/diff/2001/sync/sessions/sync_session_...
File sync/sessions/sync_session_unittest.cc (left):

http://codereview.chromium.org/10006046/diff/2001/sync/sessions/sync_session_...
sync/sessions/sync_session_unittest.cc:245: // When the server returns a
timestamp, that means we're up to date.
I think this comment is stale.

http://codereview.chromium.org/10006046/diff/2001/sync/sessions/sync_session_...
sync/sessions/sync_session_unittest.cc:256: TEST_F(SyncSessionTest,
MoreToDownloadIfGotNoChangesRemainingForSubset) {
The following tests contain stale comments and are almost line-by-line identical
to other existing tests.  They require maintenance because of my changes to the
definition of download_updates_succeeded().  I think the best course of action
is to delete them and remove the maintenance burden.

http://codereview.chromium.org/10006046/diff/2001/sync/test/engine/mock_conne...
File sync/test/engine/mock_connection_manager.h (left):

http://codereview.chromium.org/10006046/diff/2001/sync/test/engine/mock_conne...
sync/test/engine/mock_connection_manager.h:120: void
FailNextPostBufferToPathCall() { fail_next_postbuffer_ = true; }
At this point, it probably makes sense to start thinking about using gmock here.
 However, I'd rather just get my current CL done for now and we can revisit the
entire class in a later CL.

[tim (not reviewing), 2012-04-06 20:38:57]

http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:174: last_step = SYNCER_END; // Necessary for
CONFIGURATION mode.
If we're going straight to END, why not call RequestEarlyExit()? That function
is meant to provide support for bailing out of a sync session at any time.

[tim (not reviewing), 2012-04-06 20:48:21]

http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:174: last_step = SYNCER_END; // Necessary for
CONFIGURATION mode.
On 2012/04/06 20:38:57, timsteele wrote:
> If we're going straight to END, why not call RequestEarlyExit()? That function
> is meant to provide support for bailing out of a sync session at any time.
> 

Oh, I guess you *want* to run END. 

I think the comment about "CONFIGURATION" mode confused me a bit. Is the more
general comment "Necessary because some components may rely on knowing when a
sync cycle that started has ended"?

I know we've went back and forth with just sending the notification
unconditionally after the cycle loop, I forget why at the moment...

[rlarocque, 2012-04-06 20:59:31]

On 2012/04/06 20:48:21, timsteele wrote:
> http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc
> File sync/engine/syncer.cc (right):
> 
>
http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc#newco...
> sync/engine/syncer.cc:174: last_step = SYNCER_END; // Necessary for
> CONFIGURATION mode.
> On 2012/04/06 20:38:57, timsteele wrote:
> > If we're going straight to END, why not call RequestEarlyExit()? That
function
> > is meant to provide support for bailing out of a sync session at any time.
> > 
> 
> Oh, I guess you *want* to run END. 
> 
> I think the comment about "CONFIGURATION" mode confused me a bit. Is the more
> general comment "Necessary because some components may rely on knowing when a
> sync cycle that started has ended"?
> 
> I know we've went back and forth with just sending the notification
> unconditionally after the cycle loop, I forget why at the moment...

SetSyncerStepsForPurpose() should explain all.  There are some purposes for
which we don't send notifications; I presume that's intentional.  

The configuration mode is not among that list.  Even though the steps assigned
in by that function seem to indicate it won't end at the SYCNER_END step,
there's an ugly if statement in APPLY_UPDATES to work around this.

[Nicolas Zea, 2012-04-09 19:09:14]

Two nits, else non-tests LGTM

Also, the only encryption-related work that can happen before ApplyUpdates is in
ProcessUpdates, where we set the base_server_specifics as necessary. That logic
is resilient to a future update happening before we apply, so should not be
affected by this.

Similarly, any update that doesn't get applied will prevent that item from
committing (including the nigori node), so we should be fine w.r.t. pushing
changes to the server.

The issue that comes to mind would be if we receive a nigori node update, but
don't apply it, while at the same time we make encryption changes. Because we
didn't apply the nigori node update, we won't know if we have pending keys, and
hence might upload _other_ items that have been re-encrypted (although we
wouldn't upload the nigori node).

That said, that issue exists today, and we rely on the nigori node conflict
resolution to handle it (which it would still do in this case). So we should be
fine.

http://codereview.chromium.org/10006046/diff/2001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10006046/diff/2001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:255: SyncerStep* start, SyncerStep* end);
have params on separate lines for consistency with rest of file

http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10006046/diff/2001/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:178: .ServerSaysNothingMoreToDownload()) {
nit: indent by four more spaces

[tim (not reviewing), 2012-04-12 01:28:20]

http://codereview.chromium.org/10006046/diff/2001/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10006046/diff/2001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.cc:686: void SyncScheduler::SetSyncerStepsForPurpose(
On 2012/04/06 20:35:09, rlarocque wrote:
> This function has moved, but is otherwise mostly unmodified.  
> 
> The one change is that it no longer DCHECKs based on which thread we're on.  I
> think that's acceptable, considering that this function is entirely
thread-safe.

nit - add "// static" above "void SyncScheduler::SetSyncerStepsForPurpose"

I think the DCHECK was more of a self-commenting line of code telling developers
something about how the function is used, but nevertheless I agree it isn't
critical.

[rlarocque, 2012-04-12 02:32:05]

Patch updated.

[rlarocque, 2012-04-12 21:34:31]

http://codereview.chromium.org/10006046/diff/12001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10006046/diff/12001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:122:
session->context()->extensions_monitor()->GetAndClearRecords(
While working on a different CL, I realized that this CL breaks this line.  We
should put the data back into the extensions monitor if we don't commit.

[rlarocque, 2012-04-13 18:42:29]

Updated with fix for issue 123270 and prevention of a related bug we almost
created.

[tim (not reviewing), 2012-04-13 18:47:27]

On 2012/04/13 18:42:29, rlarocque wrote:
> Updated with fix for issue 123270 and prevention of a related bug we almost
> created.

LGTM

[commit-bot: I haz the power, 2012-04-13 21:21:28]

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

[commit-bot: I haz the power, 2012-04-13 21:54:22]

Try job failure for 10006046-18001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-16 18:44:47]

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

[commit-bot: I haz the power, 2012-04-16 21:22:07]

Change committed as 132455