The AutofillProfileSyncableService's lifetime should be managed by the WebDataService

The AutofillProfileSyncableService's lifetime should be managed by the WebDataService

BUG=96922, 96363
TEST=sh tools/valgrind/chrome_tests.sh -t unit_tests --gtest_filter=ProfileSyncServiceAutofillTest.HasProfileEmptySync

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105199

[GeorgeY, 2011-10-06 22:48:09]

lgtm
BTW, are the memory leaks fixed?

[Ilya Sherman, 2011-10-06 22:49:38]

On 2011/10/06 22:48:09, GeorgeY wrote:
> lgtm
> BTW, are the memory leaks fixed?

Yes, this fixes the memory leaks.  At least, it fixes one; I'm hoping that there
was just the one root cause for all the suppressions.

[Ilya Sherman, 2011-10-08 05:14:10]

Fred, could you take a look at this from the Sync side?

[akalin, 2011-10-10 21:58:57]

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/sync/abstract...
File chrome/browser/sync/abstract_profile_sync_service_test.cc (right):

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/sync/abstract...
chrome/browser/sync/abstract_profile_sync_service_test.cc:42: bool
ProfileSyncServiceTestHelper::CreateRoot(ModelType model_type,
revert these diffs please (as far as I can tell, they're just changing
indentation)

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
chrome/browser/webdata/autofill_profile_syncable_service.h:68: const
WebDatabase* web_database() { return web_database_; }
where are these used?

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
chrome/browser/webdata/autofill_table.cc:378: DCHECK_EQ(web_database,
autofill_profile_syncable_service_->web_database());
this seems kinda gross.  It seems to me that WebDatabase or WebDataService would
be better owners of the syncable service, if this is needed.

[Ilya Sherman, 2011-10-10 22:33:13]

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/sync/abstract...
File chrome/browser/sync/abstract_profile_sync_service_test.cc (right):

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/sync/abstract...
chrome/browser/sync/abstract_profile_sync_service_test.cc:42: bool
ProfileSyncServiceTestHelper::CreateRoot(ModelType model_type,
On 2011/10/10 21:58:57, akalin wrote:
> revert these diffs please (as far as I can tell, they're just changing
> indentation)

Done.

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
chrome/browser/webdata/autofill_profile_syncable_service.h:68: const
WebDatabase* web_database() { return web_database_; }
On 2011/10/10 21:58:57, akalin wrote:
> where are these used?

I added them for the sake of the DCHECK's in the autofill_table code.

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/8184001/diff/4001/chrome/browser/webdata/autof...
chrome/browser/webdata/autofill_table.cc:378: DCHECK_EQ(web_database,
autofill_profile_syncable_service_->web_database());
On 2011/10/10 21:58:57, akalin wrote:
> this seems kinda gross.  It seems to me that WebDatabase or WebDataService
would
> be better owners of the syncable service, if this is needed.

This would still be "needed" even if the WebDatabase were the owner -- it would
just become

DCHECK_EQ(this, autofill_profile_syncable_service_->web_database());

As far as I understand, GetSyncableService() should be called exactly once per
launch of a Profile.  That's essentially what these DCHECKs are enforcing,
though they allow a bit more flexibility.  If the additional flexibility is
undesirable, we could just

DCHECK(!autofill_profile_syncable_service_.get());

at the top of the function, and get rid of the if-stmt.

[akalin, 2011-10-10 22:41:36]

> This would still be "needed" even if the WebDatabase were the owner -- it
would
> just become
> 
> DCHECK_EQ(this, autofill_profile_syncable_service_->web_database());

True, but the DCHECK would be needed less, given that it would be trivial to
verify that the service's web DB is 'this' (since it's a private variable).
> 
> As far as I understand, GetSyncableService() should be called exactly once per
> launch of a Profile.  That's essentially what these DCHECKs are enforcing,
> though they allow a bit more flexibility.  If the additional flexibility is
> undesirable, we could just
> 
> DCHECK(!autofill_profile_syncable_service_.get());
> 
> at the top of the function, and get rid of the if-stmt.

Basically, I'm just objecting to the need to pass the web database (which
wouldn't be needed if the web database itself owned it) and profile and having
to DCHECK against that.  It's just a code smell that perhaps the wrong thing is
owning the object.

If the WebDataService owned the object, then it could simply pass the
profile/web_database to the service, and it would be easily verifiable that
those would always be in sync.

[Ilya Sherman, 2011-10-11 04:08:44]

+David, to sanity-check the WebDataService changes

On 2011/10/10 22:41:36, akalin wrote:
> If the WebDataService owned the object, then it could simply pass the
> profile/web_database to the service, and it would be easily verifiable that
> those would always be in sync.

Ok, I'm convinced.  Updated the ownership model.

[akalin, 2011-10-11 21:24:38]

two nits, one question

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_data_service.cc:659: if
(autofill_profile_syncable_service_) {
delete NULL works, so no need for if guard

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_data_service.cc:1382: return
autofill_profile_syncable_service_;
Will this be called only on the DB thread?  If so, DCHECK that, also DCHECK that
apss_ is initialized.

If not, well, accessing apss_ is technically not safe, but I guess it could
work...

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_data_service.h:518: // in this table.
Add comment saying that the returned APSS is owned by this object

[Ilya Sherman, 2011-10-11 22:17:34]

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_data_service.cc:659: if
(autofill_profile_syncable_service_) {
On 2011/10/11 21:24:38, akalin wrote:
> delete NULL works, so no need for if guard

Done.

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_data_service.cc:1382: return
autofill_profile_syncable_service_;
On 2011/10/11 21:24:38, akalin wrote:
> Will this be called only on the DB thread?  If so, DCHECK that, also DCHECK
that
> apss_ is initialized.
> 
> If not, well, accessing apss_ is technically not safe, but I guess it could
> work...

Done.

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8184001/diff/8003/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_data_service.h:518: // in this table.
On 2011/10/11 21:24:38, akalin wrote:
> Add comment saying that the returned APSS is owned by this object

Done.

[akalin, 2011-10-11 22:20:11]

LGTM

Make sure to update the CL description and subject (AutofillTable ->
WebDataService)

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:188: return
autofill_profile_syncable_service_;
add checks for on db thread, non-null service here

[Ilya Sherman, 2011-10-11 22:35:29]

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:188: return
autofill_profile_syncable_service_;
On 2011/10/11 22:20:12, akalin wrote:
> add checks for on db thread, non-null service here

Done.

[dhollowa, 2011-10-11 22:48:34]

WDS LGTM.  w/ nit.

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:637:
autofill_profile_syncable_service_ = new AutofillProfileSyncableService(this);
A DCHECK(!autofill_profile_syncable_service_) here would be good.

[Ilya Sherman, 2011-10-11 23:07:11]

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8184001/diff/16002/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:637:
autofill_profile_syncable_service_ = new AutofillProfileSyncableService(this);
On 2011/10/11 22:48:34, dhollowa wrote:
> A DCHECK(!autofill_profile_syncable_service_) here would be good.

Done.

[commit-bot: I haz the power, 2011-10-12 00:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8184001/15009

[commit-bot: I haz the power, 2011-10-12 00:07:06]

Can't apply patch for file tools/valgrind/memcheck/suppressions.txt.
While running patch -p1 --forward --force;
patching file tools/valgrind/memcheck/suppressions.txt
Hunk #1 FAILED at 4451.
1 out of 1 hunk FAILED -- saving rejects to file
tools/valgrind/memcheck/suppressions.txt.rej

[commit-bot: I haz the power, 2011-10-12 00:41:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8184001/14008

[commit-bot: I haz the power, 2011-10-12 04:55:50]

Change committed as 105016

[commit-bot: I haz the power, 2011-10-12 18:26:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8184001/21001

[commit-bot: I haz the power, 2011-10-12 19:27:46]

Try job failure for 8184001-21001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-12 19:46:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8184001/21011

[commit-bot: I haz the power, 2011-10-12 23:42:00]

Change committed as 105199