sync: Implement disabling of non blocking types

sync: Implement disabling of non blocking types

Adds logic to have the model-thread NonBlockingDataTypeProcessor send
a message to the sync-thread SyncCore when it receives notifcation from
the ui-thread NonBlockingDataTypeController that it should stop syncing.
This message will allow the sync thread to stop requeting updates and
commits on behalf of the now-disabled type.

Fixes the handling of a race in the NonBlockingDataTypeProcessor.  The
race is as follows:
1. NBDTP receives a request to enable sync from the UI thread, and sends
   a connection request to the sync thread via SyncCoreProxy.
2. NBDTP receives a request to disable sync from the UI thread.  It
   updates its internal state accordingly.
3. NBDTP receives the connection OK response from the sync thread,
   which was genrated in response to its request in step 1.

Previously, the processor would wrongly set itself to the enable state
in step 3.  The fix is to use a new WeakPtrFactory and invalidate the
pointers it has issued in step 2 in order to prevent the response seen
in step 3 from being run.

Adds some more tests for this scenario and more.  The test framework had
to be made a bit more complicated in order to handle these tests, but
I think the extra complexity is worth it.  I don't know of any other
way to reliably defend against these race cases.

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

[rlarocque, 2014-05-12 18:33:20]

Hi Nicolas,

A few days ago, you made a comment that made me wonder if there was a race
condition in data-type disabling.  It turns out there was.

I've fixed it with the addition of a new WeakPtrFactory.  I think this should be
a pretty reliable fix.  An invalidated WeakPtr can't possibly affect our state. 
This makes it much easier to reason about shutdown and disable logic.

Most of the new code in this CL is in the test harness.  It's been rewritten so
it can better simulate race conditions.  

Please review.

[Nicolas Zea, 2014-05-12 19:09:09]

LGTM with nits

https://codereview.chromium.org/272323002/diff/1/components/sync_driver/non_b...
File components/sync_driver/non_blocking_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/272323002/diff/1/components/sync_driver/non_b...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:25:
scoped_refptr<base::SingleThreadTaskRunner> model_task_runner,
nit: passed scoped_refptrs and weakptr's as const ref (here and below)

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/non...
File sync/internal_api/public/non_blocking_type_processor.h (right):

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/non...
sync/internal_api/public/non_blocking_type_processor.h:85: // thread, we want to
make sure that any tasks issued from the WeakPtr we
nit: "from the weakptr we issued it" - something got dropped here I think

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_core_proxy.h (right):

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_core_proxy.h:30: // Disables sync for the given
non-blocking type on the sync core.
I find "Disables .. on the sync core" to be kind of confusing. Perhaps
"Disconnects the non-blocking type from the sync core" is clearer

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/sync_core.h
File sync/internal_api/sync_core.h (right):

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/sync_core....
sync/internal_api/sync_core.h:39: void Disconnect(ModelType type);
nit: comment about the implications of this

[rlarocque, 2014-05-12 20:02:13]

https://codereview.chromium.org/272323002/diff/1/components/sync_driver/non_b...
File components/sync_driver/non_blocking_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/272323002/diff/1/components/sync_driver/non_b...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:25:
scoped_refptr<base::SingleThreadTaskRunner> model_task_runner,
On 2014/05/12 19:09:10, Nicolas Zea wrote:
> nit: passed scoped_refptrs and weakptr's as const ref (here and below)

Done and done.

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/non...
File sync/internal_api/public/non_blocking_type_processor.h (right):

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/non...
sync/internal_api/public/non_blocking_type_processor.h:85: // thread, we want to
make sure that any tasks issued from the WeakPtr we
On 2014/05/12 19:09:10, Nicolas Zea wrote:
> nit: "from the weakptr we issued it" - something got dropped here I think

Done.

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_core_proxy.h (right):

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_core_proxy.h:30: // Disables sync for the given
non-blocking type on the sync core.
On 2014/05/12 19:09:10, Nicolas Zea wrote:
> I find "Disables .. on the sync core" to be kind of confusing. Perhaps
> "Disconnects the non-blocking type from the sync core" is clearer

Done.

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/sync_core.h
File sync/internal_api/sync_core.h (right):

https://codereview.chromium.org/272323002/diff/1/sync/internal_api/sync_core....
sync/internal_api/sync_core.h:39: void Disconnect(ModelType type);
On 2014/05/12 19:09:10, Nicolas Zea wrote:
> nit: comment about the implications of this

Done.

[rlarocque, 2014-05-12 20:02:21]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-12 20:02:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/272323002/20001

[commit-bot: I haz the power, 2014-05-12 23:46:27]

Change committed as 269916