Fix unwanted sign in to Chrome when the user signs in to another service.

Fix unwanted sign in to Chrome when the user signs in to another service.

Unwanted sign in to Chrome occurred when the user signed in to another service (Gmail, crbug.com...) from a tab that previously showed the Chrome sign in flow.

This CL clears the state of the OneClickSigninHelper associated with a web contents when it's detected that the sign in process has been aborted (i.e. a navigation to a page which is not part of the web-based sign in is triggered by something else than the web contents). It's necessary to maintain the state when a navigation to an unknown page is triggered from the web contents in order to support SAML.

TEST=
1.Launch Chrome and create new user ('Welcome to Chrome' page will appear).
2.Type 'crbug.com' in omnibox and hit 'Enter' key. ('Omnibox' of the same tab that showed the Chrome sign in page).
3.Click on the 'Sign in' link of 'crbug.com'
4.Login with valid account credentials.
Expected result: You are not signed in to Chrome and no message related to sync appears.
BUG=269421
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216814

[fdoray, 2013-08-07 21:00:09]

Could you review this CL?
Thanks.

[bcwhite, 2013-08-08 13:07:29]

lgtm

[noms, 2013-08-08 13:45:18]

lgtm

[Roger Tawa OOO till Jul 10th, 2013-08-08 17:17:29]

lgtm++

[commit-bot: I haz the power, 2013-08-08 19:44:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/22253010/2001

[commit-bot: I haz the power, 2013-08-08 21:26:48]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[fdoray, 2013-08-09 14:32:24]

Roger: I had to make small changes in
chrome/browser/signin/signin_browsertest.cc and
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc because some tests
failed. Do you want to look at these changes before I commit the CL?

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1070: auto_accept_ !=
AUTO_ACCEPT_NONE)
CleanTransientState() cannot be executed in a unit test because it posts a task
to the IO thread. I added "auto_accept_ != AUTO_ACCEPT_NONE" so that it's not
executed when it's not necessary.

[(NOT FOR CODE REVIEWS), 2013-08-09 14:43:04]

Thanks for the heads up.  A couple of comments below.

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1070: auto_accept_ !=
AUTO_ACCEPT_NONE)
On 2013/08/09 14:32:25, fdoray wrote:
> CleanTransientState() cannot be executed in a unit test because it posts a
task
> to the IO thread. I added "auto_accept_ != AUTO_ACCEPT_NONE" so that it's not
> executed when it's not necessary.

Is it only for tests?  Can you set do_not_clear_pending_email_ to true?

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1071: CleanTransientState();
Should add { and } to this if statement since condition is across two lines.

[fdoray, 2013-08-09 17:42:14]

Roger, can you review this again?

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1070: auto_accept_ !=
AUTO_ACCEPT_NONE)
It's for tests, but it also avoids unnecessary calls to CleanTransientState().

It would be hard to set do_not_clear_pending_email_ for every
OneClickSigninHelper created in a BrowserWithTestWindowTest (see unit_tests 
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/157212).

I don't think that the variables cleaned by CleanTransientState() can have a
value when auto_accept_ == AUTO_ACCEPT_NONE. Am I right? If yes, can I move the
if (auto_accept_ != AUTO_ACCEPT_NONE) condition in CleanTransientState()?

On 2013/08/09 14:43:04, Roger Tawa (Google) wrote:
> On 2013/08/09 14:32:25, fdoray wrote:
> > CleanTransientState() cannot be executed in a unit test because it posts a
> task
> > to the IO thread. I added "auto_accept_ != AUTO_ACCEPT_NONE" so that it's
not
> > executed when it's not necessary.
> 
> Is it only for tests?  Can you set do_not_clear_pending_email_ to true?

https://codereview.chromium.org/22253010/diff/47001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1071: CleanTransientState();
On 2013/08/09 14:43:04, Roger Tawa (Google) wrote:
> Should add { and } to this if statement since condition is across two lines.

Done.

https://codereview.chromium.org/22253010/diff/82002/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/22253010/diff/82002/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1204: if (showing_signin_)
Is it possible to reach this line? |email_| would need to have a non-empty
value.

[commit-bot: I haz the power, 2013-08-09 18:32:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/22253010/82002

[commit-bot: I haz the power, 2013-08-09 22:19:22]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 01:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/22253010/82002

[commit-bot: I haz the power, 2013-08-10 07:17:48]

Change committed as 216814