PreferencesManager

chrome/browser/prefs/preferences_connection_manager.cc

Index: chrome/browser/prefs/preferences_connection_manager.cc
diff --git a/chrome/browser/prefs/preferences_connection_manager.cc b/chrome/browser/prefs/preferences_connection_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7709a01a8c7958d91cc0c122247dd6418b1d5c65
--- /dev/null
+++ b/chrome/browser/prefs/preferences_connection_manager.cc
@@ -0,0 +1,67 @@
+// Copyright 2016 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/prefs/preferences_connection_manager.h"
+
+#include "chrome/browser/prefs/preferences_manager.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
+#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
+
+namespace {
+
+// Required singleton subclass to control dependencies between
+// PreferenceConnectionManager and the KeyedServiceShutdownNotifier.
+class ShutdownNotifierFactory
+    : public BrowserContextKeyedServiceShutdownNotifierFactory {
+ public:
+  static ShutdownNotifierFactory* GetInstance() {
+    return base::Singleton<ShutdownNotifierFactory>::get();
+  }
+
+ private:
+  friend struct base::DefaultSingletonTraits<ShutdownNotifierFactory>;
+
+  ShutdownNotifierFactory()
+      : BrowserContextKeyedServiceShutdownNotifierFactory(
+            "PreferencesConnectionManager") {
+    DependsOn(ProfileOAuth2TokenServiceFactory::GetInstance());
+  }
+  ~ShutdownNotifierFactory() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
+};
+
+}  // namespace
+
+PreferencesConnectionManager::PreferencesConnectionManager()
+    : profile_shutdown_notification_(
+          ShutdownNotifierFactory::GetInstance()
+              ->Get(ProfileManager::GetActiveUserProfile())
+              ->Subscribe(
+                  base::Bind(&PreferencesConnectionManager::OnProfileDestroyed,
+                             base::Unretained(this)))) {}
+
+PreferencesConnectionManager::~PreferencesConnectionManager() {}
+
+void PreferencesConnectionManager::ProcessRequest(
+    prefs::mojom::PreferencesManagerRequest request) {
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  mojo::StrongBindingPtr<prefs::mojom::PreferencesManager> binding =
+      mojo::MakeStrongBinding(base::MakeUnique<PreferencesManager>(profile),
+                              std::move(request));
+  bindings_.push_back(std::move(binding));
+}
+
+void PreferencesConnectionManager::OnProfileDestroyed() {
+  for (auto& it : bindings_) {
+    // Check that the PreferencesManager was not cleared from a connection

[sky, 2016/12/07 23:54:25]

This description doesn't really match what happens here. Remove it?

[jonross, 2016/12/08 01:03:19]

Maybe tweak the wording? As its possible that arriving here that the
PreferencesManager has been cleared.  As StrongBindingPtr kills itself upon
onConnectionError, so our weakptr here may be invalid.

[dcheng, 2016/12/08 23:13:01]

Maybe just "shut down any preference managers that are still live".

(It does feel like that the PreferencesManager should just have a pointer back
to the Manager, and unregister itself on deletion. It's a bit unusual that
|bindings_| can grow without bound)

[jonross, 2016/12/08 23:32:55]

I'll switch to the cleaner doc you suggest.

I went with this pattern based on the mojo::InterfacePtrSet. Where we hold onto
WeakPtrs, which delete themselves on connection errors.

I haven't added a clearing of |bindings_| yet. I expected the size of bindings_
to remain small. One per connecting client process. With rare connection errors,
and eventual shutdowns. With most connections being like Ash, for the lifetime
of the process.

I could add a clearing of |bindings_| similar to mojo::PtrSet::ClearNullPtrs,
which runs after each action on the class.

[dcheng, 2016/12/09 05:26:41]

It feels a bit inelegant, since dead weak pointers will build up in this vector
over time. I agree that it's useful to have an object be strongly bound whenever
possible; I just wish there was a less awkward way to write this.

[jonross, 2016/12/09 16:49:16]

Yeah I agree if that.

I've filed crbug.com/672881 to track a full solution to this. I would like
future usage to avoid this problem.

I'll update this review by setting a callback on the mojo::StrongBindingPtrs
which will then clear them from the container.

+    // error. Then close the connection.
+    if (it)
+      it->Close();
+  }
+
+  profile_shutdown_notification_.reset();
+}