[Password Manager] Fix crash in reporting sync stats.

[Password Manager] Fix crash in reporting sync stats.

It is possible for password_sync_metrics::IsPasswordSyncUsername() to be
called during sync setup, which currently causes a stack overflow since
GetPasswordSyncUsername() will try to re-initialize sync. Fixed by not checking
to see if the user is syncing passwords, just if they have a saved username.
This is slightly less specific, but determining if password sync is occuring
during setup doesn't seem reasonable.

R=isherman@chromium.org
TBR=vabr@chromium.org
BUG=393626, 386403
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283254

[Ilya Sherman, 2014-07-15 01:09:12]

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/sync_metrics.cc (left):

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.cc:22: return "";
Is this the code that you're saying causes Sync to be reinitialized?  If so, how
so?  (Also, the CL description says that the method name is
IsPasswordSyncUsername(), which doesn't seem to be one of the existing method
names... or maybe I'm just overlooking it.)

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/sync_metrics.h (right):

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.h:17: // this function can be
called during sync setup (crbug.com/393626).
nit: Please prepend "http://" to the link to increase likelihood of
autolinkification.

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.h:17: // this function can be
called during sync setup (crbug.com/393626).
I'm pretty sure it's possible to check whether Sync is currently being set up or
not.  Would you be happy if the metric simply weren't emitted during Sync setup?

[Ilya Sherman, 2014-07-15 01:09:26]

BTW, if we do go with this change, should histograms.xml be updated?

[Garrett Casto, 2014-07-15 05:39:04]

I actually checked histograms.xml, and the relevant histogram doesn't
specifically call out password sync at the moment, so I don't believe it needs
to be changed.

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/sync_metrics.cc (left):

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.cc:22: return "";
On 2014/07/15 01:09:11, Ilya Sherman wrote:
> Is this the code that you're saying causes Sync to be reinitialized?  If so,
how
> so?  (Also, the CL description says that the method name is
> IsPasswordSyncUsername(), which doesn't seem to be one of the existing method
> names... or maybe I'm just overlooking it.)

ProfileSyncServiceFactory::GetForProfile() will create a a ProfileSyncService if
one isn't associated with the given profile. As part of this process,
PasswordStoreFactory::GetForProfile() is called (see
SyncRegistrar::SyncRegistrar()). Creating a new PasswordStore will call this
function and we go around until the stack blows up. 
And the function name is a typo in the CL description. It should be
GetPasswordSyncUsername().

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/sync_metrics.h (right):

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.h:17: // this function can be
called during sync setup (crbug.com/393626).
On 2014/07/15 01:09:11, Ilya Sherman wrote:
> I'm pretty sure it's possible to check whether Sync is currently being set up
or
> not.  Would you be happy if the metric simply weren't emitted during Sync
setup?

I haven't tested this yet, but I'm guessing it's pretty likely to guaranteed
that if you are syncing the first PasswordStoreFactory::GetForProfile() call is
going to happen because of sync. Even if it's more random than that, if we did
something like that it seems like we would want to have different histograms
based on if the username was for password sync or just sync. Just seems cleaner
this way.

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.h:17: // this function can be
called during sync setup (crbug.com/393626).
On 2014/07/15 01:09:11, Ilya Sherman wrote:
> nit: Please prepend "http://" to the link to increase likelihood of
> autolinkification.

Currently moving offices, so I'll get this when my computer is hooked up
tomorrow.

[Ilya Sherman, 2014-07-15 05:48:13]

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/sync_metrics.cc (left):

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.cc:22: return "";
On 2014/07/15 05:39:03, Garrett Casto wrote:
> On 2014/07/15 01:09:11, Ilya Sherman wrote:
> > Is this the code that you're saying causes Sync to be reinitialized?  If so,
> how
> > so?  (Also, the CL description says that the method name is
> > IsPasswordSyncUsername(), which doesn't seem to be one of the existing
method
> > names... or maybe I'm just overlooking it.)
> 
> ProfileSyncServiceFactory::GetForProfile() will create a a ProfileSyncService
if
> one isn't associated with the given profile. As part of this process,
> PasswordStoreFactory::GetForProfile() is called (see
> SyncRegistrar::SyncRegistrar()). Creating a new PasswordStore will call this
> function and we go around until the stack blows up. 
> And the function name is a typo in the CL description. It should be
> GetPasswordSyncUsername().

Can you use ProfileSyncServiceFactory::HasProfileSyncService()?

[Garrett Casto, 2014-07-15 06:05:55]

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/sync_metrics.cc (left):

https://codereview.chromium.org/383083009/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/sync_metrics.cc:22: return "";
On 2014/07/15 05:48:13, Ilya Sherman wrote:
> On 2014/07/15 05:39:03, Garrett Casto wrote:
> > On 2014/07/15 01:09:11, Ilya Sherman wrote:
> > > Is this the code that you're saying causes Sync to be reinitialized?  If
so,
> > how
> > > so?  (Also, the CL description says that the method name is
> > > IsPasswordSyncUsername(), which doesn't seem to be one of the existing
> method
> > > names... or maybe I'm just overlooking it.)
> > 
> > ProfileSyncServiceFactory::GetForProfile() will create a a
ProfileSyncService
> if
> > one isn't associated with the given profile. As part of this process,
> > PasswordStoreFactory::GetForProfile() is called (see
> > SyncRegistrar::SyncRegistrar()). Creating a new PasswordStore will call this
> > function and we go around until the stack blows up. 
> > And the function name is a typo in the CL description. It should be
> > GetPasswordSyncUsername().
> 
> Can you use ProfileSyncServiceFactory::HasProfileSyncService()?

I could. It just wasn't clearly useful to me, as I mentioned in the other
comment. That is, either this intermittently allows us to check if we are
syncing passwords, which seems like it may skew the data, or it's never possible
to check once password syncing start. I should verify tomorrow which one it is,
but it doesn't seem like in either scenario it's worth trying to extract this
information.

[Ilya Sherman, 2014-07-15 18:17:48]

If you don't think it's useful to check for password syncing state for this
metric, then LGTM with the http:// prefix nit addressed.  Thanks, Garrett.

[Garrett Casto, 2014-07-15 20:05:50]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-15 20:08:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/383083009/40001

[Garrett Casto, 2014-07-15 21:07:20]

TBRing vabr@ for the rename in password_manager_presenter.cc.

[Garrett Casto, 2014-07-15 21:09:14]

Committed patchset #3 manually as r283254 (presubmit successful).