sync: Introduce ModelTypeRegistry and helpers

sync/engine/model_type_registry.h

+// Copyright (c) 2013 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 SYNC_ENGINE_MODEL_TYPE_REGISTRY_H_
+#define SYNC_ENGINE_MODEL_TYPE_REGISTRY_H_
+
+#include <map>
+
+#include "base/memory/scoped_ptr.h"
+#include "sync/base/sync_export.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/internal_api/public/engine/model_safe_worker.h"
+
+namespace syncer {
+
+class CommitterList;
+class UpdaterList;
+
+namespace syncable {
+class Directory;
+}  // namespace syncable
+
+class SYNC_EXPORT_PRIVATE ModelTypeRegistry {
+ public:
+  ModelTypeRegistry(
+      syncable::Directory* directory,

[Nicolas Zea, 2014/01/04 02:09:34]

nit: have directory be second param (input/output ordering)

[rlarocque, 2014/01/06 20:03:33]

Done.

+      const std::vector<ModelSafeWorker*>& workers);

[Nicolas Zea, 2014/01/04 02:09:34]

Given that ModelSafeWorkers are refcounted, I think it's incorrect to be passing
them via raw pointer.

[rlarocque, 2014/01/06 20:03:33]

I agree.  This looks wrong to me, too.

However, for some reason this is how they're passed around inside
SyncBackendHostCore's DoInitializeOptions and SyncManager::Init().  I was hoping
there was a good reason for that, but I have no idea what that reason is.  Even
if there is no good reason for it, it seems wrong to change it in this CL.

For what it's worth, this class will end up taking a scoped_refptr to these
workers and keep it for the rest of the sync backend's lifetime.  I think that
matches what the old code did.

[Nicolas Zea, 2014/01/07 23:11:40]

Fair enough, maybe file a bug to fix this in a separate CL?

[rlarocque, 2014/01/08 01:37:23]

Done: crbug.com/332251

[tim (not reviewing), 2014/01/08 19:33:41]

I think I traced this back to
https://chromiumcodereview.appspot.com/10388187/diff2/4001:8002/chrome/browse...


It suggests (loosely) that there may not be a reason other than just wanting a
quick way to convert from the Info struct to a collection of MSWs to pass in.
Though maybe Richard will remember since he wrote the patch :)  

If you don't want to change it as part of this patch that's fine, but we should
take the opportunity to prevent a potential use-after-free bug! (because in
theory the address could be garbage before you even have a chance to addref).
The ideal thing would be fix first and rebase.  

[rlarocque, 2014/01/09 00:31:18]

Oh, I see what happened there.  I think I was trying to keep it consistent with
the old code, where the ModelSafeWorker array was retrieved from the registrar
as a vector<ModelSafeWorker*>.  The question then becomes: why did the registrar
expose these workers, which were at the time and still are refcounted, as raw
pointers?

You may be right about the use after free bug.  That's a good reason to think of
this quirk as not harmless, and therefore worth fixing.

+  ~ModelTypeRegistry();
+
+  // Sets the set of enabled types.
+  void SetEnabledDirectoryTypes(const ModelSafeRoutingInfo& routing_info);
+
+  // Simple getters.
+  UpdaterList* updater_list();
+  CommitterList* committer_list();
+
+ private:
+  // Classes to manage the types hooked up to receive and commit sync data.
+  scoped_ptr<UpdaterList> updater_list_;
+  scoped_ptr<CommitterList> committer_list_;
+
+  // The known ModelSafeWorkers.
+  std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker> > workers_map_;
+
+  // The directory.  Not owned.
+  syncable::Directory* directory_;

[Nicolas Zea, 2014/01/04 02:09:34]

DISALLOW_COPY_AND_ASSIGN

[rlarocque, 2014/01/06 20:03:33]

Done.

+};
+
+}  // namespace syncer
+
+#endif // SYNC_ENGINE_MODEL_TYPE_REGISTRY_H_
+