Use a ConditionVariable in place of WaitableEvent in BookmarkModelWorker....

Use a ConditionVariable in place of WaitableEvent in BookmarkModelWorker.

This is to ensure future changes to the code don't introduce harmful races
that could arise in presence of compiler optimizations due to the lack of
a barrier permitting instruction re-ordering.  The theoretical race that
exists now isn't causing any real malfunction today, but is causing
ThreadSanitizer warnings.

BUG=25915
TEST=BookmarkModelWorkerTest


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

[Timur Iskhodzhanov, 2009-12-14 17:53:42]

http://codereview.chromium.org/488012/diff/2004/5001
File chrome/browser/sync/glue/bookmark_model_worker.cc (right):

http://codereview.chromium.org/488012/diff/2004/5001#newcode75
chrome/browser/sync/glue/bookmark_model_worker.cc:75: syncapi_event_.Wait();
Are there any problems with having Wait() at the end of the loop as it was done
before?

http://codereview.chromium.org/488012/diff/2004/5001#newcode78
chrome/browser/sync/glue/bookmark_model_worker.cc:78: pending_work_->Run();
hm, don't we need to set pending_work_ to NULL?

[tim (not reviewing), 2009-12-14 19:41:32]

http://codereview.chromium.org/488012/diff/2004/5001
File chrome/browser/sync/glue/bookmark_model_worker.cc (right):

http://codereview.chromium.org/488012/diff/2004/5001#newcode75
chrome/browser/sync/glue/bookmark_model_worker.cc:75: syncapi_event_.Wait();
On 2009/12/14 17:53:42, Timur Iskhodzhanov wrote:
> Are there any problems with having Wait() at the end of the loop as it was
done
> before?

Nope, probably best to keep it that way. Moved it back.

http://codereview.chromium.org/488012/diff/2004/5001#newcode78
chrome/browser/sync/glue/bookmark_model_worker.cc:78: pending_work_->Run();
On 2009/12/14 17:53:42, Timur Iskhodzhanov wrote:
> hm, don't we need to set pending_work_ to NULL?

This is actually done by the task itself (it calls OnTaskCompleted.  It could
probably be done here but it's easiest to leave it as is.

[Timur Iskhodzhanov, 2009-12-14 19:54:58]

http://codereview.chromium.org/488012/diff/2004/5001
File chrome/browser/sync/glue/bookmark_model_worker.cc (right):

http://codereview.chromium.org/488012/diff/2004/5001#newcode78
chrome/browser/sync/glue/bookmark_model_worker.cc:78: pending_work_->Run();
Ah, ok
On 2009/12/14 19:41:32, timsteele wrote:
> On 2009/12/14 17:53:42, Timur Iskhodzhanov wrote:
> > hm, don't we need to set pending_work_ to NULL?
> 
> This is actually done by the task itself (it calls OnTaskCompleted.  It could
> probably be done here but it's easiest to leave it as is.

http://codereview.chromium.org/488012/diff/3005/3006#newcode77
chrome/browser/sync/glue/bookmark_model_worker.cc:77: if (pending_work_)
Do you need the nested while loop?

[tim (not reviewing), 2009-12-14 21:09:36]

http://codereview.chromium.org/488012/diff/3005/3006
File chrome/browser/sync/glue/bookmark_model_worker.cc (right):

http://codereview.chromium.org/488012/diff/3005/3006#newcode77
chrome/browser/sync/glue/bookmark_model_worker.cc:77: while (pending_work_ ==
NULL && !syncapi_has_shutdown_)
On 2009/12/14 19:54:59, Timur Iskhodzhanov wrote:
> Do you need the nested while loop?
I felt what we were waiting on was more explicit this way.  I guess it's not
helping :)  Changed.

[Timur Iskhodzhanov, 2009-12-14 21:17:39]

LGTM
On 2009/12/14 21:09:36, timsteele wrote:
> http://codereview.chromium.org/488012/diff/3005/3006
> File chrome/browser/sync/glue/bookmark_model_worker.cc (right):
> 
> http://codereview.chromium.org/488012/diff/3005/3006#newcode77
> chrome/browser/sync/glue/bookmark_model_worker.cc:77: while (pending_work_ ==
> NULL && !syncapi_has_shutdown_)
> On 2009/12/14 19:54:59, Timur Iskhodzhanov wrote:
> > Do you need the nested while loop?
> I felt what we were waiting on was more explicit this way.  I guess it's not
> helping :)  Changed.