Trim code from sync's ServerConnectionManager

Trim code form sync's ServerConnectionManager

- Remove 'authenticated' from SyncManager::Status.  This variable was
set to true iff the last attempt to contact the sync server resulted in
a 200 OK response.  There are lots of problems with this approach.
See also: crbug.com/117611.

The about:sync page also displays auth errors with
MakeSyncAuthErrorText(), which is much more likely to be accurate

- Remove server_up from SyncManager::Status.  It used to be set to true
iff the last server response was SERVER_CONNECTION_OK.  That doesn't
make a lot of sense and isn't very useful.

- Remove server_reachable from SyncManager::Status.  This was toggled
only when ServerConnectionManager::CheckServerReachable() failed, which
didn't happen very often.  CheckServerReachable() has since been
removed, so I think it now gets set only at startup and on the first
successful contact with the server.

- Remove server_reachable from ServerConnectionEvent and
ServerConnectionManager, for similar reasons.

- Remove the methods CheckTime(), IsServerReachable(),
IsUserAuthenticated(), and CheckServerReachable(),
SetServerParameters(), server_reachable() from ServerConnectionManager.
These methods were not used anywhere.  Also removed
IsGoodReplyFromServer(), whose last remaining callsite has been removed
in this commit.

- Remove the server_connection_ok_ flag from SyncScheduler.

- Hard-code AllStatus' online status to true.  See TODO comment.

- Remove OnCredentialsUpdated() and OnConnectionStatusChange() from
SyncScheduler.  Their functionality has been moved to the SyncManager.

- Rearrange some code in MockConnectionManager.  Apparently there used
to be such a thing as an "AUTHENTICATE" request, which we had unit tests
for.  That no longer exists, so I've rewritten some parts of this mock
class to better relfect ServerConnectionManager's current authentication
mechanisms.

- Update various unit tests in response to these changes.

- Update some integration tests to verify backoff behaviour when the
sync server is unreachable.  This replaces the old behaviour of
verifying that certain flags were set in the sync snapshot.

- Reduce the number of retries required to verify backoff.  This is to
help ensure that our integration tests fall within the 45s time limit.

- Disable notifications when disabling network connectivity in
integration tests.  This improves the reliability of exponential backoff
verification because the notifications would sometimes interfere with
backoff logic.


BUG=110954, 105739, 98346, 106262
TEST=


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

[rlarocque, 2012-02-08 01:57:29]

Here are the ServerConnectionManager changes I've been talking about.  This
replaces the 'preview' release at http://codereview.chromium.org/9251035/. 
Please review.

I will wait for http://codereview.chromium.org/9323015/ to land before
committing this.  Although this patch will overwrite most of it, I figure it
will be easier to merge that patch backwards if it can be landed on trunk first.

http://codereview.chromium.org/9348036/diff/18/chrome/browser/sync/internal_a...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9348036/diff/18/chrome/browser/sync/internal_a...
chrome/browser/sync/internal_api/sync_manager.cc:1154:
scheduler()->OnServerConnectionErrorFixed();
This bypasses the check of "connection_code_ == SYNC_AUTH_ERROR" in
SyncScheduler::OnCredentialsUpdated().  Although this looks dangerous, I think
it's perfectly safe.  The fact that UpdateCredentials() returns true only when
the auth token actually changed should ensure that we don't accidentally end up
in a loop pinging the server without exponential backoff.

http://codereview.chromium.org/9348036/diff/18/chrome/browser/sync/internal_a...
chrome/browser/sync/internal_api/sync_manager.cc:1723:
browser_sync::HttpResponse::CONNECTION_UNAVAILABLE)) {
I've moved the check of the most recent server_status to the
ServerConnectionManager.  I know that you and Tim were arguing against this
move, but I was hoping that seeing it in this context where it allows us to
remove lots of code might change your mind.  

Let me know what you think.  I can revert this change if necessary.

http://codereview.chromium.org/9348036/diff/18/chrome/browser/sync/sync_ui_ut...
File chrome/browser/sync/sync_ui_util.cc (left):

http://codereview.chromium.org/9348036/diff/18/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:294: } else if (!status.authenticated) {
Fred and I are reasonably convinced this is dead code.  Even if it wasn't dead,
I don't think it would make a lot of sense to keep it around.  I'm not sure what
it would actually do, since the traditional definition of status.authenticated
wasn't very reliable.

[rlarocque, 2012-02-10 23:16:06]

As discussed, I've undone some of the SyncScheduler changes.  PTAL.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:376: if
(!session_context_->connection_manager()->HasInvalidAuthToken())
You might be thinking that this condition is not the same as the old one.  You
would be right.  

We only reach this if statement when we're not in backoff.  If we were, we would
have hit the DecideWhileInWaitInterval() branch on line 355.  

So this if statement concerns the case where we're not in backoff, but
!server_connection_ok_.  That's a really strange state to be in.  I suppose that
could happen if a non-retryable job failed (ie. a POLL after issue 112954 is
fixed, CONFIGURE jobs).

I think the new logic will do a better job of detecting cases where we don't
want to even try to connect to the server.  Having no valid auth token is the
only scenario I can think of where it makes sense to not even try connecting to
the server, despite not being in backoff.  I'd prefer to err on the side of
CONTINUE rather than SAVE or DROP the job, since we'll usually up going into
backoff if it fails.

[lipalani1, 2012-02-11 00:12:10]

Hey I think we should fix up some of the values in all_status.cc rather than
removing it.

We just want to know the state of the syncer as of last sync. It is true they
might have changed by now. However when we look at a user with failing sync,
these values help us rule out network/auth as the issue for the last sync
failure.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/all_status.cc (left):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/all_status.cc:142: status_.server_reachable =
event.server_reachable;
Why was this removed? I think we just want the data as of the last completed
sync.

The usefulness comes when a user is unable to sync and we look at these values
and figure out it is not a connection issue. I think we should just fix this up.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/all_status.cc:145: status_.authenticated = true;
I think we should fix this up rather than removing. There are users out there
looking at this value. I understand the creds might have been made invalid
recently. But all we want to know is the state as of last sync.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/all_status.cc:104: const bool online = true;
I think we should update the sync's last sync success state. The success is
already being passed in snapshot.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (left):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:162: if
(conn_mgr_->server_status_ != response_->server_status) {
Is this still needed? Is this the auth notification you were talking about? I
thought it came from elsewhere.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (left):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:271: SDVLOG(2) << "Server
connection changed: " << transition;
There is some value to this log statement. You can still implement it based on
the connection code alone. if it is difficult(i dont think it is.) then it is
fine to remove it.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:376: if
(!session_context_->connection_manager()->HasInvalidAuthToken())
Yep it was designed to capture cases where this variable was updated outside of
sync. It is fine to sync and figure out whether we have the connection.
On 2012/02/10 23:16:06, rlarocque wrote:
> You might be thinking that this condition is not the same as the old one.  You
> would be right.  
> 
> We only reach this if statement when we're not in backoff.  If we were, we
would
> have hit the DecideWhileInWaitInterval() branch on line 355.  
> 
> So this if statement concerns the case where we're not in backoff, but
> !server_connection_ok_.  That's a really strange state to be in.  I suppose
that
> could happen if a non-retryable job failed (ie. a POLL after issue 112954 is
> fixed, CONFIGURE jobs).
> 
> I think the new logic will do a better job of detecting cases where we don't
> want to even try to connect to the server.  Having no valid auth token is the
> only scenario I can think of where it makes sense to not even try connecting
to
> the server, despite not being in backoff.  I'd prefer to err on the side of
> CONTINUE rather than SAVE or DROP the job, since we'll usually up going into
> backoff if it fails.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/sync_manager.cc (left):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:680: authenticated(false),
Not sure removing authenticated is a good idea. I think we should fix it up and
let it stand there as the value as per the last sync.

[rlarocque, 2012-02-11 01:30:29]

On 2012/02/11 00:12:10, lipalani1 wrote:
> Hey I think we should fix up some of the values in all_status.cc rather than
> removing it.
> 
> We just want to know the state of the syncer as of last sync. It is true they
> might have changed by now. However when we look at a user with failing sync,
> these values help us rule out network/auth as the issue for the last sync
> failure.
> 

I agree this is useful information, but I don't think the current set of status
indicators can ever be really fixed.  

Given the way they are calculated and displayed, declaring "authenticated",
"online" and "server_reachable" as bools will always cause problems.  There's no
proper way to indicate that the value hasn't been initialized, is unknown, or
N/A.  We could do it with the help of other bools, but that could add a lot of
complexity to the display logic (ie. don't display "server_reachable" until
"sync_ran_once && !sync_disabled" is true).  This is in addition to the problem
that these variables are named and displayed in a way that misrepresents how
much we actually know about the system.  

Instead, I propose that we should export the last_download_updates_result,
last_post_commit_result and last_process_commit_response variables to
about:sync.  These variables can be clearly labeled to indicate that they apply
to the most recent sync cycle.  They give us far more information about the
causes of the most recent failure.  They also support an "UNSET" value, so there
will be no confusion about what's going on during startup.  

I had planned to do this in a separate commit as part of a larger refactoring of
how we show per-session stats in about:sync, but I could squash the change into
this commit if you prefer.  

As for the status summary and the "online" bool, I'm not sure what to do with
it.  I don't think it's very useful as is.  At one point I had hoped I could
hook it up to the SyncScheduler's backoff state, but that seems unlikely now.

[rlarocque, 2012-02-11 01:31:36]

More comments.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (left):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:162: if
(conn_mgr_->server_status_ != response_->server_status) {
On 2012/02/11 00:12:10, lipalani1 wrote:
> Is this still needed? Is this the auth notification you were talking about? I
> thought it came from elsewhere.

This gets passed back to the SyncManager::OnServerConnectionEvent() which sends
the OnAuthError() notifications.

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (left):

http://codereview.chromium.org/9348036/diff/3001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:271: SDVLOG(2) << "Server
connection changed: " << transition;
On 2012/02/11 00:12:10, lipalani1 wrote:
> There is some value to this log statement. You can still implement it based on
> the connection code alone. if it is difficult(i dont think it is.) then it is
> fine to remove it.

This log statement exists specifically to tell us about status changes.  That
made sense when we actually did something in response to status changes, but we
no longer take any action based on this.  

The logs in the if statement above will tell us what the status is now.  I don't
see why we would need to add logs specifically for tracking cases where that
status changes.  If you *really* need to see the transitions, you can find them
by grepping the logs.

[lipalani1, 2012-02-22 18:28:50]

May be what we can do is, as a first step send the CL to export the last sync
status(like the commit result, GU result etc) and just print it as extra details
in the about:sync.

In the next CL then we get rid of these values and start using those values as
primary. 

That way it will not look like useful functionality is being removed.

http://codereview.chromium.org/9348036/diff/5012/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/9348036/diff/5012/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/all_status.cc:104: const bool online = true;
This does not look right?

Basically only the first if block would ever be executed(and you might get a
compiler warning as well) and the status can never be offline etc.

I think we should try to preserver this. I have some ideas in the message
section of this feedback.

[rlarocque, 2012-02-22 21:21:28]

On 2012/02/22 18:28:50, lipalani1 wrote:
> May be what we can do is, as a first step send the CL to export the last sync
> status(like the commit result, GU result etc) and just print it as extra
details
> in the about:sync.
> 
> In the next CL then we get rid of these values and start using those values as
> primary. 
> 
> That way it will not look like useful functionality is being removed.
> 

Sounds good.  I will prepare a CL.  

>
http://codereview.chromium.org/9348036/diff/5012/chrome/browser/sync/engine/a...
> File chrome/browser/sync/engine/all_status.cc (right):
> 
>
http://codereview.chromium.org/9348036/diff/5012/chrome/browser/sync/engine/a...
> chrome/browser/sync/engine/all_status.cc:104: const bool online = true;
> This does not look right?
> 
> Basically only the first if block would ever be executed(and you might get a
> compiler warning as well) and the status can never be offline etc.
> 
> I think we should try to preserver this. I have some ideas in the message
> section of this feedback.

That was mostly intentional.  I wanted to make it clear that this part of the
code is still in rough shape and we haven't really figured out an appropriate
way to report our online state yet.  The TODO comment above offers a bit of an
explanation.  In retrospect, this was not a good idea.

Thinking about it now, I wonder if we really need the summary?  Most of the time
the status will be READY or OFFLINE_UNUSABLE.  We have plenty of other
indicators on that page that can tell us whether or not sync is enabled, so that
information is redundant.  Now that the SYNCING state is no longer reachable in
practice, maybe there's no need for a summary.

At the very least, we should rethink what the set of possible states should be.

[rlarocque, 2012-02-29 18:51:22]

On 2012/02/22 21:21:28, rlarocque wrote:
> On 2012/02/22 18:28:50, lipalani1 wrote:
> > May be what we can do is, as a first step send the CL to export the last
sync
> > status(like the commit result, GU result etc) and just print it as extra
> details
> > in the about:sync.
> > 
> > In the next CL then we get rid of these values and start using those values
as
> > primary. 
> > 
> > That way it will not look like useful functionality is being removed.
> > 
> 
> Sounds good.  I will prepare a CL.  
> 

Separate CL has landed: http://codereview.chromium.org/9429064/

I'm still haven't figured out how to properly display a status summary.  The
rest of the code should be reviewable, though.

[lipalani1, 2012-03-09 19:57:53]

LGTM
2 changes:
Please refer the bug on Drew's plate in the CL description(and may be mention in
description that fixing that bug would take care of auth)

can you put a todo for status summary in code.

[rlarocque, 2012-03-12 21:39:44]

On 2012/03/09 19:57:53, lipalani1 wrote:
> LGTM
> 2 changes:
> Please refer the bug on Drew's plate in the CL description(and may be mention
in
> description that fixing that bug would take care of auth)
> 
> can you put a todo for status summary in code.

Uploaded some new patches to deal with integration tests.  Lingesh, since you're
familiar with the RetryVerifier, could you review these additional changes?

The changes are in patch sets 5-8.  Cleaner diffs available on request.  The
changes are as follows:

Integration tests that tested sync behaviour when the network was disabled used
to rely on certain flags that have been removed.  I have changed the logic used
in these tests so they verify exponential backoff behaviour, rather than looking
for a specific flag. This exposed some issues with the exponential backoff
verification.  

First, the verification would sometimes time out because the total time taken
for all retries was sometimes >30s.  This brought us dangerously close to the
45s time limit for integration tests.  I have reduced kMaxRetries to compensate.

Second, the DisableNetwork() call did not suppress XMPP notifications.  This
would trigger NOTIFICATION sync cycles, which counted as the 'had_nudge' job,
which confused the exponential backoff measurement logic.  I added code to
disable notifications when we call DisableNetwork() to avoid this problem.

[rlarocque, 2012-03-12 21:46:59]

> The changes are in patch sets 5-8.  

Correction: patch sets 5-7.

[lipalani1, 2012-03-14 23:23:21]

LGTM

[commit-bot: I haz the power, 2012-03-15 17:51:33]

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

[commit-bot: I haz the power, 2012-03-15 19:41:43]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android, revision is 126942, job name
was 9348036-29001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-03-15 20:50:53]

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

[commit-bot: I haz the power, 2012-03-16 01:22:37]

Try job failure for 9348036-29001 (retry) (previous was lost) on win_rel for
step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-16 22:09:41]

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

[commit-bot: I haz the power, 2012-03-17 00:19:07]

Change committed as 127310