Create a new AccountTrackerService

Create a new AccountTrackerService PKS based on the existing AccountTracker.

This CL is one part of the effort to change chrome to use gaiaid instead of
email address as the unique id for an account.

BUG=341408
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291263

Committed: https://crrev.com/1f295e36ff1e97eaf6a5b94c58ef757bf715371b
Cr-Commit-Position: refs/heads/master@{#291686}

[Roger Tawa OOO till Jul 10th, 2014-08-17 22:27:41]

Hi Michael,

Please take a look.  Patchset #1 is what you did an early review on.  I left it
there so you can compare against the changes I made after your initial set of
comments.  Thanks.

[Roger Tawa OOO till Jul 10th, 2014-08-19 13:50:55]

Hi Hui,

Can you take a look?  This adds a new service, but its not being used yet.  If
you want to see this change in context, take a look at these follow up CLs (in
order, as they build on each other):

https://codereview.chromium.org/475763005/
https://codereview.chromium.org/471333003/
https://codereview.chromium.org/473153002/

Thanks,

[guohui, 2014-08-20 18:05:24]

https://codereview.chromium.org/425823002/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_tracker_service_factory.h (right):

https://codereview.chromium.org/425823002/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_tracker_service_factory.h:20: // Returns the
instance of ProfileOAuth2TokenService associated with this
nits: i guess you mean AccountTrackerService here?

https://codereview.chromium.org/425823002/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_tracker_service_factory.h:26: // Returns an
instance of the ProfileOAuth2TokenServiceFactory singleton.
same here.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:224: return
state.info;
nits: no need for {}

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:240: return
state.info;
nits: no need for {}

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:261:
NotifyAccountUpdated(state);
why do we notify account updated here? It seems here the account info is not
changed.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:285: Observer,
observer_list_, OnAccountRemoved(state.info));
what is the purpose of OnAccountRemoved? It is fired when refresh token is
revoked or when user info fetcher failed. The later may occur because of network
issues. Do we really want to remove an account for those transient issues?

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:379: state.info.email
= base::UTF16ToUTF8(value);
should we notify account updated here?

[Roger Tawa OOO till Jul 10th, 2014-08-21 01:30:17]

Thanks Hui.  Please take another look.

https://codereview.chromium.org/425823002/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_tracker_service_factory.h (right):

https://codereview.chromium.org/425823002/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_tracker_service_factory.h:20: // Returns the
instance of ProfileOAuth2TokenService associated with this
On 2014/08/20 18:05:23, guohui wrote:
> nits: i guess you mean AccountTrackerService here?

Done.

https://codereview.chromium.org/425823002/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_tracker_service_factory.h:26: // Returns an
instance of the ProfileOAuth2TokenServiceFactory singleton.
On 2014/08/20 18:05:23, guohui wrote:
> same here.

Done.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:224: return
state.info;
On 2014/08/20 18:05:23, guohui wrote:
> nits: no need for {}

Done.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:240: return
state.info;
On 2014/08/20 18:05:23, guohui wrote:
> nits: no need for {}

Done.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:261:
NotifyAccountUpdated(state);
On 2014/08/20 18:05:23, guohui wrote:
> why do we notify account updated here? It seems here the account info is not
> changed.

Oops, correct.  That's a left over from when the code was sending notifications
when the sign in state of an account changed.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:285: Observer,
observer_list_, OnAccountRemoved(state.info));
On 2014/08/20 18:05:23, guohui wrote:
> what is the purpose of OnAccountRemoved? It is fired when refresh token is
> revoked or when user info fetcher failed. The later may occur because of
network
> issues. Do we really want to remove an account for those transient issues?

Good point.  Should not remove if fetcher fails.  Will need to figure out
if/when to retry on failure.

https://codereview.chromium.org/425823002/diff/420001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:379: state.info.email
= base::UTF16ToUTF8(value);
On 2014/08/20 18:05:23, guohui wrote:
> should we notify account updated here?

Yes, but only if gaiaid is not empty.

[guohui, 2014-08-21 18:23:09]

lgtm

lgtm

[Roger Tawa OOO till Jul 10th, 2014-08-21 18:27:45]

Hi Monica.

Can you do an owner review for c/b/p?  Thanks.

[noms (inactive), 2014-08-21 18:29:46]

profiles lgtm with nits

https://codereview.chromium.org/425823002/diff/480001/chrome/browser/profiles...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/425823002/diff/480001/chrome/browser/profiles...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:248:
AccountTrackerServiceFactory::GetInstance();
I think these are sorted in alphabetical order, so this should probably move way
up :)

[Roger Tawa OOO till Jul 10th, 2014-08-21 20:15:26]

Thanks Monica.

https://codereview.chromium.org/425823002/diff/480001/chrome/browser/profiles...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/425823002/diff/480001/chrome/browser/profiles...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:248:
AccountTrackerServiceFactory::GetInstance();
On 2014/08/21 18:29:46, Monica Dinculescu wrote:
> I think these are sorted in alphabetical order, so this should probably move
way
> up :)

Done.

[Roger Tawa OOO till Jul 10th, 2014-08-21 20:15:45]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-08-21 20:18:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rogerta@chromium.org/425823002/500001

[commit-bot: I haz the power, 2014-08-22 00:18:45]

Committed patchset #8 (500001) as 291263

[Lei Zhang, 2014-08-22 05:30:11]

A revert of this CL (patchset #8) has been created in
https://codereview.chromium.org/490403002/ by thestig@chromium.org.

The reason for reverting is: Likely causing
chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on Android..

[jam, 2014-08-22 14:53:19]

On 2014/08/22 05:30:11, Lei Zhang wrote:
> A revert of this CL (patchset #8) has been created in
> https://codereview.chromium.org/490403002/ by mailto:thestig@chromium.org.
> 
> The reason for reverting is: Likely causing
> chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on Android..

hmm, this fixed the tree, so thanks. However, I'd like to understand how this
had green try runs and broke the bots.

Roger: can you help me understand?

[Roger Tawa OOO till Jul 10th, 2014-08-22 15:06:04]

On 2014/08/22 14:53:19, jam wrote:
> On 2014/08/22 05:30:11, Lei Zhang wrote:
> > A revert of this CL (patchset #8) has been created in
> > https://codereview.chromium.org/490403002/ by mailto:thestig@chromium.org.
> > 
> > The reason for reverting is: Likely causing
> > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on
Android..
> 
> hmm, this fixed the tree, so thanks. However, I'd like to understand how this
> had green try runs and broke the bots.
> 
> Roger: can you help me understand?

Hi John,

My CL did indeed cause the break.  While this CL introduces a new PKS that no
code uses just yet, it still reacts to token service events.  This disrupted the
tests.  I am working on a fix right now.

As for why the try bots did not catch it, not sure.  The failing tests
(OAuth2TokenServiceIntegrationTest) did run on the android_dbg_tests_recipe bot
for patchset 8.

[jam, 2014-08-22 15:15:33]

On 2014/08/22 15:06:04, Roger Tawa wrote:
> On 2014/08/22 14:53:19, jam wrote:
> > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > A revert of this CL (patchset #8) has been created in
> > > https://codereview.chromium.org/490403002/ by mailto:thestig@chromium.org.
> > > 
> > > The reason for reverting is: Likely causing
> > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on
> Android..
> > 
> > hmm, this fixed the tree, so thanks. However, I'd like to understand how
this
> > had green try runs and broke the bots.
> > 
> > Roger: can you help me understand?
> 
> Hi John,
> 
> My CL did indeed cause the break.  While this CL introduces a new PKS that no
> code uses just yet, it still reacts to token service events.  This disrupted
the
> tests.  I am working on a fix right now.
> 
> As for why the try bots did not catch it, not sure.  The failing tests
> (OAuth2TokenServiceIntegrationTest) did run on the android_dbg_tests_recipe
bot
> for patchset 8.

Really? Isn't that
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
I clicked on "Instrumentation test ChromeShellTest" there to get to
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
and it does list
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
and
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll

[Roger Tawa OOO till Jul 10th, 2014-08-22 15:46:23]

On 2014/08/22 15:15:33, jam wrote:
> On 2014/08/22 15:06:04, Roger Tawa wrote:
> > On 2014/08/22 14:53:19, jam wrote:
> > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > A revert of this CL (patchset #8) has been created in
> > > > https://codereview.chromium.org/490403002/ by
mailto:thestig@chromium.org.
> > > > 
> > > > The reason for reverting is: Likely causing
> > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on
> > Android..
> > > 
> > > hmm, this fixed the tree, so thanks. However, I'd like to understand how
> this
> > > had green try runs and broke the bots.
> > > 
> > > Roger: can you help me understand?
> > 
> > Hi John,
> > 
> > My CL did indeed cause the break.  While this CL introduces a new PKS that
no
> > code uses just yet, it still reacts to token service events.  This disrupted
> the
> > tests.  I am working on a fix right now.
> > 
> > As for why the try bots did not catch it, not sure.  The failing tests
> > (OAuth2TokenServiceIntegrationTest) did run on the android_dbg_tests_recipe
> bot
> > for patchset 8.
> 
> Really? Isn't that
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> I clicked on "Instrumentation test ChromeShellTest" there to get to
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> and it does list
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> and
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll

Hi John,

I think we are saying the same thing :-)

I agree with you that the trybots for patchset #8 are all green.
I agree with you that the "Instrumentation test ChromeShellTest" step of the
android_dbg_tests_recipe bot ran all the OAuth2TokenServiceIntegrationTest
tests.  Why it reported no errors though, I am as clueless as you are.
The main waterfall bots did report the errors though.  Again, I don't know why
these bots would catch and report the errors while the trybots would not.  I was
able to setup an android devenv and repro the problem easily.

[jam, 2014-08-22 16:05:20]

On 2014/08/22 15:46:23, Roger Tawa wrote:
> On 2014/08/22 15:15:33, jam wrote:
> > On 2014/08/22 15:06:04, Roger Tawa wrote:
> > > On 2014/08/22 14:53:19, jam wrote:
> > > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > > A revert of this CL (patchset #8) has been created in
> > > > > https://codereview.chromium.org/490403002/ by
> mailto:thestig@chromium.org.
> > > > > 
> > > > > The reason for reverting is: Likely causing
> > > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on
> > > Android..
> > > > 
> > > > hmm, this fixed the tree, so thanks. However, I'd like to understand how
> > this
> > > > had green try runs and broke the bots.
> > > > 
> > > > Roger: can you help me understand?
> > > 
> > > Hi John,
> > > 
> > > My CL did indeed cause the break.  While this CL introduces a new PKS that
> no
> > > code uses just yet, it still reacts to token service events.  This
disrupted
> > the
> > > tests.  I am working on a fix right now.
> > > 
> > > As for why the try bots did not catch it, not sure.  The failing tests
> > > (OAuth2TokenServiceIntegrationTest) did run on the
android_dbg_tests_recipe
> > bot
> > > for patchset 8.
> > 
> > Really? Isn't that
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > I clicked on "Instrumentation test ChromeShellTest" there to get to
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > and it does list
> >
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> > and
> >
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll
> 
> Hi John,
> 
> I think we are saying the same thing :-)

sorry I misread what you wrote

> 
> I agree with you that the trybots for patchset #8 are all green.
> I agree with you that the "Instrumentation test ChromeShellTest" step of the
> android_dbg_tests_recipe bot ran all the OAuth2TokenServiceIntegrationTest
> tests.  Why it reported no errors though, I am as clueless as you are.
> The main waterfall bots did report the errors though.  Again, I don't know why
> these bots would catch and report the errors while the trybots would not.  I
was
> able to setup an android devenv and repro the problem easily.

Could it be another cl that landed between the time the trybots ran for your cl
and when it landed in the tree?

[Roger Tawa OOO till Jul 10th, 2014-08-22 16:42:50]

On 2014/08/22 16:05:20, jam wrote:
> On 2014/08/22 15:46:23, Roger Tawa wrote:
> > On 2014/08/22 15:15:33, jam wrote:
> > > On 2014/08/22 15:06:04, Roger Tawa wrote:
> > > > On 2014/08/22 14:53:19, jam wrote:
> > > > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > > > A revert of this CL (patchset #8) has been created in
> > > > > > https://codereview.chromium.org/490403002/ by
> > mailto:thestig@chromium.org.
> > > > > > 
> > > > > > The reason for reverting is: Likely causing
> > > > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures on
> > > > Android..
> > > > > 
> > > > > hmm, this fixed the tree, so thanks. However, I'd like to understand
how
> > > this
> > > > > had green try runs and broke the bots.
> > > > > 
> > > > > Roger: can you help me understand?
> > > > 
> > > > Hi John,
> > > > 
> > > > My CL did indeed cause the break.  While this CL introduces a new PKS
that
> > no
> > > > code uses just yet, it still reacts to token service events.  This
> disrupted
> > > the
> > > > tests.  I am working on a fix right now.
> > > > 
> > > > As for why the try bots did not catch it, not sure.  The failing tests
> > > > (OAuth2TokenServiceIntegrationTest) did run on the
> android_dbg_tests_recipe
> > > bot
> > > > for patchset 8.
> > > 
> > > Really? Isn't that
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > I clicked on "Instrumentation test ChromeShellTest" there to get to
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > and it does list
> > >
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> > > and
> > >
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll
> > 
> > Hi John,
> > 
> > I think we are saying the same thing :-)
> 
> sorry I misread what you wrote
> 
> > 
> > I agree with you that the trybots for patchset #8 are all green.
> > I agree with you that the "Instrumentation test ChromeShellTest" step of the
> > android_dbg_tests_recipe bot ran all the OAuth2TokenServiceIntegrationTest
> > tests.  Why it reported no errors though, I am as clueless as you are.
> > The main waterfall bots did report the errors though.  Again, I don't know
why
> > these bots would catch and report the errors while the trybots would not.  I
> was
> > able to setup an android devenv and repro the problem easily.
> 
> Could it be another cl that landed between the time the trybots ran for your
cl
> and when it landed in the tree?

I don't think so.  Now that I understand the problem, the trybot should really
have failed with my CL alone.

If you look at patchset #9, which is the same as #8 after a rebase this morning,
the "Instrumentation test ChromeShellTest" step of the
android_dbg_triggered_tests bot *is* failing.  So it seems the trybots are now
catching the error.

[jam, 2014-08-22 17:21:01]

On 2014/08/22 16:42:50, Roger Tawa wrote:
> On 2014/08/22 16:05:20, jam wrote:
> > On 2014/08/22 15:46:23, Roger Tawa wrote:
> > > On 2014/08/22 15:15:33, jam wrote:
> > > > On 2014/08/22 15:06:04, Roger Tawa wrote:
> > > > > On 2014/08/22 14:53:19, jam wrote:
> > > > > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > > > > A revert of this CL (patchset #8) has been created in
> > > > > > > https://codereview.chromium.org/490403002/ by
> > > mailto:thestig@chromium.org.
> > > > > > > 
> > > > > > > The reason for reverting is: Likely causing
> > > > > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures
on
> > > > > Android..
> > > > > > 
> > > > > > hmm, this fixed the tree, so thanks. However, I'd like to understand
> how
> > > > this
> > > > > > had green try runs and broke the bots.
> > > > > > 
> > > > > > Roger: can you help me understand?
> > > > > 
> > > > > Hi John,
> > > > > 
> > > > > My CL did indeed cause the break.  While this CL introduces a new PKS
> that
> > > no
> > > > > code uses just yet, it still reacts to token service events.  This
> > disrupted
> > > > the
> > > > > tests.  I am working on a fix right now.
> > > > > 
> > > > > As for why the try bots did not catch it, not sure.  The failing tests
> > > > > (OAuth2TokenServiceIntegrationTest) did run on the
> > android_dbg_tests_recipe
> > > > bot
> > > > > for patchset 8.
> > > > 
> > > > Really? Isn't that
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > I clicked on "Instrumentation test ChromeShellTest" there to get to
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > and it does list
> > > >
> >
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> > > > and
> > > >
> >
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll
> > > 
> > > Hi John,
> > > 
> > > I think we are saying the same thing :-)
> > 
> > sorry I misread what you wrote
> > 
> > > 
> > > I agree with you that the trybots for patchset #8 are all green.
> > > I agree with you that the "Instrumentation test ChromeShellTest" step of
the
> > > android_dbg_tests_recipe bot ran all the OAuth2TokenServiceIntegrationTest
> > > tests.  Why it reported no errors though, I am as clueless as you are.
> > > The main waterfall bots did report the errors though.  Again, I don't know
> why
> > > these bots would catch and report the errors while the trybots would not. 
I
> > was
> > > able to setup an android devenv and repro the problem easily.
> > 
> > Could it be another cl that landed between the time the trybots ran for your
> cl
> > and when it landed in the tree?
> 
> I don't think so.  Now that I understand the problem, the trybot should really
> have failed with my CL alone.
> 
> If you look at patchset #9, which is the same as #8 after a rebase this
morning,
> the "Instrumentation test ChromeShellTest" step of the
> android_dbg_triggered_tests bot *is* failing.  So it seems the trybots are now
> catching the error.

so one difference is that ps 8 had  android_dbg_tests_recipe, while 9 has
android_dbg_triggered_tests. android_dbg_tests_recipe is what replaces
android_dbg/android_dbg_triggered_tests. i've just triggered a run of
android_dbg_tests_recipe on your ps9, let's see how that runs

[jam, 2014-08-22 19:11:10]

On 2014/08/22 17:21:01, jam wrote:
> On 2014/08/22 16:42:50, Roger Tawa wrote:
> > On 2014/08/22 16:05:20, jam wrote:
> > > On 2014/08/22 15:46:23, Roger Tawa wrote:
> > > > On 2014/08/22 15:15:33, jam wrote:
> > > > > On 2014/08/22 15:06:04, Roger Tawa wrote:
> > > > > > On 2014/08/22 14:53:19, jam wrote:
> > > > > > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > > > > > A revert of this CL (patchset #8) has been created in
> > > > > > > > https://codereview.chromium.org/490403002/ by
> > > > mailto:thestig@chromium.org.
> > > > > > > > 
> > > > > > > > The reason for reverting is: Likely causing
> > > > > > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest failures
> on
> > > > > > Android..
> > > > > > > 
> > > > > > > hmm, this fixed the tree, so thanks. However, I'd like to
understand
> > how
> > > > > this
> > > > > > > had green try runs and broke the bots.
> > > > > > > 
> > > > > > > Roger: can you help me understand?
> > > > > > 
> > > > > > Hi John,
> > > > > > 
> > > > > > My CL did indeed cause the break.  While this CL introduces a new
PKS
> > that
> > > > no
> > > > > > code uses just yet, it still reacts to token service events.  This
> > > disrupted
> > > > > the
> > > > > > tests.  I am working on a fix right now.
> > > > > > 
> > > > > > As for why the try bots did not catch it, not sure.  The failing
tests
> > > > > > (OAuth2TokenServiceIntegrationTest) did run on the
> > > android_dbg_tests_recipe
> > > > > bot
> > > > > > for patchset 8.
> > > > > 
> > > > > Really? Isn't that
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > > I clicked on "Instrumentation test ChromeShellTest" there to get to
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > > and it does list
> > > > >
> > >
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> > > > > and
> > > > >
> > >
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll
> > > > 
> > > > Hi John,
> > > > 
> > > > I think we are saying the same thing :-)
> > > 
> > > sorry I misread what you wrote
> > > 
> > > > 
> > > > I agree with you that the trybots for patchset #8 are all green.
> > > > I agree with you that the "Instrumentation test ChromeShellTest" step of
> the
> > > > android_dbg_tests_recipe bot ran all the
OAuth2TokenServiceIntegrationTest
> > > > tests.  Why it reported no errors though, I am as clueless as you are.
> > > > The main waterfall bots did report the errors though.  Again, I don't
know
> > why
> > > > these bots would catch and report the errors while the trybots would
not. 
> I
> > > was
> > > > able to setup an android devenv and repro the problem easily.
> > > 
> > > Could it be another cl that landed between the time the trybots ran for
your
> > cl
> > > and when it landed in the tree?
> > 
> > I don't think so.  Now that I understand the problem, the trybot should
really
> > have failed with my CL alone.
> > 
> > If you look at patchset #9, which is the same as #8 after a rebase this
> morning,
> > the "Instrumentation test ChromeShellTest" step of the
> > android_dbg_triggered_tests bot *is* failing.  So it seems the trybots are
now
> > catching the error.
> 
> so one difference is that ps 8 had  android_dbg_tests_recipe, while 9 has
> android_dbg_triggered_tests. android_dbg_tests_recipe is what replaces
> android_dbg/android_dbg_triggered_tests. i've just triggered a run of
> android_dbg_tests_recipe on your ps9, let's see how that runs

well that finished and it's red. so i have no idea why it was green earlier.

can you do a git cl try -m tryserver.chromium.linux -b android_dbg_tests_recipe
-r291161 ? that way we can just know for sure if it wasn't due to a problem with
a colliding cl. if it's red, then we can file a bug against the android bots for
not pushing correctly.

[Roger Tawa OOO till Jul 10th, 2014-08-22 21:30:25]

On 2014/08/22 19:11:10, jam wrote:
> On 2014/08/22 17:21:01, jam wrote:
> > On 2014/08/22 16:42:50, Roger Tawa wrote:
> > > On 2014/08/22 16:05:20, jam wrote:
> > > > On 2014/08/22 15:46:23, Roger Tawa wrote:
> > > > > On 2014/08/22 15:15:33, jam wrote:
> > > > > > On 2014/08/22 15:06:04, Roger Tawa wrote:
> > > > > > > On 2014/08/22 14:53:19, jam wrote:
> > > > > > > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > > > > > > A revert of this CL (patchset #8) has been created in
> > > > > > > > > https://codereview.chromium.org/490403002/ by
> > > > > mailto:thestig@chromium.org.
> > > > > > > > > 
> > > > > > > > > The reason for reverting is: Likely causing
> > > > > > > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest
failures
> > on
> > > > > > > Android..
> > > > > > > > 
> > > > > > > > hmm, this fixed the tree, so thanks. However, I'd like to
> understand
> > > how
> > > > > > this
> > > > > > > > had green try runs and broke the bots.
> > > > > > > > 
> > > > > > > > Roger: can you help me understand?
> > > > > > > 
> > > > > > > Hi John,
> > > > > > > 
> > > > > > > My CL did indeed cause the break.  While this CL introduces a new
> PKS
> > > that
> > > > > no
> > > > > > > code uses just yet, it still reacts to token service events.  This
> > > > disrupted
> > > > > > the
> > > > > > > tests.  I am working on a fix right now.
> > > > > > > 
> > > > > > > As for why the try bots did not catch it, not sure.  The failing
> tests
> > > > > > > (OAuth2TokenServiceIntegrationTest) did run on the
> > > > android_dbg_tests_recipe
> > > > > > bot
> > > > > > > for patchset 8.
> > > > > > 
> > > > > > Really? Isn't that
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > > > I clicked on "Instrumentation test ChromeShellTest" there to get to
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > > > and it does list
> > > > > >
> > > >
> >
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> > > > > > and
> > > > > >
> > > >
> >
OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll
> > > > > 
> > > > > Hi John,
> > > > > 
> > > > > I think we are saying the same thing :-)
> > > > 
> > > > sorry I misread what you wrote
> > > > 
> > > > > 
> > > > > I agree with you that the trybots for patchset #8 are all green.
> > > > > I agree with you that the "Instrumentation test ChromeShellTest" step
of
> > the
> > > > > android_dbg_tests_recipe bot ran all the
> OAuth2TokenServiceIntegrationTest
> > > > > tests.  Why it reported no errors though, I am as clueless as you are.
> > > > > The main waterfall bots did report the errors though.  Again, I don't
> know
> > > why
> > > > > these bots would catch and report the errors while the trybots would
> not. 
> > I
> > > > was
> > > > > able to setup an android devenv and repro the problem easily.
> > > > 
> > > > Could it be another cl that landed between the time the trybots ran for
> your
> > > cl
> > > > and when it landed in the tree?
> > > 
> > > I don't think so.  Now that I understand the problem, the trybot should
> really
> > > have failed with my CL alone.
> > > 
> > > If you look at patchset #9, which is the same as #8 after a rebase this
> > morning,
> > > the "Instrumentation test ChromeShellTest" step of the
> > > android_dbg_triggered_tests bot *is* failing.  So it seems the trybots are
> now
> > > catching the error.
> > 
> > so one difference is that ps 8 had  android_dbg_tests_recipe, while 9 has
> > android_dbg_triggered_tests. android_dbg_tests_recipe is what replaces
> > android_dbg/android_dbg_triggered_tests. i've just triggered a run of
> > android_dbg_tests_recipe on your ps9, let's see how that runs
> 
> well that finished and it's red. so i have no idea why it was green earlier.
> 
> can you do a git cl try -m tryserver.chromium.linux -b
android_dbg_tests_recipe
> -r291161 ? that way we can just know for sure if it wasn't due to a problem
with
> a colliding cl. if it's red, then we can file a bug against the android bots
for
> not pushing correctly.

Hi John,

Ran try jobs as you suggested with ps9 and they seemed to fail.

ps10 contains the fix, running android bots now to see.

[Roger Tawa OOO till Jul 10th, 2014-08-22 21:35:13]

Hi Ruslan,

Can you take a look at patchset 10 please?  This suppressed the mock grant
permission activity for these tests.  Thanks,

[jam, 2014-08-22 21:53:47]

On 2014/08/22 21:30:25, Roger Tawa wrote:
> On 2014/08/22 19:11:10, jam wrote:
> > On 2014/08/22 17:21:01, jam wrote:
> > > On 2014/08/22 16:42:50, Roger Tawa wrote:
> > > > On 2014/08/22 16:05:20, jam wrote:
> > > > > On 2014/08/22 15:46:23, Roger Tawa wrote:
> > > > > > On 2014/08/22 15:15:33, jam wrote:
> > > > > > > On 2014/08/22 15:06:04, Roger Tawa wrote:
> > > > > > > > On 2014/08/22 14:53:19, jam wrote:
> > > > > > > > > On 2014/08/22 05:30:11, Lei Zhang wrote:
> > > > > > > > > > A revert of this CL (patchset #8) has been created in
> > > > > > > > > > https://codereview.chromium.org/490403002/ by
> > > > > > mailto:thestig@chromium.org.
> > > > > > > > > > 
> > > > > > > > > > The reason for reverting is: Likely causing
> > > > > > > > > > chrome.browser.signin.OAuth2TokenServiceIntegrationTest
> failures
> > > on
> > > > > > > > Android..
> > > > > > > > > 
> > > > > > > > > hmm, this fixed the tree, so thanks. However, I'd like to
> > understand
> > > > how
> > > > > > > this
> > > > > > > > > had green try runs and broke the bots.
> > > > > > > > > 
> > > > > > > > > Roger: can you help me understand?
> > > > > > > > 
> > > > > > > > Hi John,
> > > > > > > > 
> > > > > > > > My CL did indeed cause the break.  While this CL introduces a
new
> > PKS
> > > > that
> > > > > > no
> > > > > > > > code uses just yet, it still reacts to token service events. 
This
> > > > > disrupted
> > > > > > > the
> > > > > > > > tests.  I am working on a fix right now.
> > > > > > > > 
> > > > > > > > As for why the try bots did not catch it, not sure.  The failing
> > tests
> > > > > > > > (OAuth2TokenServiceIntegrationTest) did run on the
> > > > > android_dbg_tests_recipe
> > > > > > > bot
> > > > > > > > for patchset 8.
> > > > > > > 
> > > > > > > Really? Isn't that
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > > > > I clicked on "Instrumentation test ChromeShellTest" there to get
to
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> > > > > > > and it does list
> > > > > > >
> > > > >
> > >
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveOne
> > > > > > > and
> > > > > > >
> > > > >
> > >
> OAuth2TokenServiceIntegrationTest#testValidateAccountsTwoAccountsThenRemoveAll
> > > > > > 
> > > > > > Hi John,
> > > > > > 
> > > > > > I think we are saying the same thing :-)
> > > > > 
> > > > > sorry I misread what you wrote
> > > > > 
> > > > > > 
> > > > > > I agree with you that the trybots for patchset #8 are all green.
> > > > > > I agree with you that the "Instrumentation test ChromeShellTest"
step
> of
> > > the
> > > > > > android_dbg_tests_recipe bot ran all the
> > OAuth2TokenServiceIntegrationTest
> > > > > > tests.  Why it reported no errors though, I am as clueless as you
are.
> > > > > > The main waterfall bots did report the errors though.  Again, I
don't
> > know
> > > > why
> > > > > > these bots would catch and report the errors while the trybots would
> > not. 
> > > I
> > > > > was
> > > > > > able to setup an android devenv and repro the problem easily.
> > > > > 
> > > > > Could it be another cl that landed between the time the trybots ran
for
> > your
> > > > cl
> > > > > and when it landed in the tree?
> > > > 
> > > > I don't think so.  Now that I understand the problem, the trybot should
> > really
> > > > have failed with my CL alone.
> > > > 
> > > > If you look at patchset #9, which is the same as #8 after a rebase this
> > > morning,
> > > > the "Instrumentation test ChromeShellTest" step of the
> > > > android_dbg_triggered_tests bot *is* failing.  So it seems the trybots
are
> > now
> > > > catching the error.
> > > 
> > > so one difference is that ps 8 had  android_dbg_tests_recipe, while 9 has
> > > android_dbg_triggered_tests. android_dbg_tests_recipe is what replaces
> > > android_dbg/android_dbg_triggered_tests. i've just triggered a run of
> > > android_dbg_tests_recipe on your ps9, let's see how that runs
> > 
> > well that finished and it's red. so i have no idea why it was green earlier.
> > 
> > can you do a git cl try -m tryserver.chromium.linux -b
> android_dbg_tests_recipe
> > -r291161 ? that way we can just know for sure if it wasn't due to a problem
> with
> > a colliding cl. if it's red, then we can file a bug against the android bots
> for
> > not pushing correctly.
> 
> Hi John,
> 
> Ran try jobs as you suggested with ps9 and they seemed to fail.
> 
> ps10 contains the fix, running android bots now to see.

Thanks for verifying, before I saw this I sent out tryjobs as well. I've filed
https://code.google.com/p/chromium/issues/detail?id=406596 to track this, since
it seems like a serious infrastructure bug.

[aruslan, 2014-08-25 14:46:02]

android alwaysAccept lgtm

[nyquist, 2014-08-25 14:53:20]

chrome/android lgtm

[Roger Tawa OOO till Jul 10th, 2014-08-25 15:27:11]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-08-25 15:28:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rogerta@chromium.org/425823002/580001

[commit-bot: I haz the power, 2014-08-25 16:00:12]

Committed patchset #12 (580001) as 37f129bd726752325f2aed48d051bed9014f916f

[commit-bot: I haz the power, 2014-09-10 02:34:39]

Patchset 12 (id:??) landed as
https://crrev.com/1f295e36ff1e97eaf6a5b94c58ef757bf715371b
Cr-Commit-Position: refs/heads/master@{#291686}