Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22581003:
Handling of multiple concurrent requests from different clients in OAuth2TokenService (Closed)
|
Created:
7 years, 4 months ago by zel Modified:
7 years, 3 months ago CC:
chromium-reviews, tim+watch_chromium.org, rsimha+watch_chromium.org, haitaol+watch_chromium.org, Michael Courage, fgorski Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandling of multiple concurrent requests from different clients in OAuth2TokenService
BUG=268937
TEST=OAuth2TokenServiceTest.SameScopesRequestedForDifferentClients
TBR=tim (for chrome/browser/sync)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221303
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221572
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : #Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 12
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : rebase #Patch Set 12 : #Patch Set 13 : rebase #Patch Set 14 : #
Total comments: 1
Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : fixes for bustes tests #
Messages
Total messages: 39 (0 generated)
Some comments below. Michael, Filip: Zel is adding client_id support to O2TS. I think this could make it a lot easier for you to reuse O2TS in the identity api. What do you think? https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.cc:81: const std::string& chrome_client_secret, Remove |chrome_| prefix from these two args. A few places below too. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service.h (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:130: #endif +Michael and Filip. I think this should be in all version of chrome. This would be useful for the chrome identity js api. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:224: // OAuth2 refresh token. Add comment about client_id too? https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:271: ScopeSet> FetchParameters; Would be clearer to declare a struct with 3 distinct members. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service_unittest.cc (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service_unittest.cc:562: EXPECT_FALSE(factory_.GetFetcherByID(2)); Instead of this, why not expose a method like GetFetcherCountForTesting()? Or maybe a method like: std::vector<std::pair<ClientScopeSet, int waiting_request_count> > GetFetchingClientScopeSetsForTesting() Then you can really test things correctly, and you can also undo all the changes in oauth2_access_token_fetcher.cc.
Yes, this gets us closer to being able to use the service for identity API requests. We'd need to make some more changes to support our specific gaia request format, but once we have support for multiple clients we're doing pretty well. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service.h (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:130: #endif On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > +Michael and Filip. > > I think this should be in all version of chrome. This would be useful for the > chrome identity js api. Our input to a token request is (extension_id, client_id, scopes), so I suspect we'd add a new public method with those parameters, which would call to the common StartRequestForClientWithContext.
I second Michael, the method that Zel wants to make ChromeOS only is not immediately useful to Identity API. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service.h (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:271: ScopeSet> FetchParameters; On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > Would be clearer to declare a struct with 3 distinct members. Alternative solution would be to use ClientScopeSet, as that is how we are keying the TokenCache: std::pair<std::string /*refresh_token*/, ClientScopeSet> (Fetcher would not have to expose client_id, but a ClientScopeSet).
https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.cc:81: const std::string& chrome_client_secret, On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > Remove |chrome_| prefix from these two args. A few places below too. Done. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service.h (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:130: #endif On 2013/08/07 21:07:31, Michael Courage wrote: > On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > > +Michael and Filip. > > > > I think this should be in all version of chrome. This would be useful for the > > chrome identity js api. > > Our input to a token request is (extension_id, client_id, scopes), so I suspect > we'd add a new public method with those parameters, which would call to the > common StartRequestForClientWithContext. To answer the first question, atwilson@ ask me to #ifdef this for ChromeOS in the bug. I can easily remove this. I can also easily add extension_id to the list of parameters. We can maybe make it more generic - something like |request_origin|. WDYT? https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service.h:271: ScopeSet> FetchParameters; On 2013/08/07 21:36:59, Filip Gorski wrote: > On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > > Would be clearer to declare a struct with 3 distinct members. > > Alternative solution would be to use ClientScopeSet, as that is how we are > keying the TokenCache: > std::pair<std::string /*refresh_token*/, ClientScopeSet> > (Fetcher would not have to expose client_id, but a ClientScopeSet). I've used more complex key now and made this a struct - PTAL.
Looks great Zel, thanks for adding the request_id support! Michael, Filip: does this work for you? Zel: What about my comment on the unit tests in patchset 2? I think it would simplify the code and not introduce more dependencies between OAuth2TokenService tests and OAuth2AccessTokenFetcher.
Zel, I am fairly new to c++ and STL, but are you sure that the < operators will work with that definition? https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:35: const ClientScopeSet& set) const { Will this produce strict weak ordering required by the map? Here is my concern: I think that this implementation of the operator< does not guarantee asymmetry and will lead to situations where std::map using the operation will have order of elements dependent on the order in which they were inserted. That would cause a situation where you might not find what you are looking for even if it is there. Example: Consider situation where you have to sets a and b, and the following holds: a.request_origin < b.request_origin and b.client_id < a.client_id in that case both of these hold: a < b and b < a https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:60: const FetchParameters& params) const { Same comment as implementation of the ClientScopeSet::operator<
https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:60: const FetchParameters& params) const { On 2013/08/08 17:25:33, Filip Gorski wrote: > Same comment as implementation of the ClientScopeSet::operator< you are absolutely right good catch! fixing that now and adding unit tests...
https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:35: const ClientScopeSet& set) const { On 2013/08/08 17:25:33, Filip Gorski wrote: > Will this produce strict weak ordering required by the map? > > Here is my concern: > I think that this implementation of the operator< does not guarantee asymmetry > and will lead to situations where std::map using the operation will have order > of elements dependent on the order in which they were inserted. That would cause > a situation where you might not find what you are looking for even if it is > there. > > Example: > Consider situation where you have to sets a and b, and the following holds: > a.request_origin < b.request_origin and b.client_id < a.client_id > > in that case both of these hold: > a < b and b < a The ordering should be fixed now and covered with unit tests. PTAL. https://codereview.chromium.org/22581003/diff/22001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:60: const FetchParameters& params) const { On 2013/08/08 18:31:20, zel wrote: > On 2013/08/08 17:25:33, Filip Gorski wrote: > > Same comment as implementation of the ClientScopeSet::operator< > > you are absolutely right good catch! fixing that now and adding unit tests... Done.
https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service_unittest.cc (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service_unittest.cc:562: EXPECT_FALSE(factory_.GetFetcherByID(2)); On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > Instead of this, why not expose a method like GetFetcherCountForTesting()? > > Or maybe a method like: > > std::vector<std::pair<ClientScopeSet, int waiting_request_count> > > GetFetchingClientScopeSetsForTesting() > > Then you can really test things correctly, and you can also undo all the changes > in oauth2_access_token_fetcher.cc. The biggest problem here was that |factory_| implementation does not really store anything else other than the last fetcher at index 0 (factory_.GetFetcherByID(0)) since fetcher's id was hardcoded to 0 in OAuth2AccessTokenFetcher. So, the count of fetcher would always be 1 or 0... I have exposed factory_.GetFetcherCount() to make some checks more human readable in these unit test. PTAL.
Sorry Zel, I was not very clear. See below for more details of what I really meant. https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service_unittest.cc (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service_unittest.cc:562: EXPECT_FALSE(factory_.GetFetcherByID(2)); On 2013/08/08 20:56:20, zel wrote: > On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > > Instead of this, why not expose a method like GetFetcherCountForTesting()? > > > > Or maybe a method like: > > > > std::vector<std::pair<ClientScopeSet, int waiting_request_count> > > > GetFetchingClientScopeSetsForTesting() > > > > Then you can really test things correctly, and you can also undo all the > changes > > in oauth2_access_token_fetcher.cc. > > The biggest problem here was that |factory_| implementation does not really > store anything else other than the last fetcher at index 0 > (factory_.GetFetcherByID(0)) since fetcher's id was hardcoded to 0 in > OAuth2AccessTokenFetcher. So, the count of fetcher would always be 1 or 0... > > I have exposed factory_.GetFetcherCount() to make some checks more human > readable in these unit test. PTAL. I meant putting these methods on OAuth2TokenService, not the fetcher factory. So for example you could replace the lines 572-576 with: ASSERT_EQ(2U, oauth2_service->GetFetcherCount()); As for the second proposed new method, maybe a better prototype would be: const std::map<FetchParameters, Fetcher*>& OAuth2TokenService::GetPendingFetchersForTesting(); and you could then do something like: const std::map<FetchParameters, Fetcher*>& fetchers = oauth2_service->GetPendingFetchersForTesting(); ASSERT_EQ(2U, fetchers[FetchParameters(std::string(), client_id_1, oauth2_service->GetRefreshToken(), scope_set)]->GetWaitingRequestCount()); ASSERT_EQ(1U, fetchers[ClientScopeSet(std::string(), client_id_2, oauth2_service->GetRefreshToken(), scope_set)]->GetWaitingRequestCount());
https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... File chrome/browser/signin/oauth2_token_service_unittest.cc (right): https://codereview.chromium.org/22581003/diff/3001/chrome/browser/signin/oaut... chrome/browser/signin/oauth2_token_service_unittest.cc:562: EXPECT_FALSE(factory_.GetFetcherByID(2)); On 2013/08/09 14:22:14, Roger Tawa wrote: > On 2013/08/08 20:56:20, zel wrote: > > On 2013/08/07 19:56:49, Roger Tawa (Google) wrote: > > > Instead of this, why not expose a method like GetFetcherCountForTesting()? > > > > > > Or maybe a method like: > > > > > > std::vector<std::pair<ClientScopeSet, int waiting_request_count> > > > > GetFetchingClientScopeSetsForTesting() > > > > > > Then you can really test things correctly, and you can also undo all the > > changes > > > in oauth2_access_token_fetcher.cc. > > > > The biggest problem here was that |factory_| implementation does not really > > store anything else other than the last fetcher at index 0 > > (factory_.GetFetcherByID(0)) since fetcher's id was hardcoded to 0 in > > OAuth2AccessTokenFetcher. So, the count of fetcher would always be 1 or 0... > > > > I have exposed factory_.GetFetcherCount() to make some checks more human > > readable in these unit test. PTAL. > > I meant putting these methods on OAuth2TokenService, not the fetcher factory. > So for example you could replace the lines 572-576 with: > > ASSERT_EQ(2U, oauth2_service->GetFetcherCount()); > > As for the second proposed new method, maybe a better prototype would be: > > const std::map<FetchParameters, Fetcher*>& > OAuth2TokenService::GetPendingFetchersForTesting(); > > and you could then do something like: > > const std::map<FetchParameters, Fetcher*>& fetchers = > oauth2_service->GetPendingFetchersForTesting(); > > ASSERT_EQ(2U, > fetchers[FetchParameters(std::string(), > client_id_1, > oauth2_service->GetRefreshToken(), > scope_set)]->GetWaitingRequestCount()); > ASSERT_EQ(1U, > fetchers[ClientScopeSet(std::string(), > client_id_2, > oauth2_service->GetRefreshToken(), > scope_set)]->GetWaitingRequestCount()); > OK, check it out now... way more stuff needed to be moved around with this approach.
lgtm Thanks Zel for putting up with my comments and addressing them. I still think you can simplify this CL with the comments below. If you did, you would not need any of the changes in oauth2_access_token_fetcher.h|cc and test_url_fetcher_factory.h|cc. But I'll leave that up to you. https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service_unittest.cc (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service_unittest.cc:577: ASSERT_TRUE(fetcher2); Lines 574 to 577 seem redundant with line 573. I even wonder if calling |factory_.GetFetcherCount()| in any of the tests is a good thing, since that tests the implementation and not the api contract. https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service_unittest.cc:603: EXPECT_EQ(0, consumer_.number_of_errors_); Don't need lines 594 to 603 for this test.
https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:34: (!(s.request_origin < request_origin) && (client_id < s.client_id))) || I wonder if this would be more legible as if statements: if (request < s.request_origin) return true; if (client_id < s.client_id) return true; if (scopes < s.scopes) return true; return false; https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:55: (!(p.request_origin < request_origin) && See my previous comment. This nested logic just seems like it's crying out to be broken subtly via a typo. https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.h (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.h:128: // the internal chrome services while we will use webapp id for their So, one thing this CL (and the associated bug) is missing is an explanation about why we want to silo requests on a per-request-origin basis. If two apps want to access the same service (they pass the same scope - say userinfo.email) why would we want to mint two separate access tokens? Seems like siloing only based on client_id would be sufficient. Also, it seems like we can't implement this API at all on Android, because Chrome doesn't do the caching on that platform (we rely entirely on the Android AccountManager to do the caching, and it doesn't know anything about request_origin, client_id, etc). Not a huge issue, but maybe we should #if !defined(OS_ANDROID) here... https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.h:236: // Class that fetches an OAuth2 access token for a given set of scopes and I don't really like the fact that we're exposing this in our header file just so a test can call GetWaitingRequestCount(). Can we move this back into the .cc file, and instead just expose a simple "GetNumPendingRequestsForTesting(request_origin, client_id, refresh_token, scopes)"?
https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:34: (!(s.request_origin < request_origin) && (client_id < s.client_id))) || On 2013/08/19 14:04:19, Andrew T Wilson wrote: > I wonder if this would be more legible as if statements: > > if (request < s.request_origin) return true; > if (client_id < s.client_id) return true; > if (scopes < s.scopes) return true; > return false; I've also got deceived believing that what you propose would work, but Filip cleverly spotted that this logic is pretty incorrect and hence these new correct checks are in place - they are not not as obvious as you read them, but they are correct nevertheless. For more details, see Filip's comments from the beginning of the above. https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:55: (!(p.request_origin < request_origin) && On 2013/08/19 14:04:19, Andrew T Wilson wrote: > See my previous comment. This nested logic just seems like it's crying out to be > broken subtly via a typo. See my comments above. There aren't many obvious ways to make this both more readable and still accurate. https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service_unittest.cc (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service_unittest.cc:577: ASSERT_TRUE(fetcher2); On 2013/08/16 21:16:50, Roger Tawa wrote: > Lines 574 to 577 seem redundant with line 573. I even wonder if calling > |factory_.GetFetcherCount()| in any of the tests is a good thing, since that > tests the implementation and not the api contract. Done. https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service_unittest.cc:603: EXPECT_EQ(0, consumer_.number_of_errors_); On 2013/08/16 21:16:50, Roger Tawa wrote: > Don't need lines 594 to 603 for this test. Done.
https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:34: (!(s.request_origin < request_origin) && (client_id < s.client_id))) || On 2013/08/19 16:03:17, zel wrote: > On 2013/08/19 14:04:19, Andrew T Wilson wrote: > > I wonder if this would be more legible as if statements: > > > > if (request < s.request_origin) return true; > > if (client_id < s.client_id) return true; > > if (scopes < s.scopes) return true; > > return false; > > I've also got deceived believing that what you propose would work, but Filip > cleverly spotted that this logic is pretty incorrect and hence these new correct > checks are in place - they are not not as obvious as you read them, but they are > correct nevertheless. For more details, see Filip's comments from the beginning > of the above. There is a way to rewrite the conditions in a longer form - more readable and less prone to typo errors: if (request_origin < s.request_origin) return true; else if (s.request_origin < request_origin) return false; // at this point you know they are equal and you just move on to the next field if (client_id < ... Added advantage is that every field will be compared at most twice and it is easier to spot a typo since every if has pretty much a canonical form.
https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... File chrome/browser/signin/oauth2_token_service.cc (right): https://codereview.chromium.org/22581003/diff/57001/chrome/browser/signin/oau... chrome/browser/signin/oauth2_token_service.cc:34: (!(s.request_origin < request_origin) && (client_id < s.client_id))) || On 2013/08/19 16:16:06, Filip Gorski wrote: > On 2013/08/19 16:03:17, zel wrote: > > On 2013/08/19 14:04:19, Andrew T Wilson wrote: > > > I wonder if this would be more legible as if statements: > > > > > > if (request < s.request_origin) return true; > > > if (client_id < s.client_id) return true; > > > if (scopes < s.scopes) return true; > > > return false; > > > > I've also got deceived believing that what you propose would work, but Filip > > cleverly spotted that this logic is pretty incorrect and hence these new > correct > > checks are in place - they are not not as obvious as you read them, but they > are > > correct nevertheless. For more details, see Filip's comments from the > beginning > > of the above. > > There is a way to rewrite the conditions in a longer form - more readable and > less prone to typo errors: > if (request_origin < s.request_origin) return true; > else if (s.request_origin < request_origin) return false; > // at this point you know they are equal and you just move on to the next field > if (client_id < ... > > Added advantage is that every field will be compared at most twice and it is > easier to spot a typo since every if has pretty much a canonical form. Right. Done. PTAL.
lgtm
Patch set 10 LGTM. I only reviewed the two files in net.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/90001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/90001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Still not understanding why we want to silo requests on request_id as opposed to merely client_id - sent an email to Zel directly about this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/118001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/151001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/151001
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Zel, I found out why the CQ is failing (at least one of the reasons) https://codereview.chromium.org/22581003/diff/151001/google_apis/gaia/oauth2_... File google_apis/gaia/oauth2_access_token_fetcher.h (right): https://codereview.chromium.org/22581003/diff/151001/google_apis/gaia/oauth2_... google_apis/gaia/oauth2_access_token_fetcher.h:98: static void ResetLastFetcherIdForTest(); To check in you will either need to make the method public or friend UserPolicySigninServiceTest.SignOutThenSignInAgain, and add a reset in line 788 of that file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/202001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/186001
Retried try job too often on mac_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/179001
Message was sent while issue was closed.
Change committed as 221303
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/22581003/273001
Message was sent while issue was closed.
Change committed as 221572 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
