Unify the Windows Parental Controls Platform Caching

chrome/browser/prefs/incognito_mode_prefs.cc

Index: chrome/browser/prefs/incognito_mode_prefs.cc
diff --git a/chrome/browser/prefs/incognito_mode_prefs.cc b/chrome/browser/prefs/incognito_mode_prefs.cc
index e0255d4947d11a90c3d2e5fbbd56f707459c811b..99f4c3fe21db81dbbf155e3b46fa1c040603f912 100644
--- a/chrome/browser/prefs/incognito_mode_prefs.cc
+++ b/chrome/browser/prefs/incognito_mode_prefs.cc
@@ -7,10 +7,12 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
+#include "base/threading/thread_restrictions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
+#include "content/public/browser/browser_thread.h"
 
 #if defined(OS_WIN)
 #include "base/win/metro.h"
@@ -20,10 +22,20 @@
 #include "chrome/browser/android/chromium_application.h"
 #endif  // OS_ANDROID
 
+using content::BrowserThread;
+
 #if defined(OS_WIN)
 namespace {
 
-bool g_parental_control_on = false;
+// Possible values for the parental controls state.
+enum class ParentalControlsState {
+  // Parental controls state unknown. We have not queried for this value yet.
+  UNKNOWN = 0,
+  // Parental controls activity logging disabled.
+  ACTIVITY_LOGGING_DISABLED = 1,
+  // Parental controls activity logging enabled.
+  ACTIVITY_LOGGING_ENABLED = 2,
+} g_parental_controls_state = ParentalControlsState::UNKNOWN;
 
 } // empty namespace
 #endif  // OS_WIN
@@ -104,25 +116,22 @@ bool IncognitoModePrefs::CanOpenBrowser(Profile* profile) {
 // static
 bool IncognitoModePrefs::ArePlatformParentalControlsEnabled() {
 #if defined(OS_WIN)
-  // Disable incognito mode windows if parental controls are on. This is only
-  // for Windows Vista and above.
-  return base::win::IsParentalControlActivityLoggingOn();
-#elif defined(OS_ANDROID)
-  return chrome::android::ChromiumApplication::AreParentalControlsEnabled();
-#else
-  return false;
-#endif
-}
-
-#if defined(OS_WIN)
-void IncognitoModePrefs::InitializePlatformParentalControls() {
-  g_parental_control_on = base::win::IsParentalControlActivityLoggingOn();
-}
-#endif // OS_WIN
-
-bool IncognitoModePrefs::ArePlatformParentalControlsEnabledCached() {
-#if defined(OS_WIN)
-  return g_parental_control_on;
+  if (g_parental_controls_state == ParentalControlsState::UNKNOWN) {
+    // Production: The thread isn't initialized, so we're the only thread with

[gab, 2015/03/03 13:38:59]

I don't really like having the implementation rely on call order for
thread-safety.

How about my earlier proposal of storing this state in a:

class ParentalControlsStateCache {
 public:
  ParentalControlsStateCache() : 
    state_(base::win::IsParentalControlActivityLoggingOn()) {}

  bool enabled() { return state_; }
 private:
  bool state_;

  DISALLOW_COPY_AND_ASSIGN(ParentalControlsStateCache);
}

base::LazyInstance<ParentalControlsStateCache>::Leaky
    g_parental_controls_state_cache;


Then the impl of ArePlatformParentalControlsEnabled() simply becomes:

return g_parental_controls_state_cache.Get().enabled();

and initializes itself in a thread-safe manner on the first call (which can be
primed from PreMainMessageLoopStart() for now and moved to another thread in a
follow-up CL).

[robliao, 2015/03/03 18:10:12]

We already depend on this today. base::win::IsParentalControlActivityLoggingOn
must be called from the UI or COM initialized thread. Making the state thread
safe doesn't solve the problem of requiring the call to happen on a COM
initialized thread.

[gab, 2015/03/03 18:22:59]

Acknowledged.

+    // IO and waiting allowed.
+    // Test: The thread may be initialized, so check that it's the UI thread.
+    CHECK(

[gab, 2015/03/03 13:38:59]

DCHECK is good enough for such things.

[robliao, 2015/03/03 18:10:12]

If this condition fails, we are potentially presenting incognito to a user when
incognito guarantees cannot be satisfied (e.g. website tracking), hence the
selection for check.

[gab, 2015/03/03 18:22:59]

Acknowledged.

[gab, 2015/03/03 18:36:38]

Actually going back on this, I don't think it's possible for this check to pass
in a Debug build yet fail in the wild.

So while you're right that we would never want to ship a build that doesn't pass
this condition and must enforce the contract here; enforcing the contract in
official builds is overkill.

(i.e. this not the same as CHECKs on things that affect security and could be
altered by malicious third-parties who escaped the sandbox or something)

[robliao, 2015/03/03 18:52:42]

Fair enough. Downgrading the check.

+        !BrowserThread::IsThreadInitialized(BrowserThread::UI) ||
+        BrowserThread::CurrentlyOn(BrowserThread::UI));
+    base::ThreadRestrictions::AssertIOAllowed();
+    base::ThreadRestrictions::AssertWaitAllowed();
+    g_parental_controls_state =
+        base::win::IsParentalControlActivityLoggingOn() ?

[gab, 2015/03/03 13:39:00]

I would argue that we should move the implementation of this call in the unnamed
namespace of this file (since it's the only user and other people using the
uncached value would be incorrect and would bring back this issue) -- if
multiple users require it one day (outside of chrome/), the generic impl should
provide a caching solution to each individual caller anyways (but it doesn't
make sense to implement this generically for now).

[robliao, 2015/03/03 18:10:12]

I'm don't quite understand this comment.
The reason the implementations here were combined was for the unit tests that
can't initialize.

[gab, 2015/03/03 18:22:59]

I'm talking about moving the implementation of
base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace of
this file to avoid other accidental misuses.

[robliao, 2015/03/03 18:26:13]

Ah gotcha. To confirm, the proposal to move this from metro.cc to here?

[gab, 2015/03/03 18:36:38]

Yes.

+            ParentalControlsState::ACTIVITY_LOGGING_ENABLED :
+            ParentalControlsState::ACTIVITY_LOGGING_DISABLED;
+  }
+  return g_parental_controls_state ==
+      ParentalControlsState::ACTIVITY_LOGGING_ENABLED;
 #elif defined(OS_ANDROID)
   return chrome::android::ChromiumApplication::AreParentalControlsEnabled();
 #else