Remove Obsolete HTTP SiteStatistics From PasswordStore during Migration

Remove Obsolete HTTP SiteStatistics From PasswordStore during Migration

As a follow up to a comment in http://crrev.com/2721663002 this change
implements the removal of obsolete site statistics when a site migrated to HTTPS
and has HSTS enabled.

BUG=687968
R=vasilii@chromium.org

Review-Url: https://codereview.chromium.org/2748933005
Cr-Commit-Position: refs/heads/master@{#458836}
Committed: https://chromium.googlesource.com/chromium/src/+/7dc7688b853c5ae80cae066021d02c591e06a1e2

[jdoerrie, 2017-03-15 12:19:12]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 12:19:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-03-15 12:21:45]

Hi Vasilii, please review :)

[commit-bot: I haz the power, 2017-03-15 13:11:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 13:11:25]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-03-15 13:27:59]

https://codereview.chromium.org/2748933005/diff/1/components/password_manager...
File components/password_manager/core/browser/http_password_store_migrator.cc
(right):

https://codereview.chromium.org/2748933005/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_store_migrator.cc:72:
client_->GetPasswordStore()->RemoveSiteStats(http_origin_domain_);
I think it'a valid to assume that HttpPasswordStoreMigrator may be destroyed in
ProcessPasswordStoreResults() (through ProcessMigratedForms). Therefore, this
line should go up.
Ideally we need a test for this if we still don't have it.

[jdoerrie, 2017-03-22 13:29:22]

Sorry for the added noise due to the rebase.

https://codereview.chromium.org/2748933005/diff/1/components/password_manager...
File components/password_manager/core/browser/http_password_store_migrator.cc
(right):

https://codereview.chromium.org/2748933005/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_store_migrator.cc:72:
client_->GetPasswordStore()->RemoveSiteStats(http_origin_domain_);
On 2017/03/15 13:27:59, vasilii wrote:
> I think it'a valid to assume that HttpPasswordStoreMigrator may be destroyed
in
> ProcessPasswordStoreResults() (through ProcessMigratedForms). Therefore, this
> line should go up.
> Ideally we need a test for this if we still don't have it.

Done.

[vasilii, 2017-03-22 15:04:02]

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File components/password_manager/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/BUILD.gn:56:
"http_password_store_migrator.h",
What about http_credential_data_migrator?

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File
components/password_manager/core/browser/http_password_store_migrator_unittest.cc
(right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:26:
delete ptr;
Why not taking a pointer to the unique_ptr and resetting it?
Thus, memory leaks are impossible.

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:132:
EXPECT_CALL(store(), RemoveSiteStatsImpl(GURL(kTestHttpURL))).Times(is_hsts);
Move it before the line where we really expect the call (line 141). Same below.

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:185:
void HttpPasswordStoreMigratorTest::TestDeletion(bool is_hsts) {
Can you be more verbose? What deletion are we testing?

[jdoerrie, 2017-03-22 15:58:56]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[jdoerrie, 2017-03-22 15:59:16]

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File components/password_manager/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/BUILD.gn:56:
"http_password_store_migrator.h",
On 2017/03/22 15:04:02, vasilii wrote:
> What about http_credential_data_migrator?

I guess it depends on whether site statistics are also considered credential
data. Are they?

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File
components/password_manager/core/browser/http_password_store_migrator_unittest.cc
(right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:26:
delete ptr;
On 2017/03/22 15:04:02, vasilii wrote:
> Why not taking a pointer to the unique_ptr and resetting it?
> Thus, memory leaks are impossible.

True, didn't think of that. I only tried passing the unique_ptr directly, which
didn't work due to ACTION_P's invocation of a copy constructor
(https://github.com/google/googletest/blob/48ee8e98abc950abd8541e15550b18f8f6c...).
I solved it differently now, though. WDYT?

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:132:
EXPECT_CALL(store(), RemoveSiteStatsImpl(GURL(kTestHttpURL))).Times(is_hsts);
On 2017/03/22 15:04:02, vasilii wrote:
> Move it before the line where we really expect the call (line 141). Same
below.

Done.

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:185:
void HttpPasswordStoreMigratorTest::TestDeletion(bool is_hsts) {
On 2017/03/22 15:04:02, vasilii wrote:
> Can you be more verbose? What deletion are we testing?

Done.

[commit-bot: I haz the power, 2017-03-22 15:59:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 16:55:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 16:55:33]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-03-22 17:24:51]

lgtm

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File components/password_manager/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/BUILD.gn:56:
"http_password_store_migrator.h",
On 2017/03/22 15:59:16, jdoerrie wrote:
> On 2017/03/22 15:04:02, vasilii wrote:
> > What about http_credential_data_migrator?
> 
> I guess it depends on whether site statistics are also considered credential
> data. Are they?

It seems to me. But I don't insist :-)

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File
components/password_manager/core/browser/http_password_store_migrator_unittest.cc
(right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:26:
delete ptr;
On 2017/03/22 15:59:16, jdoerrie wrote:
> On 2017/03/22 15:04:02, vasilii wrote:
> > Why not taking a pointer to the unique_ptr and resetting it?
> > Thus, memory leaks are impossible.
> 
> True, didn't think of that. I only tried passing the unique_ptr directly,
which
> didn't work due to ACTION_P's invocation of a copy constructor
>
(https://github.com/google/googletest/blob/48ee8e98abc950abd8541e15550b18f8f6c...).
> I solved it differently now, though. WDYT?

Acknowledged.

https://codereview.chromium.org/2748933005/diff/40001/components/password_man...
File
components/password_manager/core/browser/http_password_store_migrator_unittest.cc
(right):

https://codereview.chromium.org/2748933005/diff/40001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:197:
EXPECT_CALL(consumer(), ProcessForms(_)).WillOnce(Invoke([&](Unused) {
[&migrator]

[jdoerrie, 2017-03-22 17:57:09]

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
File components/password_manager/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2748933005/diff/20001/components/password_man...
components/password_manager/core/browser/BUILD.gn:56:
"http_password_store_migrator.h",
On 2017/03/22 17:24:51, vasilii wrote:
> On 2017/03/22 15:59:16, jdoerrie wrote:
> > On 2017/03/22 15:04:02, vasilii wrote:
> > > What about http_credential_data_migrator?
> > 
> > I guess it depends on whether site statistics are also considered credential
> > data. Are they?
> 
> It seems to me. But I don't insist :-)

Acknowledged. I'll keep the name for now, but I'm open to a rename in the future
:)

https://codereview.chromium.org/2748933005/diff/40001/components/password_man...
File
components/password_manager/core/browser/http_password_store_migrator_unittest.cc
(right):

https://codereview.chromium.org/2748933005/diff/40001/components/password_man...
components/password_manager/core/browser/http_password_store_migrator_unittest.cc:197:
EXPECT_CALL(consumer(), ProcessForms(_)).WillOnce(Invoke([&](Unused) {
On 2017/03/22 17:24:51, vasilii wrote:
> [&migrator]

Done.

[jdoerrie, 2017-03-22 17:57:14]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-03-22 17:57:14]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2748933005/#ps60001
(title: "Capture explicitly")

[commit-bot: I haz the power, 2017-03-22 17:57:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 19:25:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490205434314770,
"parent_rev": "9d1abef69b6441eba82f137f2b38ef3d6935182a", "commit_rev":
"7dc7688b853c5ae80cae066021d02c591e06a1e2"}

[commit-bot: I haz the power, 2017-03-22 19:26:39]

Description was changed from

==========
Remove Obsolete HTTP SiteStatistics From PasswordStore during Migration

As a follow up to a comment in http://crrev.com/2721663002 this change
implements the removal of obsolete site statistics when a site migrated to HTTPS
and has HSTS enabled.

BUG=687968
R=vasilii@chromium.org
==========

to

==========
Remove Obsolete HTTP SiteStatistics From PasswordStore during Migration

As a follow up to a comment in http://crrev.com/2721663002 this change
implements the removal of obsolete site statistics when a site migrated to HTTPS
and has HSTS enabled.

BUG=687968
R=vasilii@chromium.org

Review-Url: https://codereview.chromium.org/2748933005
Cr-Commit-Position: refs/heads/master@{#458836}
Committed:
https://chromium.googlesource.com/chromium/src/+/7dc7688b853c5ae80cae066021d0...
==========

[commit-bot: I haz the power, 2017-03-22 19:26:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7dc7688b853c5ae80cae066021d0...