Add hooks to permission layer for permission action reporting

chrome/browser/permissions/permission_uma_util_unittest.cc

+// 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/permissions/permission_uma_util.h"
+
+#include "base/command_line.h"
+#include "chrome/browser/signin/fake_signin_manager_builder.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
+#include "components/browser_sync/common/browser_sync_switches.h"
+#include "components/prefs/pref_service.h"
+#include "components/sync_driver/glue/sync_backend_host_mock.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+const char* kTestingGaiaId = "gaia_id";

[Lei Zhang, 2016/07/14 09:34:59]

This should be:

const char kFoo[] = "foo";

Because one can do:

const char* kBar = "bar";
kBar = nullptr;

and that's not very const.

[grt (UTC plus 2), 2016/07/14 10:08:09]

even better:

constexpr char kFoo[] = "foo";

[stefanocs, 2016/07/14 13:29:24]

Done. Sorry I missed this, copied this from
https://cs.chromium.org/chromium/src/chrome/browser/ui/sync/one_click_signin_....

+const char* kTestingUsername = "fake_username";
+}  // namespace
+
+class PermissionUmaUtilTest : public testing::Test {
+ protected:
+  PermissionUmaUtilTest() : profile_(new TestingProfile()) {}

[Lei Zhang, 2016/07/14 09:34:58]

I think you are creating |profile_| twice per test - here and in SetUp().

[stefanocs, 2016/07/14 13:29:24]

Done.

+
+  static bool IsOptedIntoPermissionActionReporting(Profile* profile) {

[Lei Zhang, 2016/07/14 09:34:58]

Make this a member function, since you always call it for profile() ?

[stefanocs, 2016/07/14 13:29:24]

It is called for profile()->GetOffTheRecordProfile() once below.

+    return PermissionUmaUtil::IsOptedIntoPermissionActionReporting(profile);
+  }
+
+  void SetUp() override {
+    profile_.reset(new TestingProfile());
+    ResetCommandLine();
+  }
+
+  void FakeSignIn() {
+    SigninManagerBase* signin_manager =
+        static_cast<FakeSigninManagerForTesting*>(

[Lei Zhang, 2016/07/14 09:34:58]

How did you get a FakeSigninManagerForTesting? Who created it?

[stefanocs, 2016/07/14 13:29:24]

It was just casted like in other unittest
https://cs.chromium.org/chromium/src/chrome/browser/signin/signin_tracker_uni...

[Lei Zhang, 2016/07/15 00:00:53]

Yes, but the SigninTrackerTest first built one ~10 lines above.

+            SigninManagerFactory::GetForProfile(profile()));
+    signin_manager->SetAuthenticatedAccountInfo(kTestingGaiaId,
+                                                kTestingUsername);
+    // Attach a sync backend to the profile sync service.
+    GetProfileSyncService()->backend_.reset(
+        new browser_sync::SyncBackendHostMock());
+    GetProfileSyncService()->backend_initialized_ = true;
+  }
+
+  void ResetCommandLine() {
+    base::CommandLine::Reset();

[Lei Zhang, 2016/07/14 09:14:42]

Sorry, but we cannot do this in a unit test. There are 6 places in the entire
code base that calls Reset(), and the other 5 are all main() like functions.

Especially when running with --single-process-tests, the unit test process is a
single process shared by all the test cases. Doing this changes the global
CommandLine and screws up any other code that's holding on to the
about-to-be-destroyed CommandLine via a pointer. Any test cases that run after
this gets called may end up calling code holding on to the old CommandLine,
resulting in a use after free. See https://crbug.com/628061 for an example of
this, or try running:

unit_tests --gtest_filter=M*.*:P*.* --gtest_shuffle --test-launcher-bot-mode
--single-process-tests --test-tiny-timeout=1000

a few times. Eventually --gtest_shuffle rolls a 12 and kaboom. Unit tests that
mess with globals are a pain to track down because it is not obvious who set us
up with the bomb. Fortunately I have seen this before, and it *only* took 2
hours of running test combinations to figure this out.

[Lei Zhang, 2016/07/14 09:34:58]

You probably want base::test::ScopedCommandLine instead.

[stefanocs, 2016/07/14 13:29:23]

Acknowledged.

[raymes, 2016/07/15 02:20:40]

:( Really sorry for the hassle. I didn't think about this carefully and thought
it would be isolated for the test (which is a silly assumption in retrospect).

Should we make this scarier looking (e.g. rename it)?

[Lei Zhang, 2016/07/15 02:33:52]

I have a CL to add a comment to encourage the use of ScopedCommandLine. I think
the general issue is tests can be sloppier about cleanup because most bots now
run tests with the parallel test runner, where these types of issues are masked.
The bots that run tests serially get hosed.

+    base::CommandLine::Init(0, NULL);
+  }
+
+  void SetKeepEverythingSynced(bool flag) {
+    GetProfileSyncService()->sync_prefs_.SetKeepEverythingSynced(flag);
+  }
+
+  void SetSafeBrowsing(bool enabled) {
+    PrefService* preferences = profile_->GetPrefs();
+    preferences->SetBoolean(prefs::kSafeBrowsingEnabled, enabled);
+  }
+
+  ProfileSyncService* GetProfileSyncService() {
+    return ProfileSyncServiceFactory::GetForProfile(profile());
+  }
+
+  Profile* profile() { return profile_.get(); }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  std::unique_ptr<Profile> profile_;
+};
+
+// Test that PermissionUmaUtil::IsOptedIntoPermissionActionReporting returns
+// true if Safe Browsing is enabled, Permission Action Reporting flag is
+// enabled, not in incognito mode and signed in with default sync preferences.
+TEST_F(PermissionUmaUtilTest, IsOptedIntoPermissionActionReportingSignInCheck) {
+  SetSafeBrowsing(true);
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnablePermissionActionReporting);
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+
+  FakeSignIn();
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(
+      profile()->GetOffTheRecordProfile()));
+  EXPECT_TRUE(IsOptedIntoPermissionActionReporting(profile()));
+}
+
+// Test that PermissionUmaUtil::IsOptedIntoPermissionActionReporting returns
+// false if Permission Action Reporting is not enabled.
+TEST_F(PermissionUmaUtilTest, IsOptedIntoPermissionActionReportingFlagCheck) {
+  SetSafeBrowsing(true);
+  FakeSignIn();
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnablePermissionActionReporting);
+  EXPECT_TRUE(IsOptedIntoPermissionActionReporting(profile()));
+
+  ResetCommandLine();
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisablePermissionActionReporting);
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+}
+
+// Test that PermissionUmaUtil::IsOptedIntoPermissionActionReporting returns
+// false if Safe Browsing is disabled.
+TEST_F(PermissionUmaUtilTest,
+       IsOptedIntoPermissionActionReportingSafeBrowsingCheck) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnablePermissionActionReporting);
+  FakeSignIn();
+  SetSafeBrowsing(true);
+  EXPECT_TRUE(IsOptedIntoPermissionActionReporting(profile()));
+
+  SetSafeBrowsing(false);
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+}
+
+// Test that PermissionUmaUtil::IsOptedIntoPermissionActionReporting returns
+// false if Sync is disabled.
+TEST_F(PermissionUmaUtilTest,
+       IsOptedIntoPermissionActionReportingProfileSyncServiceCheck) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnablePermissionActionReporting);
+  SetSafeBrowsing(true);
+  FakeSignIn();
+  EXPECT_TRUE(IsOptedIntoPermissionActionReporting(profile()));
+
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kDisableSync);
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+}
+
+// Test that PermissionUmaUtil::IsOptedIntoPermissionActionReporting returns
+// false if Tab Sync and Pref Sync are not both enabled.
+TEST_F(PermissionUmaUtilTest,
+       IsOptedIntoPermissionActionReportingSyncPreferenceCheck) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnablePermissionActionReporting);
+  SetSafeBrowsing(true);
+  FakeSignIn();
+  EXPECT_TRUE(IsOptedIntoPermissionActionReporting(profile()));
+
+  SetKeepEverythingSynced(false);
+  GetProfileSyncService()->ChangePreferredDataTypes(
+      syncer::ModelTypeSet(syncer::PROXY_TABS));
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+
+  SetKeepEverythingSynced(false);
+  GetProfileSyncService()->ChangePreferredDataTypes(
+      syncer::ModelTypeSet(syncer::PRIORITY_PREFERENCES));
+  EXPECT_FALSE(IsOptedIntoPermissionActionReporting(profile()));
+
+  SetKeepEverythingSynced(true);
+  GetProfileSyncService()->ChangePreferredDataTypes(
+      syncer::ModelTypeSet(syncer::PROXY_TABS, syncer::PREFERENCES));
+  EXPECT_TRUE(IsOptedIntoPermissionActionReporting(profile()));
+}