Fix WaitForTypeEncryption race condition

Fix WaitForTypeEncryption race condition

IsTypeEncrypted() and IsSynced() are both changed when by a
WriteTransaction on the syncer thread.  When IsTypeEncrytped() gets
toggled from false to true, that should imply that IsSynced() is false
(at least until a sync cycle comes along and commits the changes to
the server).

Sometimes the thread executing this code will see the value of IsSynced()
before the transaction (true) and IsTypeEncrypted after the transaction
(true).  This is bad, because we end up inside the if branch when
IsSynced() is false, which is not the intent of this code.

A proper fix might involve wrapping the entire if condition in a
ReadTrasaction.  An easier fix is to swap the order in which the
conditions are evaluated, and that's what this commit does.

BUG=95619
TEST=

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

[rlarocque, 2011-09-08 17:39:54]

Looking for review of this change.  It looks like a no-op, but it actually
prevents a race condition.  See the description for more info.  

There are some trybot failures but they have nothing to do with the code I've
modified.  I think it's safe to ignore them.

[Nicolas Zea, 2011-09-08 17:50:57]

LGTM.

[commit-bot: I haz the power, 2011-09-08 17:52:54]

Presubmit check for 7847010-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
You might be calling functions intended only for testing from
production code. Please verify that the following usages are OK,
and email joi@chromium.org if you are seeing false positives:
  chrome/browser/sync/profile_sync_service_harness.cc:227
    service_->GetBackendMigratorForTest(); \
  chrome/browser/sync/profile_sync_service_harness.cc:432
    service()->GetBackendMigratorForTest()-> \
  chrome/browser/sync/profile_sync_service_harness.cc:433
    GetPendingMigrationTypesForTest(); \
  chrome/browser/sync/profile_sync_service_harness.cc:750
    service()->GetBackendMigratorForTest();

[Nicolas Zea, 2011-09-08 17:59:09]

It's a false positive, I would just commit manually.

On Thu, Sep 8, 2011 at 10:52 AM, <commit-bot@chromium.org> wrote:

> Presubmit check for 7847010-1 failed and returned exit status 1.
>
> Running presubmit commit checks ...
>
> ** Presubmit Messages **
> If this change requires manual test instructions to QA team, add
> TEST=[instructions].
>
> ** Presubmit Warnings **
> You might be calling functions intended only for testing from
> production code. Please verify that the following usages are OK,
> and email joi@chromium.org if you are seeing false positives:
>  chrome/browser/sync/profile_**sync_service_harness.cc:227
>    service_->**GetBackendMigratorForTest(); \
>  chrome/browser/sync/profile_**sync_service_harness.cc:432
>    service()->**GetBackendMigratorForTest()-> \
>  chrome/browser/sync/profile_**sync_service_harness.cc:433
>    GetPendingMigrationTypesForTes**t(); \
>  chrome/browser/sync/profile_**sync_service_harness.cc:750
>    service()->**GetBackendMigratorForTest();
>
>
>
>
>
http://codereview.chromium.**org/7847010/<http://codereview.chromium.org/7847...
>

[rlarocque1, 2011-09-08 18:01:26]

Already done.

On Thu, Sep 8, 2011 at 10:59, Nicolas Zea <zea@chromium.org> wrote:

> It's a false positive, I would just commit manually.
>
>
> On Thu, Sep 8, 2011 at 10:52 AM, <commit-bot@chromium.org> wrote:
>
>> Presubmit check for 7847010-1 failed and returned exit status 1.
>>
>> Running presubmit commit checks ...
>>
>> ** Presubmit Messages **
>> If this change requires manual test instructions to QA team, add
>> TEST=[instructions].
>>
>> ** Presubmit Warnings **
>> You might be calling functions intended only for testing from
>> production code. Please verify that the following usages are OK,
>> and email joi@chromium.org if you are seeing false positives:
>>  chrome/browser/sync/profile_**sync_service_harness.cc:227
>>    service_->**GetBackendMigratorForTest(); \
>>  chrome/browser/sync/profile_**sync_service_harness.cc:432
>>    service()->**GetBackendMigratorForTest()-> \
>>  chrome/browser/sync/profile_**sync_service_harness.cc:433
>>    GetPendingMigrationTypesForTes**t(); \
>>  chrome/browser/sync/profile_**sync_service_harness.cc:750
>>    service()->**GetBackendMigratorForTest();
>>
>>
>>
>>
>>
http://codereview.chromium.**org/7847010/<http://codereview.chromium.org/7847...
>>
>
>

[akalin, 2011-09-22 18:12:05]

You already check this in, but can you address the comments in a separate CL?

http://codereview.chromium.org/7847010/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7847010/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service_harness.cc:1005: if
(IsTypeEncrypted(type) &&
Can you add a comment for this?  This can easily be unknowingly reverted.

Also, maybe a TODO to figure out a better way to do this.

[rlarocque, 2011-09-22 19:05:57]

You bring up a good point.  Thanks for reminding me about this.  

I wasn't sure if it was possible to reuse this code review for the new commit,
so I've started a new one.  See http://codereview.chromium.org/7995015.

http://codereview.chromium.org/7847010/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7847010/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service_harness.cc:1005: if
(IsTypeEncrypted(type) &&
On 2011/09/22 18:12:05, akalin wrote:
> Can you add a comment for this?  This can easily be unknowingly reverted.
> 
> Also, maybe a TODO to figure out a better way to do this.

Done.