[Sync] Fix thread-ordering-dependent NULL dereference in NewNonFrontendDTC

[Sync] Fix thread-ordering-dependent NULL dereference in NewNonFrontendDTC

Re-enable migration tests that were affected.

Rewrite autofill DTC tests.

Add basic unit test for SharedChangeProcessor.

Fix bug in SharedChangeProcessor where its GenericChangeProcessor could be deleted on the wrong thread.

Renamed some tests to have 'Sync' in their names.

BUG=107743, 111552
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120698

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120942

[akalin, 2012-02-03 00:57:37]

+zea for review

You'll love this solution!

[Nicolas Zea, 2012-02-03 18:54:43]

Some comments.

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
File chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc (right):

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:76: // by
|shared_change_processor_|, which is guaranteed to have been Disconnected
shared_change_processor_backend_ref_

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:104: //
Connect |shared_change_processor_| to the syncer and |local_service_|.
here too

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:134:
shared_change_processor_backend_ref_->GetSyncDataForType(
move to line above?

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:152: //
pointer, but the shared_change_processor_ protects us from attempting to
shared_change_processor_backend_ref_

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:184:
ClearSharedChangeProcessor();
Is there any reason to keep the backend shared change processor ref after
StartDoneImpl is called (even for a success)? It's only used in StartAssociation
right?

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
File chrome/browser/sync/glue/new_non_frontend_data_type_controller.h (right):

https://chromiumcodereview.appspot.com/9307079/diff/1013/chrome/browser/sync/...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.h:99:
scoped_refptr<SharedChangeProcessor> shared_change_processor_backend_ref_;
I'm still trying to decide whether I like this approach better or not. I guess
it makes sense that we would use two separate references, one for each thread.
But, it seems like it would be simpler if we let the task itself handle
maintaining the second reference. Was there some other reason as well you took
this approach?

[akalin, 2012-02-03 23:09:03]

Addressed your main comment, and all other comments were rendered moot, I think.

Please take another look.

http://codereview.chromium.org/9307079/diff/1013/chrome/browser/sync/glue/new...
File chrome/browser/sync/glue/new_non_frontend_data_type_controller.h (right):

http://codereview.chromium.org/9307079/diff/1013/chrome/browser/sync/glue/new...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.h:99:
scoped_refptr<SharedChangeProcessor> shared_change_processor_backend_ref_;
On 2012/02/03 18:54:43, nzea wrote:
> I'm still trying to decide whether I like this approach better or not. I guess
> it makes sense that we would use two separate references, one for each thread.
> But, it seems like it would be simpler if we let the task itself handle
> maintaining the second reference. Was there some other reason as well you took
> this approach?

I think I decided against that because I got confused by inheriting from
NonFrontendDTC's methods.  (since we can't just define
StartAssociationAsync(args), since data types may call StartAssociationAsync()
directly if model startup is delayed.)

But I think if I make StartAssociationAsync() virtual, and override that in
NewNonFrontendDTC to post the task with the SCP ref, that works out, and removes
the need for the two refs.  I like that better.  Done.

[Nicolas Zea, 2012-02-03 23:52:09]

LGTM once trybots like it (note that AutofillDataTypeControllerUnitTests mocks
the old StartAssociation() method)

http://codereview.chromium.org/9307079/diff/8/chrome/browser/sync/glue/new_no...
File chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc (right):

http://codereview.chromium.org/9307079/diff/8/chrome/browser/sync/glue/new_no...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:7: #include
"base/compiler_specific.h"
Is this still necessary?

http://codereview.chromium.org/9307079/diff/8/chrome/browser/sync/glue/new_no...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:247: //
|shared_change_processor_| should not be NULL.
DCHECK this is only on UI thread.

[akalin, 2012-02-06 19:31:35]

Also, I basically rewrote the autofill tests (to use a fake instead of a mock).

Committing on green trybots.

https://chromiumcodereview.appspot.com/9307079/diff/8/chrome/browser/sync/glu...
File chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc (right):

https://chromiumcodereview.appspot.com/9307079/diff/8/chrome/browser/sync/glu...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:7: #include
"base/compiler_specific.h"
On 2012/02/03 23:52:09, nzea wrote:
> Is this still necessary?

Done.

https://chromiumcodereview.appspot.com/9307079/diff/8/chrome/browser/sync/glu...
chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc:247: //
|shared_change_processor_| should not be NULL.
On 2012/02/03 23:52:09, nzea wrote:
> DCHECK this is only on UI thread.

Done.

[commit-bot: I haz the power, 2012-02-06 21:29:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9307079/8001

[commit-bot: I haz the power, 2012-02-07 01:59:44]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120668, job name
was 9307079-8001 (retry) (previous was lost) (previous was lost) (retry).

[akalin, 2012-02-07 06:16:45]

Fixed bug in SyncableService and added test.  recommitting soon.

[commit-bot: I haz the power, 2012-02-07 06:17:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9307079/18001

[commit-bot: I haz the power, 2012-02-07 07:21:42]

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

[commit-bot: I haz the power, 2012-02-08 00:23:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9307079/25001