[Sync] Fix use of ObserverList by multiple threads.

chrome/browser/sync/syncable/syncable.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/syncable/syncable.h"
 
 #include "build/build_config.h"
 
 #include <sys/stat.h>
 #if defined(OS_POSIX)
@@ skipping 366 matching lines @@
 
 void Directory::Kernel::AddRef() {
   base::subtle::NoBarrier_AtomicIncrement(&refcount, 1);
 }
 
 void Directory::Kernel::Release() {
   if (!base::subtle::NoBarrier_AtomicIncrement(&refcount, -1))
     delete this;
 }
 
+void Directory::Kernel::AddChangeListener(
+    DirectoryChangeListener* listener) {
+  base::AutoLock lock(change_listeners_lock_);
+  change_listeners_.AddObserver(listener);
+}
+
+void Directory::Kernel::RemoveChangeListener(
+    DirectoryChangeListener* listener) {
+  base::AutoLock lock(change_listeners_lock_);
+  change_listeners_.RemoveObserver(listener);
+}
+
+// Note: it is possible that a listener will remove itself after we
+// have made a copy, but before the copy is consumed. This could
+// theoretically result in accessing a garbage pointer, but can only
+// occur when an about:sync window is closed in the middle of a
+// notification.  See crbug.com/85481.
+void Directory::Kernel::CopyChangeListeners(
+    ObserverList<DirectoryChangeListener>* change_listeners) {
+  DCHECK_EQ(0U, change_listeners->size());
+  base::AutoLock lock(change_listeners_lock_);
+  ObserverListBase<DirectoryChangeListener>::Iterator it(change_listeners_);
+  DirectoryChangeListener* obs;
+  while ((obs = it.GetNext()) != NULL)
+    change_listeners->AddObserver(obs);
+}
+
 Directory::Kernel::~Kernel() {
   CHECK_EQ(0, refcount);
   delete channel;
   delete unsynced_metahandles;
   delete unapplied_update_metahandles;
   delete dirty_metahandles;
   delete metahandles_to_purge;
   delete parent_id_child_index;
   delete client_tag_index;
   delete ids_index;
@@ skipping 756 matching lines @@
     if (elapsed_ms > max_ms) {
       VLOG(1) << "Cutting Invariant check short after " << elapsed_ms
               << "ms. Processed " << entries_done << "/" << handles.size()
               << " entries";
       return;
     }
   }
 }
 
 void Directory::AddChangeListener(DirectoryChangeListener* listener) {
-  kernel_->change_listeners_.AddObserver(listener);
+  kernel_->AddChangeListener(listener);
 }
 
 void Directory::RemoveChangeListener(DirectoryChangeListener* listener) {
-  kernel_->change_listeners_.RemoveObserver(listener);
+  kernel_->RemoveChangeListener(listener);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ScopedKernelLock
 
 ScopedKernelLock::ScopedKernelLock(const Directory* dir)
   :  scoped_lock_(dir->kernel_->mutex), dir_(const_cast<Directory*>(dir)) {
 }
 
 ///////////////////////////////////////////////////////////////////////////
@@ skipping 53 matching lines @@
   const base::TimeDelta elapsed = base::TimeTicks::Now() - time_acquired_;
   if (LOG_IS_ON(INFO) &&
       (1 <= logging::GetVlogLevelHelper(
           source_file_, ::strlen(source_file_))) &&
       (elapsed.InMilliseconds() > 50)) {
     logging::LogMessage(source_file_, line_, logging::LOG_INFO).stream()
         << name_ << " transaction completed in " << elapsed.InSecondsF()
         << " seconds.";
   }
 
+  ObserverList<DirectoryChangeListener> change_listeners;
+  dirkernel_->CopyChangeListeners(&change_listeners);
+
   if (NULL == originals.get() || originals->empty() ||
-      (dirkernel_->change_listeners_.size() == 0)) {
+      (change_listeners.size() == 0)) {
     dirkernel_->transaction_mutex.Release();
     return false;
   }
 
   if (writer_ == syncable::SYNCAPI) {
     FOR_EACH_OBSERVER(DirectoryChangeListener,
-                      dirkernel_->change_listeners_,
+                      change_listeners,
                       HandleCalculateChangesChangeEventFromSyncApi(
                           *originals.get(),
                           writer_,
                           this));
   } else {
     FOR_EACH_OBSERVER(DirectoryChangeListener,
-                      dirkernel_->change_listeners_,
+                      change_listeners,
                       HandleCalculateChangesChangeEventFromSyncer(
                           *originals.get(),
                           writer_,
                           this));
   }
 
   // Set |*models_with_changes| to the union of the return values of
   // the HandleTransactionEndingChangeEvent call to each
   // DirectoryChangeListener.
   {
-    ObserverList<DirectoryChangeListener>::Iterator it(
-        dirkernel_->change_listeners_);
+    ObserverList<DirectoryChangeListener>::Iterator it(change_listeners);
     DirectoryChangeListener* obs = NULL;
     while ((obs = it.GetNext()) != NULL) {
       *models_with_changes |= obs->HandleTransactionEndingChangeEvent(this);
     }
   };
 
   // Release the transaction. Note, once the transaction is released this thread
   // can be interrupted by another that was waiting for the transaction,
   // resulting in this code possibly being interrupted with another thread
   // performing following the same code path. From this point foward, only
   // local state can be touched.
   dirkernel_->transaction_mutex.Release();
   return true;
 }
 
 void BaseTransaction::NotifyTransactionComplete(
     ModelTypeBitSet models_with_changes) {
-    FOR_EACH_OBSERVER(DirectoryChangeListener,
-                      dirkernel_->change_listeners_,
-                      HandleTransactionCompleteChangeEvent(
-                          models_with_changes));
+  ObserverList<DirectoryChangeListener> change_listeners;
+  dirkernel_->CopyChangeListeners(&change_listeners);
+  FOR_EACH_OBSERVER(DirectoryChangeListener,
+                    change_listeners,
+                    HandleTransactionCompleteChangeEvent(
+                        models_with_changes));
 }
 
 ReadTransaction::ReadTransaction(Directory* directory, const char* file,
                                  int line)
   : BaseTransaction(directory, "Read", file, line, INVALID) {
 }
 
 ReadTransaction::ReadTransaction(const ScopedDirLookup& scoped_dir,
                                  const char* file, int line)
   : BaseTransaction(scoped_dir.operator -> (), "Read", file, line, INVALID) {
@@ skipping 701 matching lines @@
   CHECK(result);
   for (iterator i = GetParentChildIndexLowerBound(lock, parent_id),
            end = GetParentChildIndexUpperBound(lock, parent_id);
        i != end; ++i) {
     DCHECK_EQ(parent_id, (*i)->ref(PARENT_ID));
     result->push_back((*i)->ref(META_HANDLE));
   }
 }
 
 }  // namespace syncable