[signin] Move Mirror code to ChromeConnectedHeaderHelper

[signin] Move Mirror code to ChromeConnectedHeaderHelper

This is a refactoring/cleanup, to prepare for DICE response handling.

BUG=730589
Review-Url: https://codereview.chromium.org/2925083002
Cr-Commit-Position: refs/heads/master@{#478664}
Committed: https://chromium.googlesource.com/chromium/src/+/2d5c7b158fb963fea6f63a568a610ddf43f72426

[droger, 2017-06-07 13:08:02]

Patchset #2 (id:20001) has been deleted

[droger, 2017-06-07 13:08:08]

Patchset #1 (id:1) has been deleted

[droger, 2017-06-07 13:08:14]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 13:08:20]

Dry run: 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-06-07 13:40:45]

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

[commit-bot: I haz the power, 2017-06-07 13:40:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[droger, 2017-06-08 14:54:02]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 14:54:14]

Dry run: CQ is trying da patch.

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

[droger, 2017-06-08 15:01:10]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 15:01:21]

Dry run: 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-06-08 15:11:21]

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

[commit-bot: I haz the power, 2017-06-08 15:11:22]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[droger, 2017-06-08 16:16:52]

Description was changed from

==========
[signin] Refactor handling of account consistency response headers

This refactoring prepares for the support of "X-Chrome-ID-Consistency-Response".
No behavior change.

BUG=730589
==========

to

==========
[signin] Move Mirror code to ChromeConnectedHeaderHelper

This is a refactoring/cleanup, to prepare for DICE response handling.

BUG=730589
==========

[droger, 2017-06-08 16:17:08]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 16:17:21]

Dry run: 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-06-08 18:07:31]

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

[commit-bot: I haz the power, 2017-06-08 18:07:32]

Dry run: This issue passed the CQ dry run.

[droger, 2017-06-09 12:00:03]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 12:00:09]

Dry run: CQ is trying da patch.

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

[droger, 2017-06-09 12:04:38]

Patchset #8 (id:170001) has been deleted

[droger, 2017-06-09 12:04:47]

Patchset #3 (id:70001) has been deleted

[droger, 2017-06-09 12:04:56]

Patchset #1 (id:40001) has been deleted

[droger, 2017-06-09 12:05:10]

Patchset #2 (id:90001) has been deleted

[droger, 2017-06-09 12:05:16]

Patchset #1 (id:50001) has been deleted

[droger, 2017-06-09 12:05:24]

Patchset #1 (id:110001) has been deleted

[droger, 2017-06-09 12:05:31]

Patchset #1 (id:130001) has been deleted

[droger, 2017-06-09 12:05:39]

Patchset #1 (id:150001) has been deleted

[droger, 2017-06-09 12:06:54]

droger@chromium.org changed reviewers:
+ msarda@chromium.org

[droger, 2017-06-09 12:06:55]

https://codereview.chromium.org/2925083002/diff/210001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/210001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:50:
switches::AccountConsistencyMethod::kMirror) {
Note: this was a DCHECK but I think a if is more correct, because it could
happen as a result of a sever bug, which is not under our control.

https://codereview.chromium.org/2925083002/diff/210001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:152: if
(io_data->IsOffTheRecord())
Note: this was a DCHECK but I think a if is more correct, because it could
happen as a result of a sever bug, which is not under our control.

[droger, 2017-06-09 14:34:40]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 14:34:52]

Dry run: 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-06-09 15:49:25]

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

[commit-bot: I haz the power, 2017-06-09 15:49:26]

Dry run: This issue passed the CQ dry run.

[droger, 2017-06-12 11:40:08]

Patchset #3 (id:220001) has been deleted

[droger, 2017-06-12 11:40:16]

Patchset #1 (id:190001) has been deleted

[msarda, 2017-06-12 12:21:14]

https://codereview.chromium.org/2925083002/diff/210001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/210001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:50:
switches::AccountConsistencyMethod::kMirror) {
On 2017/06/09 12:06:55, droger wrote:
> Note: this was a DCHECK but I think a if is more correct, because it could
> happen as a result of a sever bug, which is not under our control.

I'm afraid I do not understand this comment - I do not know what DCHECK you are
mentioning here.

https://codereview.chromium.org/2925083002/diff/210001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:152: if
(io_data->IsOffTheRecord())
On 2017/06/09 12:06:55, droger wrote:
> Note: this was a DCHECK but I think a if is more correct, because it could
> happen as a result of a sever bug, which is not under our control.

Same here - what DCHECK are you referring to?

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:49: if
(switches::GetAccountConsistencyMethod() !=
I think we should not add this if - Gaia is not supposed to send an Manage
Accounts header when Mirror is not enabled.

[droger, 2017-06-12 12:46:38]

https://codereview.chromium.org/2925083002/diff/210001/components/signin/core...
File components/signin/core/browser/signin_header_helper.cc (left):

https://codereview.chromium.org/2925083002/diff/210001/components/signin/core...
components/signin/core/browser/signin_header_helper.cc:227:
DCHECK(switches::IsAccountConsistencyMirrorEnabled() && !is_off_the_record);
This is the DCHECK I am mentioning in the other comments.

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:49: if
(switches::GetAccountConsistencyMethod() !=
On 2017/06/12 12:21:14, msarda wrote:
> I think we should not add this if - Gaia is not supposed to send an Manage
> Accounts header when Mirror is not enabled.

I agree that Gaia should not do it.

The question is: should a Gaia bug be able to crash Chrome? I changed it from a
DCHECK because I think we have to handle it gracefully, even if it is not
supposed to happen. Because this is not something that depends on Chrome code
and state, it depends on server input.

[msarda, 2017-06-12 14:08:57]

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:49: if
(switches::GetAccountConsistencyMethod() !=
On 2017/06/12 12:46:38, droger wrote:
> On 2017/06/12 12:21:14, msarda wrote:
> > I think we should not add this if - Gaia is not supposed to send an Manage
> > Accounts header when Mirror is not enabled.
> 
> I agree that Gaia should not do it.
> 
> The question is: should a Gaia bug be able to crash Chrome? I changed it from
a
> DCHECK because I think we have to handle it gracefully, even if it is not
> supposed to happen. Because this is not something that depends on Chrome code
> and state, it depends on server input.

The DCHECK also included the fact that it does not work in incognito. I think it
would make sense to the the following:
* on ProcessMirrorResponseHeaderIfExists, we should add an if
(io_data->IsOffTheRecord() && switches::GetAccountConsistencyMethod() !=
switches::AccountConsistencyMethod::kMirror) (I suppose we can use the switches
on the IO thread, but I may be wrong).
* in ProcessMirrorHeaderUIThread, we should probably have a DCHECK(!incognito);
DCHECK_EQ(switches::AccountConsistencyMethod::kMirror,
switches::GetAccountConsistencyMethod())

https://codereview.chromium.org/2925083002/diff/240001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:152: if
(io_data->IsOffTheRecord())
I think it would somehow make sense to have a NOTREACHED here as this is a
brreakage in the Chrome / Gaia protocol and if this starts to happen then there
is definitely a problem in the product that we need to fix. Or should this be
covered by an integration test?

[droger, 2017-06-12 14:58:34]

Thanks.

As per offline discussion:
- implement both a if() and a NOTREACHED() so that Chrome cannot crash on a bad
server input, but we still can catch it in development.
- keep accessing the command line from the IO thread for now, but file a bug to
address this (https://crbug.com/732406)

[droger, 2017-06-12 14:58:45]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 14:59:00]

Dry run: CQ is trying da patch.

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

[msarda, 2017-06-12 15:05:32]

lgtm

https://codereview.chromium.org/2925083002/diff/260001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/260001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:168: NOTREACHED() << "Gaia should
not send the Chrome-Connected header in "
s/Chrome-Connected/X-Chrome-Manage-Accounts

https://codereview.chromium.org/2925083002/diff/260001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:175: NOTREACHED() << "Gaia should
not send the Chrome-Connected header when "
s/Chrome-Connected/X-Chrome-Manage-Accounts

[droger, 2017-06-12 15:57:12]

https://codereview.chromium.org/2925083002/diff/260001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_helper.cc (right):

https://codereview.chromium.org/2925083002/diff/260001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_helper.cc:168: NOTREACHED() << "Gaia should
not send the Chrome-Connected header in "
On 2017/06/12 15:05:32, msarda wrote:
> s/Chrome-Connected/X-Chrome-Manage-Accounts

Good catch!

[droger, 2017-06-12 15:58:02]

The CQ bit was checked by droger@chromium.org

[droger, 2017-06-12 15:58:03]

The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2925083002/#ps280001
(title: "fix comment")

[commit-bot: I haz the power, 2017-06-12 15:58: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-06-12 16:59:37]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1497283082641500,
"parent_rev": "84a45d44559e3cb97b13b7534621ed5aebef784e", "commit_rev":
"2d5c7b158fb963fea6f63a568a610ddf43f72426"}

[commit-bot: I haz the power, 2017-06-12 16:59:49]

Description was changed from

==========
[signin] Move Mirror code to ChromeConnectedHeaderHelper

This is a refactoring/cleanup, to prepare for DICE response handling.

BUG=730589
==========

to

==========
[signin] Move Mirror code to ChromeConnectedHeaderHelper

This is a refactoring/cleanup, to prepare for DICE response handling.

BUG=730589

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

[commit-bot: I haz the power, 2017-06-12 16:59:50]

Committed patchset #4 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/2d5c7b158fb963fea6f63a568a61...