[Sync] Try to fix race conditions in CookieJarMismatch.

[Sync] Try to fix race conditions in CookieJarMismatch.

When we navigate to a URL in sync integration tests, sometimes, usually
when this is the first navigation, we see two separate commits. The
first has most of the data correctly set, but lacks a title. Later on
the title is updated through a second commit. This test case has
previously been updated in an attempt to resilient to this quirk, but
it was not enough.

In the flake in attached bug, what appeared to have happened was that
the first navigation created two commits, but only the first one was
needed to satisfy the WaitForURLOnServer check. Then we call
OnGaiaAccountsInCookieUpdated but the affect is asynchronous. Before
the posted task to update cookie information reaches the sync thread,
but after the second histogram checker is created, a second commit from
navigating to kURL1 executes and we get histogram data that we're not
expecting, leading to a failure.

I was not able to repro locally, so it is unclear if both of the
following fixes were actually needed. Regardless, this should make us
significantly more resilient to races and flakiness.

First, changed the way blocking for sessions changes works, to only
consider data that has a title set. When used in a way, this should
force both commits to happen (if there are going to be two), and less
likely the later will leak out.

Second, added a callback to the mechanism that sets cookie information.
This allows us to truly block until the sync thread has been updated.
It is a significant amount of boilerplate for a very simple concept,
unfortunately. 

BUG=689662
Review-Url: https://codereview.chromium.org/2751333007
Cr-Commit-Position: refs/heads/master@{#458773}
Committed: https://chromium.googlesource.com/chromium/src/+/4d5e4f5dc10d0203717fbfcce22e546876b793ff

[skym, 2017-03-17 23:28:34]

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

[commit-bot: I haz the power, 2017-03-17 23:29:05]

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

[skym, 2017-03-17 23:33:44]

Description was changed from

==========
[Sync] Try to fix race conditions in CookieJarMismatch.

BUG=689662
==========

to

==========
[Sync] Try to fix race conditions in CookieJarMismatch.

When we navigate to a URL in sync integration tests, sometimes, usually
when this is the first navigation, we see two separate commits. The
first has most of the data correctly set, but lacks a title. Later on
the title is updated through a second commit. This test case has
previously been updated in an attempt to resilient to this quirk, but
it was not enough.

In the flake in attached bug, what appeared to have happened was that
the first navigation created two commits, but only the first one was
needed to satisfy the WaitForURLOnServer check. Then we call
OnGaiaAccountsInCookieUpdated but the affect is asynchronous. Before
the posted task to update cookie information reaches the sync thread,
but after the second histogram checker is created, a second commit from
navigating to kURL1 executes and we get histogram data that we're not
expecting, leading to a failure.

I was not able to repro locally, so it is unclear if both of the
following fixes were actually needed. Regardless, this should make us
significantly more resilient to races and flakiness.

First, changed the way blocking for sessions changes works, to only
consider data that has a title set. When used in a way, this should
force both commits to happen (if there are going to be two), and less
likely the later will leak out.

Second, added a callback to the mechanism that sets cookie information.
This allows us to truly block until the sync thread has been updated.
It is a significant amount of boilerplate for a very simple concept,
unfortunately. 

BUG=689662
==========

[skym, 2017-03-17 23:33:44]

skym@chromium.org changed reviewers:
+ pavely@chromium.org

[skym, 2017-03-17 23:33:51]

PTAL

[commit-bot: I haz the power, 2017-03-18 02:33:31]

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

[commit-bot: I haz the power, 2017-03-18 02:33:31]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-03-20 04:32:02]

https://codereview.chromium.org/2751333007/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2751333007/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:379:
run_loop.Run();
Waiting for values to arrive on sync thread doesn't guarantee they will get
recorded in histogram before HistogramTester takes snapshot. What if you called
OnGaiaAccountsInCookieUpdatedWithCallback inside the block, after
HistogramTester? This will allow you to avoid wiring callback to SBHC and back.
I think you would need less strict function to verify histogram sample presence.

[pavely, 2017-03-22 15:32:01]

lgtm

[skym, 2017-03-22 15:49:42]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-03-22 15:50:04]

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:35:59]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490197782810080, "parent_rev":
"96bce3e41fb2611184a4616cebc614bc037fa4c8", "commit_rev":
"4d5e4f5dc10d0203717fbfcce22e546876b793ff"}

[commit-bot: I haz the power, 2017-03-22 16:36:45]

Description was changed from

==========
[Sync] Try to fix race conditions in CookieJarMismatch.

When we navigate to a URL in sync integration tests, sometimes, usually
when this is the first navigation, we see two separate commits. The
first has most of the data correctly set, but lacks a title. Later on
the title is updated through a second commit. This test case has
previously been updated in an attempt to resilient to this quirk, but
it was not enough.

In the flake in attached bug, what appeared to have happened was that
the first navigation created two commits, but only the first one was
needed to satisfy the WaitForURLOnServer check. Then we call
OnGaiaAccountsInCookieUpdated but the affect is asynchronous. Before
the posted task to update cookie information reaches the sync thread,
but after the second histogram checker is created, a second commit from
navigating to kURL1 executes and we get histogram data that we're not
expecting, leading to a failure.

I was not able to repro locally, so it is unclear if both of the
following fixes were actually needed. Regardless, this should make us
significantly more resilient to races and flakiness.

First, changed the way blocking for sessions changes works, to only
consider data that has a title set. When used in a way, this should
force both commits to happen (if there are going to be two), and less
likely the later will leak out.

Second, added a callback to the mechanism that sets cookie information.
This allows us to truly block until the sync thread has been updated.
It is a significant amount of boilerplate for a very simple concept,
unfortunately. 

BUG=689662
==========

to

==========
[Sync] Try to fix race conditions in CookieJarMismatch.

When we navigate to a URL in sync integration tests, sometimes, usually
when this is the first navigation, we see two separate commits. The
first has most of the data correctly set, but lacks a title. Later on
the title is updated through a second commit. This test case has
previously been updated in an attempt to resilient to this quirk, but
it was not enough.

In the flake in attached bug, what appeared to have happened was that
the first navigation created two commits, but only the first one was
needed to satisfy the WaitForURLOnServer check. Then we call
OnGaiaAccountsInCookieUpdated but the affect is asynchronous. Before
the posted task to update cookie information reaches the sync thread,
but after the second histogram checker is created, a second commit from
navigating to kURL1 executes and we get histogram data that we're not
expecting, leading to a failure.

I was not able to repro locally, so it is unclear if both of the
following fixes were actually needed. Regardless, this should make us
significantly more resilient to races and flakiness.

First, changed the way blocking for sessions changes works, to only
consider data that has a title set. When used in a way, this should
force both commits to happen (if there are going to be two), and less
likely the later will leak out.

Second, added a callback to the mechanism that sets cookie information.
This allows us to truly block until the sync thread has been updated.
It is a significant amount of boilerplate for a very simple concept,
unfortunately. 

BUG=689662

Review-Url: https://codereview.chromium.org/2751333007
Cr-Commit-Position: refs/heads/master@{#458773}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d5e4f5dc10d0203717fbfcce22e...
==========

[commit-bot: I haz the power, 2017-03-22 16:36:47]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/4d5e4f5dc10d0203717fbfcce22e...