[GCM] Fetching OAuth2 tokens periodically in account tracker

[GCM] Fetching OAuth2 tokens periodically in account tracker

BUG=374969
R=zea@chromium.org

Committed: https://crrev.com/ede4117a85d34a1c117169d538b297020e69de87
Cr-Commit-Position: refs/heads/master@{#303340}

[Nicolas Zea, 2014-10-08 00:45:29]

Does this patch need to be diffed from the other?

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_account_tracker.cc:48: kGCMLastTokenFetchingTs,
remind me why we store this in the prefs rather than the gcm store? Do we clear
it on last app uninstall as well?

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_account_tracker.h:33: class GCMAccountTracker :
public gaia::AccountTracker::Observer,
FYI I think it would be good to explain _why_ we're doing a periodic check in
the first place. My instinct when thinking about this is that periodic work
shouldn't be necessary if we only care about local data. Explaining that this is
actually checking that the token is valid periodically would probably make
things clearer.

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_account_tracker.h:160: base::Time
last_token_fetch_time_;
nit: comment about what this is for

[fgorski, 2014-11-06 01:16:14]

Nicolas, please take a look. I addressed the initial comments and added some
tests.

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_account_tracker.cc:48: kGCMLastTokenFetchingTs,
On 2014/10/08 00:45:29, Nicolas Zea wrote:
> remind me why we store this in the prefs rather than the gcm store? Do we
clear
> it on last app uninstall as well?

Done.

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_account_tracker.h:33: class GCMAccountTracker :
public gaia::AccountTracker::Observer,
On 2014/10/08 00:45:29, Nicolas Zea wrote:
> FYI I think it would be good to explain _why_ we're doing a periodic check in
> the first place. My instinct when thinking about this is that periodic work
> shouldn't be necessary if we only care about local data. Explaining that this
is
> actually checking that the token is valid periodically would probably make
> things clearer.

Done.

https://codereview.chromium.org/631343002/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_account_tracker.h:160: base::Time
last_token_fetch_time_;
On 2014/10/08 00:45:29, Nicolas Zea wrote:
> nit: comment about what this is for

Done.

[Nicolas Zea, 2014-11-06 21:43:17]

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.cc:172: SanitizeTokens();
Why do we ignore the return value now and use IsTokenFetchingRequired instead?
Isn't that what SanitizeTokens returns?

Maybe we should just make SanitizeTokens a void method and update the comments?

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.cc:303: // fetching.
What code is listening to the network change?

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:16: #include
"testing/gtest/include/gtest/gtest_prod.h"
why is this needed?

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:30: // still logged into the
profile, so that in the case that the user is not, we
Is it just to ensure that we're still logged into the profile? Or is it to
ensure that the tokens themselves haven't expired or been revoked remotely?

We shouldn't need to periodically poll for a local event (i.e. sign out). We can
listen for those events directly.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:157:
base::WeakPtrFactory<GCMAccountTracker> weak_ptr_factory_;
nit: Given that this is specifically for reporting tokens, perhaps we should
name it something else? If someone were to attempt to use this for binding to
other methods, the InvalidateAll might introduce bugs.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (left):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:115: 
Did you mean to remove this?

https://codereview.chromium.org/631343002/diff/40001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/631343002/diff/40001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.cc:366:
last_token_fetch_time_(base::Time::Max()),  // set to max as a sentinel
Why Max, vs null?

[fgorski, 2014-11-07 01:42:13]

Fixed code review comments.

Nicolas, there is 7 tests failing in sync integration tests. I looked at those
with Pavel, but we have no clue so far. I may drop by Seattle tomorrow to figure
it out.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.cc:172: SanitizeTokens();
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> Why do we ignore the return value now and use IsTokenFetchingRequired instead?
> Isn't that what SanitizeTokens returns?
> 
> Maybe we should just make SanitizeTokens a void method and update the
comments?

I think that was the comment we discussed with one of the previous patches. I
was meant to fix it. Thanks for the catch.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.cc:303: // fetching.
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> What code is listening to the network change?

Lines 162:169 above.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:16: #include
"testing/gtest/include/gtest/gtest_prod.h"
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> why is this needed?

Done.
In the end I made the test class a friend

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:30: // still logged into the
profile, so that in the case that the user is not, we
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> Is it just to ensure that we're still logged into the profile? Or is it to
> ensure that the tokens themselves haven't expired or been revoked remotely?
> 
> We shouldn't need to periodically poll for a local event (i.e. sign out). We
can
> listen for those events directly.

Every checkin with GCM should have a fresh set of tokens. We are watching for
the event, but on top of that want to periodically get a fresh batch of tokens
to report (in case the user never restarts Chrome.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:157:
base::WeakPtrFactory<GCMAccountTracker> weak_ptr_factory_;
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> nit: Given that this is specifically for reporting tokens, perhaps we should
> name it something else? If someone were to attempt to use this for binding to
> other methods, the InvalidateAll might introduce bugs. 

Done.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (left):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:115: 
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> Did you mean to remove this?

Done. Definitely an accident.

https://codereview.chromium.org/631343002/diff/40001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/631343002/diff/40001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.cc:366:
last_token_fetch_time_(base::Time::Max()),  // set to max as a sentinel
On 2014/11/06 21:43:17, Nicolas Zea wrote:
> Why Max, vs null?

Max > Now() which means reporting will not be triggered until date in the past
(perhaps Min) is read from the DB and set here. I added a comment that tries to
address that.

In short. Null or Min (meaning never happend, are valid values that will trigger
activity)

In cases when tokens have been reported in the past we might wipe that
information and then again populate it. Once the read from DB is over, we are in
a much safer situation.

[fgorski, 2014-11-07 21:51:15]

Patchset #5 (id:80001) has been deleted

[Nicolas Zea, 2014-11-07 22:19:25]

Mostly LG, just some cleanup comments.

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_account_tracker.h:30: // still logged into the
profile, so that in the case that the user is not, we
On 2014/11/07 01:42:13, fgorski wrote:
> On 2014/11/06 21:43:17, Nicolas Zea wrote:
> > Is it just to ensure that we're still logged into the profile? Or is it to
> > ensure that the tokens themselves haven't expired or been revoked remotely?
> > 
> > We shouldn't need to periodically poll for a local event (i.e. sign out). We
> can
> > listen for those events directly.
> 
> Every checkin with GCM should have a fresh set of tokens. We are watching for
> the event, but on top of that want to periodically get a fresh batch of tokens
> to report (in case the user never restarts Chrome.

I think its worth mentioning this part in the comment. We're not really fetching
tokens periodically to see if we're logged in (although that checks that too),
but to actually have a fresh set of access tokens to send to the Checkin server.

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.cc:249: if
(GetTimeToNextTokenFetching() == base::TimeDelta())
should this be in IsTokenFetchingRequired? I'm finding myself confused what the
relation is between when reporting is required vs when fetching is required.

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:111: // Schedules fetching
tokens at the time when they are needed to be reported.
nit: "at the time when they are needed to be reported" -> "if new tokens are
required (see IsTokenReportingRequired)"

Also, does it make more sense to name this ScheduleReportTokens? It's kind of
strange that this is calling ReportTokens, rather than triggering Fetching. In
my mind fetching and reporting are independent things.

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:156:
base::WeakPtrFactory<GCMAccountTracker> reporting_weak_ptr_factory_;
nit: comment that this is for ReportTokens usage.

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:475: gcm_store_->SetLastTokenFetchTime(
How about just not calling this at all if we're uninitialized? (seems like this
would probably apply to the other account mapping methods above too, right?)

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:233: // Callback for store operation
where result does not mapper.
nit: mapper -> matter

[fgorski, 2014-11-07 22:57:03]

PTAL

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.cc:249: if
(GetTimeToNextTokenFetching() == base::TimeDelta())
On 2014/11/07 22:19:25, Nicolas Zea wrote:
> should this be in IsTokenFetchingRequired? I'm finding myself confused what
the
> relation is between when reporting is required vs when fetching is required.

Renamed to GetTimeToNextTokenReporting

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:111: // Schedules fetching
tokens at the time when they are needed to be reported.
On 2014/11/07 22:19:25, Nicolas Zea wrote:
> nit: "at the time when they are needed to be reported" -> "if new tokens are
> required (see IsTokenReportingRequired)"
> 
> Also, does it make more sense to name this ScheduleReportTokens? It's kind of
> strange that this is calling ReportTokens, rather than triggering Fetching. In
> my mind fetching and reporting are independent things.

Done. Restructured and renamed the GetTimeTo... as well.

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:156:
base::WeakPtrFactory<GCMAccountTracker> reporting_weak_ptr_factory_;
On 2014/11/07 22:19:25, Nicolas Zea wrote:
> nit: comment that this is for ReportTokens usage.

Done.

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:475: gcm_store_->SetLastTokenFetchTime(
On 2014/11/07 22:19:25, Nicolas Zea wrote:
> How about just not calling this at all if we're uninitialized? (seems like
this
> would probably apply to the other account mapping methods above too, right?)

tracker does not call the other methods. GCMAccountMapper does it, and it is
typically shutdown properly.
as for checking the state, check this out:

void GCMClientImpl::Stop() {
  DVLOG(1) << "Stopping the GCM Client";
  weak_ptr_factory_.InvalidateWeakPtrs();
  device_checkin_info_.Reset();
  connection_factory_.reset();
  delegate_->OnDisconnected();
  mcs_client_.reset();
  checkin_request_.reset();
  // Delete all of the pending registration requests, whithout telling the
  // consumers.
  // TODO(fgorski): Perhaps we should make a distinction between a Stop and a
  // Shutdown.
  STLDeleteValues(&pending_registration_requests_);
  state_ = INITIALIZED;
  gcm_store_->Close();
}

specifically last two lines...
This might be a genuine bug actually. But I'd rather not fix it as part of this
patch.

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:233: // Callback for store operation
where result does not mapper.
On 2014/11/07 22:19:25, Nicolas Zea wrote:
> nit: mapper -> matter

Done.

[Nicolas Zea, 2014-11-07 23:03:23]

Makes more sense now, thanks!

LGTM with a couple comments.

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:156:
base::WeakPtrFactory<GCMAccountTracker> reporting_weak_ptr_factory_;
On 2014/11/07 22:57:02, fgorski wrote:
> On 2014/11/07 22:19:25, Nicolas Zea wrote:
> > nit: comment that this is for ReportTokens usage.
> 
> Done.

Looks like this didn't make it into the patch?

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/631343002/diff/100001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:475: gcm_store_->SetLastTokenFetchTime(
On 2014/11/07 22:57:02, fgorski wrote:
> On 2014/11/07 22:19:25, Nicolas Zea wrote:
> > How about just not calling this at all if we're uninitialized? (seems like
> this
> > would probably apply to the other account mapping methods above too, right?)
> 
> tracker does not call the other methods. GCMAccountMapper does it, and it is
> typically shutdown properly.
> as for checking the state, check this out:
> 
> void GCMClientImpl::Stop() {
>   DVLOG(1) << "Stopping the GCM Client";
>   weak_ptr_factory_.InvalidateWeakPtrs();
>   device_checkin_info_.Reset();
>   connection_factory_.reset();
>   delegate_->OnDisconnected();
>   mcs_client_.reset();
>   checkin_request_.reset();
>   // Delete all of the pending registration requests, whithout telling the
>   // consumers.
>   // TODO(fgorski): Perhaps we should make a distinction between a Stop and a
>   // Shutdown.
>   STLDeleteValues(&pending_registration_requests_);
>   state_ = INITIALIZED;
>   gcm_store_->Close();
> }
> 
> specifically last two lines...
> This might be a genuine bug actually. But I'd rather not fix it as part of
this
> patch.

Okay, maybe file followup bug?

https://codereview.chromium.org/631343002/diff/120001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/120001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.cc:270: base::TimeDelta
time_till_next_fetching =
I'm still confused that these variables (and the methods in GCMDriver) are all
about fetching. Should they be renamed too? (can be done as a separate patch)

[fgorski, 2014-11-07 23:28:45]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2014-11-07 23:29:07]

Updated. Flipping the CQ

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.h (right):

https://codereview.chromium.org/631343002/diff/100001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.h:156:
base::WeakPtrFactory<GCMAccountTracker> reporting_weak_ptr_factory_;
On 2014/11/07 23:03:23, Nicolas Zea wrote:
> On 2014/11/07 22:57:02, fgorski wrote:
> > On 2014/11/07 22:19:25, Nicolas Zea wrote:
> > > nit: comment that this is for ReportTokens usage.
> > 
> > Done.
> 
> Looks like this didn't make it into the patch?

didn't :w it. Updated.

https://codereview.chromium.org/631343002/diff/120001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_account_tracker.cc (right):

https://codereview.chromium.org/631343002/diff/120001/chrome/browser/services...
chrome/browser/services/gcm/gcm_account_tracker.cc:270: base::TimeDelta
time_till_next_fetching =
On 2014/11/07 23:03:23, Nicolas Zea wrote:
> I'm still confused that these variables (and the methods in GCMDriver) are all
> about fetching. Should they be renamed too? (can be done as a separate patch)

Done.

[commit-bot: I haz the power, 2014-11-07 23:30:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631343002/140001

[commit-bot: I haz the power, 2014-11-08 00:39:00]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-11-08 00:39:40]

Patchset 7 (id:??) landed as
https://crrev.com/ede4117a85d34a1c117169d538b297020e69de87
Cr-Commit-Position: refs/heads/master@{#303340}

[matthewyuan, 2014-11-08 22:29:40]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.chromium.org/710903002/ by matthewyuan@chromium.org.

The reason for reverting is: Reverting since it's causing a crash on canary..