Provide correct guard for GCM connection tracking of login failures.

Provide correct guard for GCM connection tracking of login failures.

The original implementation of GCM Connection tracking had a DCHECK in
ConnectionLoginFailed to validate that ConnectionAttemptSucceeded had
been called first to validate the expected ordering of the calls.

That was removed as a result of crbug.com/673706 because it was failing.

After investigating, the reason for the failures is that the entry check
for calling ConnectionLoginFailed was checking the logging_in_ member on
the ConnectionFactoryImpl. However, that member tracks whether the
initial connection is established, not whether the login handshake has
been completed.

The correct check for when to call ConnectionLoginFailed is if the
result passed to SignalConnectionReset is LOGIN_FAILURE, since that
indicates that the MCSClient was unable to complete the login.

BUG=673706
Review-Url: https://codereview.chromium.org/2624653003
Cr-Commit-Position: refs/heads/master@{#443531}
Committed: https://chromium.googlesource.com/chromium/src/+/2d56c660fa73bbc7a8bb65839e473e326c7021d7

[harkness, 2017-01-10 16:34:05]

harkness@chromium.org changed reviewers:
+ peter@chromium.org, zea@chromium.org

[Peter Beverloo, 2017-01-11 13:46:41]

lgtm, but please consider renaming |logging_in_| to something more appropriate
if it's about the handshake prior to login.

[harkness, 2017-01-12 10:26:30]

Updated. 

I renamed logging_in_ to handshake_in_progress_, which is wordier, but also
seems to more accurately describe the phase when the flag is true.

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:171: return "LOGGING IN";
I couldn't find any places that might be relying on this text. @zea, do you know
of any? Or should I just leave it as LOGGING_IN?

[Nicolas Zea, 2017-01-12 18:00:57]

lgtm

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:171: return "LOGGING IN";
On 2017/01/12 10:26:30, harkness wrote:
> I couldn't find any places that might be relying on this text. @zea, do you
know
> of any? Or should I just leave it as LOGGING_IN?

I'd say go ahead and update for consistency sake . This is used in
chrome://gcm-internals

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.h:186: // Whether handshake
successfully completed after the connection was
nit: given this is "in progress", perhaps just reword this to be "Whether
handshake is in progress after the connection..."

[harkness, 2017-01-13 08:28:08]

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:171: return "LOGGING IN";
On 2017/01/12 18:00:57, Nicolas Zea wrote:
> On 2017/01/12 10:26:30, harkness wrote:
> > I couldn't find any places that might be relying on this text. @zea, do you
> know
> > of any? Or should I just leave it as LOGGING_IN?
> 
> I'd say go ahead and update for consistency sake . This is used in
> chrome://gcm-internals

Done.

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/2624653003/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.h:186: // Whether handshake
successfully completed after the connection was
On 2017/01/12 18:00:57, Nicolas Zea wrote:
> nit: given this is "in progress", perhaps just reword this to be "Whether
> handshake is in progress after the connection..."

Done.

[harkness, 2017-01-13 08:28:15]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-01-13 08:28:15]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2624653003/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2017-01-13 08:28:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 09:56:45]

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

[commit-bot: I haz the power, 2017-01-13 09:56:46]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[harkness, 2017-01-13 10:36:43]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2017-01-13 10:36:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 11:26:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484303803962060,
"parent_rev": "cc480a0a050fb3c067675fb302379ffb139c6e50", "commit_rev":
"2d56c660fa73bbc7a8bb65839e473e326c7021d7"}

[commit-bot: I haz the power, 2017-01-13 11:26:36]

Description was changed from

==========
Provide correct guard for GCM connection tracking of login failures.

The original implementation of GCM Connection tracking had a DCHECK in
ConnectionLoginFailed to validate that ConnectionAttemptSucceeded had
been called first to validate the expected ordering of the calls.

That was removed as a result of crbug.com/673706 because it was failing.

After investigating, the reason for the failures is that the entry check
for calling ConnectionLoginFailed was checking the logging_in_ member on
the ConnectionFactoryImpl. However, that member tracks whether the
initial connection is established, not whether the login handshake has
been completed.

The correct check for when to call ConnectionLoginFailed is if the
result passed to SignalConnectionReset is LOGIN_FAILURE, since that
indicates that the MCSClient was unable to complete the login.

BUG=673706
==========

to

==========
Provide correct guard for GCM connection tracking of login failures.

The original implementation of GCM Connection tracking had a DCHECK in
ConnectionLoginFailed to validate that ConnectionAttemptSucceeded had
been called first to validate the expected ordering of the calls.

That was removed as a result of crbug.com/673706 because it was failing.

After investigating, the reason for the failures is that the entry check
for calling ConnectionLoginFailed was checking the logging_in_ member on
the ConnectionFactoryImpl. However, that member tracks whether the
initial connection is established, not whether the login handshake has
been completed.

The correct check for when to call ConnectionLoginFailed is if the
result passed to SignalConnectionReset is LOGIN_FAILURE, since that
indicates that the MCSClient was unable to complete the login.

BUG=673706

Review-Url: https://codereview.chromium.org/2624653003
Cr-Commit-Position: refs/heads/master@{#443531}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d56c660fa73bbc7a8bb65839e47...
==========

[commit-bot: I haz the power, 2017-01-13 11:26:37]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2d56c660fa73bbc7a8bb65839e47...