Per bug 121738, ignore old saved logins for http*://www.google.com.

Per bug 121738, ignore old saved logins for http*://www.google.com which is no longer used for Google login forms.
Google login forms are now always on https://accounts.google.com, so these saved logins should be unused.
This is a security feature to help minimize the damage that can be done by XSS attacks.
BUG=121738
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135938

[Tom Sepez, 2012-04-28 01:04:08]

One small concern, otherwise looks good.  But I'm not the best person to review
this change.  Thanks.

http://codereview.chromium.org/10209036/diff/1/chrome/browser/password_manage...
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/10209036/diff/1/chrome/browser/password_manage...
chrome/browser/password_manager/password_store.cc:91: form.signon_realm ==
"http://www.google.com/" ||
Is slash the only trailing character that could be mistakenly appended to a
realm?

http://codereview.chromium.org/10209036/diff/1/chrome/browser/password_manage...
File chrome/browser/password_manager/password_store_unittest.cc (right):

http://codereview.chromium.org/10209036/diff/1/chrome/browser/password_manage...
chrome/browser/password_manager/password_store_unittest.cc:151:
"https://www.google.com",
Might want to include a trailing slash on one of these to cover that case?

[Mike Mammarella, 2012-04-30 16:09:45]

+Tim

https://chromiumcodereview.appspot.com/10209036/diff/1/chrome/browser/passwor...
File chrome/browser/password_manager/password_store.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/1/chrome/browser/passwor...
chrome/browser/password_manager/password_store.cc:91: form.signon_realm ==
"http://www.google.com/" ||
On 2012/04/28 01:04:08, Tom Sepez wrote:
> Is slash the only trailing character that could be mistakenly appended to a
> realm?

As far as I know, yes.

https://chromiumcodereview.appspot.com/10209036/diff/1/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/1/chrome/browser/passwor...
chrome/browser/password_manager/password_store_unittest.cc:151:
"https://www.google.com",
On 2012/04/28 01:04:08, Tom Sepez wrote:
> Might want to include a trailing slash on one of these to cover that case?

Done.

[Mike Mammarella, 2012-05-02 18:58:38]

Seems Tim may be on a trip of some sort. Ilya, mind reviewing this?

[Ilya Sherman, 2012-05-02 22:10:17]

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
File chrome/browser/password_manager/password_store.h (right):

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store.h:73: time_t
ignore_logins_cutoff_;
nit: Please use base::Time for this, unless there's a compelling reason not to?

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
File chrome/browser/password_manager/password_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:38: const
std::vector<webkit::forms::PasswordForm*>&));
nit: Might as well omit the "webkit::forms::" here.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:87: typedef
std::vector<PasswordForm*> VectorOfForms;
nit: Please either shorten this to "Forms" or "FormsList", or omit it entirely. 
(My preference is to avoid this sort of typedef entirely, but the most recent
thread [1] on the topic settled on using typedefs with simpler names.)
[1]
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:87: typedef
std::vector<PasswordForm*> VectorOfForms;
nit: Please move this to the top of the file, together with the using
directives.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:98:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
nit: Can you re-use the TestingProfile's tempdir, or is it important that this
one be separate from the profile dir?

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:212: done.Wait();
Is there any named notification / callback that you could wait for here instead?
 If not, this is ok; but it would be more future-proof if the code were to wait
on an explicit notification.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:245: // Expect the
appropriate replies, as above, in reverse order that we will
nit: "that" -> "than"

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:247: // the last
which quits the message loop.
For my own edification, why is it important that the expectations are in reverse
order?

[Mike Mammarella, 2012-05-03 03:23:41]

My machine is powered off for an office move, so I can't easily make any changes
at the moment. (Also a good part of the office is going on an offsite Thu-Fri so
I'll probably get back to this on Monday.) But I've replied to your question
about test expectations since that can be done on my laptop easily, and it's an
interesting question and answer.

With regard to some of the other comments about the test - this new test, for
password_store, is based on the existing test for password_store_default. Many
of your comments apply there as well; in some cases (e.g. the typedef,
webkit::forms::) I can just make the change in both places. In other cases (the
temp dir, waitable events) I'm not necessarily sure if the existing mechanism is
the only way or not, but it has precedent, so I may go with that for consistency
unless the change is easy to make.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
File chrome/browser/password_manager/password_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:247: // the last
which quits the message loop.
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> For my own edification, why is it important that the expectations are in
reverse
> order?

Good question! When you register expectations with matchers other than _, the
matcher gets called on every call to the mock function. In our case, the matcher
uses a comparison function that generates a bunch of diagnostic output whenever
the parameters are not equal. That's great for an EXPECT_EQ but it's not great
when used as a filter like this, when we're actually intending that it be used
as a filter rather than an expectation.

The expectation are traversed in LIFO order when trying to find one that
matches, so we want the last expectation to be the one that matches the first
call to the method. That's also why we need to retire each expectation on
saturation, so that it won't be considered at all on the next call to the mock
method. The default action above is only triggered in this test if none of these
expectations matches, which should not happen when the test is passing.

[Ilya Sherman, 2012-05-04 07:22:35]

On 2012/05/03 03:23:41, Mike Mammarella wrote:
> With regard to some of the other comments about the test - this new test, for
> password_store, is based on the existing test for password_store_default. Many
> of your comments apply there as well; in some cases (e.g. the typedef,
> webkit::forms::) I can just make the change in both places. In other cases
(the
> temp dir, waitable events) I'm not necessarily sure if the existing mechanism
is
> the only way or not, but it has precedent, so I may go with that for
consistency
> unless the change is easy to make.

That's fair.  I'd appreciate if you could at least add TODOs to investigate
them, so that future readers of this code will at least know that there *might*
be cleaner approaches :)

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
File chrome/browser/password_manager/password_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:247: // the last
which quits the message loop.
On 2012/05/03 03:23:41, Mike Mammarella wrote:
> On 2012/05/02 22:10:17, Ilya Sherman wrote:
> > For my own edification, why is it important that the expectations are in
> reverse
> > order?
> 
> Good question! When you register expectations with matchers other than _, the
> matcher gets called on every call to the mock function. In our case, the
matcher
> uses a comparison function that generates a bunch of diagnostic output
whenever
> the parameters are not equal. That's great for an EXPECT_EQ but it's not great
> when used as a filter like this, when we're actually intending that it be used
> as a filter rather than an expectation.
> 
> The expectation are traversed in LIFO order when trying to find one that
> matches, so we want the last expectation to be the one that matches the first
> call to the method. That's also why we need to retire each expectation on
> saturation, so that it won't be considered at all on the next call to the mock
> method. The default action above is only triggered in this test if none of
these
> expectations matches, which should not happen when the test is passing.

Aha, I was not aware that expectations are processed in LIFO order.  Thanks for
the explanation!

[Mike Mammarella, 2012-05-07 00:14:36]

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
File chrome/browser/password_manager/password_store.h (right):

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store.h:73: time_t
ignore_logins_cutoff_;
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> nit: Please use base::Time for this, unless there's a compelling reason not
to?

Done.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
File chrome/browser/password_manager/password_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:38: const
std::vector<webkit::forms::PasswordForm*>&));
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> nit: Might as well omit the "webkit::forms::" here.

Done.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:87: typedef
std::vector<PasswordForm*> VectorOfForms;
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> nit: Please either shorten this to "Forms" or "FormsList", or omit it
entirely. 
> (My preference is to avoid this sort of typedef entirely, but the most recent
> thread [1] on the topic settled on using typedefs with simpler names.)
> [1]
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

Done.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:87: typedef
std::vector<PasswordForm*> VectorOfForms;
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> nit: Please move this to the top of the file, together with the using
> directives.

Removed.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:98:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> nit: Can you re-use the TestingProfile's tempdir, or is it important that this
> one be separate from the profile dir?

Looks like we can reuse it. Done.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:212: done.Wait();
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> Is there any named notification / callback that you could wait for here
instead?
>  If not, this is ok; but it would be more future-proof if the code were to
wait
> on an explicit notification.

Ah, so, thinking about this more I don't think there's anything we can wait for
here. We want to wait for the password store to asynchronously process several
requests, and when it's replied to all of them, then continue. It doesn't have
any idea that once it has finished with these requests it's "done," so it
doesn't have any sort of notification for that event. I've noted that in the
comment.

https://chromiumcodereview.appspot.com/10209036/diff/26011/chrome/browser/pas...
chrome/browser/password_manager/password_store_unittest.cc:245: // Expect the
appropriate replies, as above, in reverse order that we will
On 2012/05/02 22:10:17, Ilya Sherman wrote:
> nit: "that" -> "than"

Done.

[Ilya Sherman, 2012-05-07 07:15:09]

LGTM

https://chromiumcodereview.appspot.com/10209036/diff/39002/chrome/browser/pas...
File chrome/browser/password_manager/password_store.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/39002/chrome/browser/pas...
chrome/browser/password_manager/password_store.cc:92: ignore_logins_cutoff =
1325376000;  // 00:00 Jan 1 2012 UTC
nit: Please use base::Time::FromUTCExploded() -- base::Time::FromTimeT() is
deprecated.

[Mike Mammarella, 2012-05-07 15:48:13]

https://chromiumcodereview.appspot.com/10209036/diff/39002/chrome/browser/pas...
File chrome/browser/password_manager/password_store.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/39002/chrome/browser/pas...
chrome/browser/password_manager/password_store.cc:92: ignore_logins_cutoff =
1325376000;  // 00:00 Jan 1 2012 UTC
On 2012/05/07 07:15:10, Ilya Sherman wrote:
> nit: Please use base::Time::FromUTCExploded() -- base::Time::FromTimeT() is
> deprecated.

I thought about that, but it's both less concise and less efficient. (And more
ambiguous, since it's not exactly clear what happens if the day of week value
doesn't match the others.) But it is self-commenting, while I've had to
explicitly document the time this value represents here.

Would you still prefer that I change it anyway? Ultimately it'll get converted
right back to a time_t when stored anyway...

[Ilya Sherman, 2012-05-07 20:12:02]

https://chromiumcodereview.appspot.com/10209036/diff/39002/chrome/browser/pas...
File chrome/browser/password_manager/password_store.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/39002/chrome/browser/pas...
chrome/browser/password_manager/password_store.cc:92: ignore_logins_cutoff =
1325376000;  // 00:00 Jan 1 2012 UTC
On 2012/05/07 15:48:13, Mike Mammarella wrote:
> On 2012/05/07 07:15:10, Ilya Sherman wrote:
> > nit: Please use base::Time::FromUTCExploded() -- base::Time::FromTimeT() is
> > deprecated.
> 
> I thought about that, but it's both less concise and less efficient. (And more
> ambiguous, since it's not exactly clear what happens if the day of week value
> doesn't match the others.) But it is self-commenting, while I've had to
> explicitly document the time this value represents here.
> 
> Would you still prefer that I change it anyway? Ultimately it'll get converted
> right back to a time_t when stored anyway...

The TimeT functions are documented as deprecated:

// TODO(brettw) this should be removed once everybody starts using the |Time|
// class.

Hence, yes, I would still prefer that you not add new dependencies on them ;) 
Or, you could try checking with Brett whether this TODO is no longer relevant
(I'd be a little surprised if that were the case, but it's not inconceivable).

I do also like the fact that the exploded time representation is self
documenting, and typos are easier to spot; though I do agree that having to set
the day of week value is a little odd.

[Mike Mammarella, 2012-05-08 20:28:19]

https://chromiumcodereview.appspot.com/10209036/diff/47003/chrome/browser/pas...
File chrome/browser/password_manager/password_store.cc (right):

https://chromiumcodereview.appspot.com/10209036/diff/47003/chrome/browser/pas...
chrome/browser/password_manager/password_store.cc:93: { 2012, 1, 0, 1, 0, 0, 0,
0 };  // 00:00 Jan 1 2012
Hmm. This isn't actually as self-documenting as I thought it would be. I could
do one value per line to add comments per value, but still those are comments. I
still prefer the time_t constant. :)

[commit-bot: I haz the power, 2012-05-08 20:31:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mdm@chromium.org/10209036/47003

[commit-bot: I haz the power, 2012-05-08 22:51:17]

Try job failure for 10209036-47003 (retry) on win_rel for step "update".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.