Reland cros: Test for overlapping ContinueSessionRestore calls

Reland cros: Test for overlapping ContinueSessionRestore calls

Original review: https://codereview.chromium.org/2658953002/

OAuth2LoginManager::ContinueSessionRestore with strategy
RESTORE_FROM_SAVED_OAUTH2_REFRESH_TOKEN would call
TokenService::LoadCredentials that does not support
multiple concurrent calls. Add a test to verify that it
does not attempt to overlap LoadCredentials calls.

BUG=682695
TEST=OAuth2Test.OverlappingContinueSessionRestore

Review-Url: https://codereview.chromium.org/2668043002
Cr-Commit-Position: refs/heads/master@{#448392}
Committed: https://chromium.googlesource.com/chromium/src/+/5a99a823e8dcc16c9804df03ba30747255104c2b

[xiyuan, 2017-02-01 21:25:11]

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

[commit-bot: I haz the power, 2017-02-01 21:25:38]

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

[xiyuan, 2017-02-01 21:30:48]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org, derat@chromium.org, rogerta@chromium.org

[xiyuan, 2017-02-01 21:30:49]

derat@:
  Reland of the test. Changing supporting code to fix
  the flakiness.

achuith@:
  c/b/chromeos/login/test/oobe_base_test.cc
  Fix WaitForGaiaPageReload timing by creating
  DOMMessageQueue before executing JS and changing JS
  to also check 'readyFired_' before listening to it.

rogerta@:
  google_apis/gaia/fake_gaia.*
  Add a /GetCheckConnectionInfo so that /MergeSession
  is invoked earlier. Otherwise, GaiaCookieManagerService
  would try 3 times with some back-off timer before kick
  start /MergeSession. This could result in test time out.

Thanks.

[Daniel Erat, 2017-02-01 22:15:42]

lgtm

[commit-bot: I haz the power, 2017-02-01 22:45:20]

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

[commit-bot: I haz the power, 2017-02-01 22:45:21]

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

[xiyuan, 2017-02-01 23:15:13]

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

[commit-bot: I haz the power, 2017-02-01 23:15:30]

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-02-02 00:15:31]

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

[commit-bot: I haz the power, 2017-02-02 00:15:31]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-02-02 00:37:11]

lgtm

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_browsertest.cc (right):

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:147: // Blocks a
BrowserThread on construction and unblocks it on destruction.
This seems like a generally useful class - I'm surprised there's not a utility
somewhere that does this.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:158:
~BrowserThreadBlocker() { unblock_event_->Signal(); }
Wonder if it's safer to use an explicit Release method instead of doing work in
the dtor?

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:164: // Owned by the
BlockThreadOnThread callback.
Maybe it's more accurate to say that this shared semaphore is deleted by the
callback?

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:166:
DISALLOW_COPY_AND_ASSIGN(BrowserThreadBlocker);
I'd add a newline before this line

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:544: // Tests that
ContinueSessionRestore could be called multiple times.
Impressive test.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (right):

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.cc:231:
content::DOMMessageQueue message_queue;
A comment might be useful in case someone decides to move this below closer to
where it's used in a later refactoring.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.cc:236: "  var f = function()
{"
This doesn't make a functional difference, right?

[xiyuan, 2017-02-02 18:44:00]

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_browsertest.cc (right):

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:147: // Blocks a
BrowserThread on construction and unblocks it on destruction.
On 2017/02/02 00:37:10, achuithb wrote:
> This seems like a generally useful class - I'm surprised there's not a utility
> somewhere that does this.

I am surprised that I could not find similar class in test util code either.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:158:
~BrowserThreadBlocker() { unblock_event_->Signal(); }
On 2017/02/02 00:37:10, achuithb wrote:
> Wonder if it's safer to use an explicit Release method instead of doing work
in
> the dtor?

Dtor should be safe as long as we don't have derived class etc. Doing it  in
dtor makes it work like a scoped object. IMHO, it is easier to use this way.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:164: // Owned by the
BlockThreadOnThread callback.
On 2017/02/02 00:37:10, achuithb wrote:
> Maybe it's more accurate to say that this shared semaphore is deleted by the
> callback?

Updated the comment to:
  // |unblock_event_| is deleted after BlockThreadOnThread returns.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:166:
DISALLOW_COPY_AND_ASSIGN(BrowserThreadBlocker);
On 2017/02/02 00:37:10, achuithb wrote:
> I'd add a newline before this line

Done.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (right):

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.cc:231:
content::DOMMessageQueue message_queue;
On 2017/02/02 00:37:11, achuithb wrote:
> A comment might be useful in case someone decides to move this below closer to
> where it's used in a later refactoring.

Done.

https://codereview.chromium.org/2668043002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.cc:236: "  var f = function()
{"
On 2017/02/02 00:37:11, achuithb wrote:
> This doesn't make a functional difference, right?

No functional change in PS4. PS3 was too aggressive and breaks a few tests that
needs more time to trigger a Gaia reload.

[xiyuan, 2017-02-02 18:44:03]

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

[commit-bot: I haz the power, 2017-02-02 18:44:34]

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

[Roger Tawa OOO till Jul 10th, 2017-02-02 18:48:21]

lgtm google_apis

[commit-bot: I haz the power, 2017-02-02 20:23:03]

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

[commit-bot: I haz the power, 2017-02-02 20:23:04]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-02-06 20:16:13]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-02-06 20:16:14]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2668043002/#ps80001
(title: "for comments in #4")

[commit-bot: I haz the power, 2017-02-06 20:16:54]

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-02-06 21:22:43]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486412173718490,
"parent_rev": "ea4becd40cfc07b0bfa453ea6413dd9f31c0af93", "commit_rev":
"5a99a823e8dcc16c9804df03ba30747255104c2b"}

[commit-bot: I haz the power, 2017-02-06 21:23:41]

Description was changed from

==========
Reland cros: Test for overlapping ContinueSessionRestore calls

Original review: https://codereview.chromium.org/2658953002/

OAuth2LoginManager::ContinueSessionRestore with strategy
RESTORE_FROM_SAVED_OAUTH2_REFRESH_TOKEN would call
TokenService::LoadCredentials that does not support
multiple concurrent calls. Add a test to verify that it
does not attempt to overlap LoadCredentials calls.

BUG=682695
TEST=OAuth2Test.OverlappingContinueSessionRestore
==========

to

==========
Reland cros: Test for overlapping ContinueSessionRestore calls

Original review: https://codereview.chromium.org/2658953002/

OAuth2LoginManager::ContinueSessionRestore with strategy
RESTORE_FROM_SAVED_OAUTH2_REFRESH_TOKEN would call
TokenService::LoadCredentials that does not support
multiple concurrent calls. Add a test to verify that it
does not attempt to overlap LoadCredentials calls.

BUG=682695
TEST=OAuth2Test.OverlappingContinueSessionRestore

Review-Url: https://codereview.chromium.org/2668043002
Cr-Commit-Position: refs/heads/master@{#448392}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a99a823e8dcc16c9804df03ba30...
==========

[commit-bot: I haz the power, 2017-02-06 21:23:42]

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