[Sync] Refactor non-frontend DTC to handle new API properly.

chrome/browser/sync/glue/shared_change_processor.h

+// 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.
+
+#ifndef CHROME_BROWSER_SYNC_GLUE_SHARED_CHANGE_PROCESSOR_H_
+#define CHROME_BROWSER_SYNC_GLUE_SHARED_CHANGE_PROCESSOR_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "chrome/browser/sync/glue/generic_change_processor.h"
+#include "content/browser/browser_thread.h"
+
+class SyncData;
+class SyncableService;
+
+typedef std::vector<SyncData> SyncDataList;
+
+namespace browser_sync {
+
+
+// A ref-counted GenericChangeProcessor for use with datatypes that don't live
+// on the UI thread. We need to make it refcounted as the ownership transfer
+// from the DataTypeController is dependent on threading, and hence racy.
+// The SharedChangeProcessor should be created and destroyed on the UI thread,
+// but should only be connected and used on the same thread as the datatype it
+// interacts with. The only thread-safe method is Disconnect, which will
+// disconnect the change processor from the syncer, letting us shut down the
+// syncer/datatype without waiting for non-UI threads.
+class SharedChangeProcessor
+    : public GenericChangeProcessor,

[akalin, 2011/10/07 20:51:16]

I think this shouldn't inherit from GenericChangeProcessor, and should take the
GenericChangeProcessor as a parameter of Connect.  Whatever interacts with this
needs to know it's a SharedChangeProcessor, anyway (to call Disconnect)

[Nicolas Zea, 2011/10/07 22:05:52]

My main reason was that we want to access protected methods/data of the
GenericChangeProcessor, and that this does implement the same methods as a
generic change processor. What benefit is there in being a separate wrapper
class?

[akalin, 2011/10/07 22:18:28]

What protected methods do you need?  I only see the set_* methods being used,
and if this took a GCP in the constructor, you wouldn't need that.

The problem is that "SCP isa GCP" doesn't hold in a few ways: SCP isn't
non-thread-safe whereas GCP is (which is why you have to Detach all over the
place), and SCP is ref-counted whereas GCP isn't.  Basically, the stuff that
uses SCP uses it as a SCP directly, and there's nowhere where an SCP is used as
GCP (nor would it be desirable to, I think).

+      public base::RefCountedThreadSafe<SharedChangeProcessor,
+                                        BrowserThread::DeleteOnUIThread> {

[akalin, 2011/10/07 20:51:16]

any reason why this is DeleteOnUIThread?

[Nicolas Zea, 2011/10/07 22:05:52]

So that we have some determinism as to which thread it gets deleted on
(therefore letting  me add DCHECK's that it gets destroyed on the UI thread). If
you feel that's not reason enough I've no problem getting rid of it and the
DCHECK in the destructor though.

[akalin, 2011/10/07 22:18:28]

The problem is that DeleteOnUIThread is a guaranteed shutdown leak (since it
posts a task after the message loop is dead).

I'd prefer just asserting that the destructor is either on the UI thread or on
the data type thread.  If there's stuff that needs to be destroyed on a certain
thread, you can DeleteSoon() from there. (probably a shutdown leak, too, but
hey)

+ public:
+  // Create an uninitialized change processor (to be later connected).
+  explicit SharedChangeProcessor(UnrecoverableErrorHandler* error_handler);
+
+  // Connect to the syncer and the SyncableService specified. Should be called
+  // on the same thread the datatype resides.
+  virtual void Connect(SyncableService* local_service,
+                       sync_api::UserShare* user_share);
+
+  // Disconnects from the syncer. May be called from any thread. After this, all
+  // attempts to interact with the change processor by |local_service_| are
+  // dropped and return errors. The syncer will be safe to shut down from the
+  // point of view of this datatype.
+  // Note: Once disconnected, you cannot reconnect without creating a new

[akalin, 2011/10/07 20:51:16]

can we provide a stronger guarantee?  Will this be called on the UI thread?  if
so, we can use a CancellationFlag (base/memory/cancellation_flag.h) instead of a
lock + bool.

[Nicolas Zea, 2011/10/07 22:05:52]

Yes, this will be called on the UI thread. What guarantee isn't strong enough?
That we allow it to be called from any thread, vs just the UI thread?

The reason we can't use a cancellation flag is that the scope of the locking
there is only within the Set() and IsSet() methods. As a result there could
still be races if you attempt to Set() after something else has called IsSet()
but before it actually interacted with the syncer. I believe locking for the
scope of the method is required.

[akalin, 2011/10/07 22:18:28]

Ah, right.  I'm not sure I like the lock that much, since the UI thread may wait
on it while the other thread is doing something expensive (possibly I/O stuff). 
I'd have to think about it...

+  // change processor.
+  virtual void Disconnect();
+
+  // GenericChangeProcessor implementation (with disconnect support).
+  // Should only be called on the same thread the datatype resides.
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) OVERRIDE;
+  virtual SyncError GetSyncDataForType(syncable::ModelType type,
+                                       SyncDataList* current_sync_data);
+  virtual bool SyncModelHasUserCreatedNodes(syncable::ModelType type,
+                                            bool* has_nodes);
+  virtual bool CryptoReadyIfNecessary(syncable::ModelType type);
+
+ protected:
+  friend class base::RefCountedThreadSafe<SharedChangeProcessor>;
+  friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
+  friend class DeleteTask<SharedChangeProcessor>;
+
+  virtual ~SharedChangeProcessor() OVERRIDE;
+
+ private:
+  // Monitor lock for this object. All methods that interact with the Syncer
+  // must aquire this lock and check whether we're disconnected or not. Once
+  // disconnected, all attempted changes to or loads from the syncer return
+  // errors. This enables us to shut down the syncer without having to wait
+  // for possibly non-UI thread datatypes to complete work.
+  base::Lock monitor_lock_;
+  bool disconnected_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedChangeProcessor);
+};
+
+}  // namespace browser_sync
+
+#endif  // CHROME_BROWSER_SYNC_GLUE_SHARED_CHANGE_PROCESSOR_H_