[Sync] Ensure all accesses to SyncInternal's observers are thread safe.

[Sync] Ensure all accesses to SyncInternal's observers are thread safe.

BUG=72913
TEST=unit tests with tsan


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

[Nicolas Zea, 2011-06-08 20:31:21]

Initial work

[tim (not reviewing), 2011-06-08 20:51:24]

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:1533: // from any thread and added
to/removed from on the core thread.
Are the two threads the core and syncer thread?  We can't ensure the syncer
thread isn't running when init / shutdown add/remove is happening?
I think it used to work that way,  but something may well have changed and this
isn't possible...

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:1609:
FOR_EACH_OBSERVER(SyncManager::Observer, temp_observers, function); \
In the remove case (above), isn't it possible that we now wind up notifying a
garbage pointer?

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2460: if (!HaveObservers() <= 0) {
don't think this is what you meant

[Nicolas Zea, 2011-06-08 21:01:29]

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:1533: // from any thread and added
to/removed from on the core thread.
On 2011/06/08 20:51:24, timsteele wrote:
> Are the two threads the core and syncer thread?  We can't ensure the syncer
> thread isn't running when init / shutdown add/remove is happening?
> I think it used to work that way,  but something may well have changed and
this
> isn't possible...

Discussed offline, it's primarily core thread (where add  and remove are called
from) and the datatype threads.

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:1609:
FOR_EACH_OBSERVER(SyncManager::Observer, temp_observers, function); \
On 2011/06/08 20:51:24, timsteele wrote:
> In the remove case (above), isn't it possible that we now wind up notifying a
> garbage pointer?

Theoretically yes, but on the one hand, this vulnerability was already there
before the locking was introduced, since the observerlist is not threadsafe, and
on the other, the sync backend host shuts down the syncapi before it removes
itself as an observer, so I think that case should never happen.

http://codereview.chromium.org/7044059/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2460: if (!HaveObservers() <= 0) {
On 2011/06/08 20:51:24, timsteele wrote:
> don't think this is what you meant

Done.

[akalin, 2011-06-08 22:39:37]

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1240: bool HaveObservers();
make const

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1534: base::Lock observers_lock_;
make mutable

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1598: // while the lock is released.
Add a TODO explaining the problem with notifying garbage pointers, why it
doesn't happen often (only happens when you close last about:sync window in the
middle of a notify), but that we should fix it.

Also file a bug and link it, for good measure.  (Feel free to assign to me.)

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1599: #define
NOTIFY_SYNCMANAGER_OBSERVERS(function)                          \
I think it would be cleaner to define a utility function
CopyObserverList(ObserverList* observers) that would copy observers_ into
observers under lock and use that.  That way, callers would just add:

ObserverList<> observers;
CopyObserverList(&observers);
FOR_EACH(..., observers, ...);

Cleaner than having a big macro.

[Nicolas Zea, 2011-06-08 23:10:23]

PTAL

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1240: bool HaveObservers();
On 2011/06/08 22:39:38, akalin wrote:
> make const

Done.

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1534: base::Lock observers_lock_;
On 2011/06/08 22:39:38, akalin wrote:
> make mutable

Done.

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1598: // while the lock is released.
On 2011/06/08 22:39:38, akalin wrote:
> Add a TODO explaining the problem with notifying garbage pointers, why it
> doesn't happen often (only happens when you close last about:sync window in
the
> middle of a notify), but that we should fix it.
> 
> Also file a bug and link it, for good measure.  (Feel free to assign to me.)

Done.

http://codereview.chromium.org/7044059/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1599: #define
NOTIFY_SYNCMANAGER_OBSERVERS(function)                          \
On 2011/06/08 22:39:38, akalin wrote:
> I think it would be cleaner to define a utility function
> CopyObserverList(ObserverList* observers) that would copy observers_ into
> observers under lock and use that.  That way, callers would just add:
> 
> ObserverList<> observers;
> CopyObserverList(&observers);
> FOR_EACH(..., observers, ...);
> 
> Cleaner than having a big macro.

Done.

[akalin, 2011-06-08 23:14:40]

LGTM

http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1241: void
CopyObservers(ObserverList<SyncManager::Observer>* observers_copy);
this should be const, too

[Nicolas Zea, 2011-06-08 23:25:33]

Committing if this is still okay.

http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1241: void
CopyObservers(ObserverList<SyncManager::Observer>* observers_copy);
On 2011/06/08 23:14:41, akalin wrote:
> this should be const, too

ObserverList does not have a const iterator, so this method can't be made const.
Not sure if there is a reason for that lack of a const iterator or not.

[akalin, 2011-06-08 23:43:03]

On 2011/06/08 23:25:33, nzea wrote:
> Committing if this is still okay.
> 
>
http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
> File chrome/browser/sync/engine/syncapi.cc (right):
> 
>
http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
> chrome/browser/sync/engine/syncapi.cc:1241: void
> CopyObservers(ObserverList<SyncManager::Observer>* observers_copy);
> On 2011/06/08 23:14:41, akalin wrote:
> > this should be const, too
> 
> ObserverList does not have a const iterator, so this method can't be made
const.
> Not sure if there is a reason for that lack of a const iterator or not.

Okay, that's fine.  Still LGTM.

[commit-bot: I haz the power, 2011-06-09 01:58:40]

Change committed as 88472

[tim (not reviewing), 2011-06-09 02:02:44]

On 2011/06/08 23:43:03, akalin wrote:
> On 2011/06/08 23:25:33, nzea wrote:
> > Committing if this is still okay.
> > 
> >
>
http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
> > File chrome/browser/sync/engine/syncapi.cc (right):
> > 
> >
>
http://codereview.chromium.org/7044059/diff/5003/chrome/browser/sync/engine/s...
> > chrome/browser/sync/engine/syncapi.cc:1241: void
> > CopyObservers(ObserverList<SyncManager::Observer>* observers_copy);
> > On 2011/06/08 23:14:41, akalin wrote:
> > > this should be const, too
> > 
> > ObserverList does not have a const iterator, so this method can't be made
> const.
> > Not sure if there is a reason for that lack of a const iterator or not.
> 
> Okay, that's fine.  Still LGTM.

From a 'clean' perspective I actually thought the new macro was nice as it
resulted in less boilerplate :)  And maybe made the code less prone to errors
forgetting the 'CopyObservers' bit when adding a new observe callsite or
copy/pasting.  But either works.

I'm still a bit puzzled where we end up racing between notifying, and
add/remove.  I'll read up :)