Do not record startup metrics when non-browser UI was displayed

chrome/browser/ui/profile_error_browsertest.cc

+// Copyright 2014 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 <string>
+
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_samples.h"
+#include "base/metrics/statistics_recorder.h"
+#include "base/path_service.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/string_util.h"
+#include "base/values.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/simple_message_box_internal.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/ui_test_utils.h"
+
+namespace {
+
+// Returns the number of times |histogram_name| was reported so far;
+int GetTrackedStartupHistogramCount(const char* histogram_name) {

[gab, 2015/06/09 15:52:28]

Given you test only cares whether queried histograms were reported or not, make
this return a bool and don't check the actual count below.

+  const base::HistogramBase* histogram =
+      base::StatisticsRecorder::FindHistogram(histogram_name);
+  if (!histogram)
+    return 0;
+
+  scoped_ptr<base::HistogramSamples> samples(histogram->SnapshotSamples());
+  return samples->TotalCount();
+}
+
+#define PROFILE_ERROR_BROWSER_TEST(fixture, test_name)                         \
+  IN_PROC_BROWSER_TEST_P(fixture, PRE_##test_name) { SetupProfile(); }         \
+  IN_PROC_BROWSER_TEST_P(fixture, test_name) { VerifyReactionToProfAttack(); } \
+  INSTANTIATE_TEST_CASE_P(fixture##Instance, fixture, testing::Bool());

[gab, 2015/06/09 15:52:28]

Inline these below (i.e. get rid of the PROFILE_ERROR_BROWSER_TEST macro).

The only reason the other test was using a macro is that it needed to do this
multiple times. In general, try to avoid writing macros as much as possible (in
fact I think this is the only one I've ever written in 3 years here!).

+
+// A base fixture that instantiates tests via the PREF_HASH_BROWSER_TEST
+// macro above.

[gab, 2015/06/09 15:52:28]

Remove this comment.

+class ProfileErrorBrowserTest : public InProcessBrowserTest,
+                                public testing::WithParamInterface<bool> {
+ public:
+  ProfileErrorBrowserTest() : do_attack_(GetParam()) {}
+
+  // Called from the PRE_ test's body, sets up the profile so the main test
+  // can attack it.
+  void SetupProfile() {
+    browser()->profile()->GetPrefs()->SetString(prefs::kHomePage,
+                                                "http://example.com");

[gab, 2015/06/09 15:52:28]

I'd say get rid of this (and have the PRE_ test be an empty body).

We only care to get a default set of preferences (which we get without having to
set any).

(in general, keep the test to strictly what is required, no more)

+  }
+
+  bool SetUpUserDataDirectory() override {
+    // Setup normally in the PRE test and attack user profile in the main test.
+    if (IsPRETest())
+      return InProcessBrowserTest::SetUpUserDataDirectory();
+
+    AttackProfileOnDisk();
+    return true;
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    InProcessBrowserTest::SetUpInProcessBrowserTestFixture();

[gab, 2015/06/09 15:52:28]

Remove this override (i.e. if you don't override than it will do exactly that --
call the superclass -- anyways).

+  }
+
+ protected:
+  void AttackProfileOnDisk() {

[gab, 2015/06/09 15:52:28]

Rename "Attack" to "Corrupt" here an elsewhere (this test is not really about
attacking the file but about corrupting the profile)

+    if (do_attack_) {

[gab, 2015/06/09 15:52:28]

Instead of having the boolean in here, have the caller of this code decide
whether it should call CorruptProfileOnDisk() based on this boolean (i.e. in
general, separate logic (how) from policy (whether to do it or not)).

+      base::FilePath profile_dir;
+      EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &profile_dir));
+      profile_dir =
+          profile_dir.AppendASCII(TestingProfile::kTestUserProfileDir);
+
+      // Read the preferences from disk.

[gab, 2015/06/09 15:52:28]

Remove this comment.

+      const base::FilePath unprotected_pref_file =

[gab, 2015/06/09 15:52:28]

s/unprotected_pref_file/pref_file/

+          profile_dir.Append(chrome::kPreferencesFilename);
+      EXPECT_TRUE(base::PathExists(unprotected_pref_file));
+
+      // Corrupt the user profile.
+      std::string junk("junk");
+      EXPECT_TRUE(
+          base::AppendToFile(unprotected_pref_file, junk.c_str(), junk.size()));
+
+      // Emulate but do not show the message box so the test can terminate
+      chrome::internal::g_should_skip_message_box_for_test = true;

[gab, 2015/06/09 15:52:28]

I don't think this belongs in the CorruptProfileOnDisk() logic.

Rather I think this belongs in SetUp() (which you'll need to override).

See in_process_browser_test.h for an idea of everything you can override:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...

We would typically prefer to override SetUpOnMainThread() but in this case I'm
afraid it is called late (i.e. after the profile has failed to load) and will
result in blocking on the message box before we set this boolean.

+    }
+  }
+
+  // Called from the body of the main test. Verifies that the browser had the
+  // desired reaction facing the attack orchestrated in AttackProfileOnDisk().
+  void VerifyReactionToProfAttack() {

[gab, 2015/06/09 15:52:28]

I think this should simply move to the body of the main test.

+    // Navigate to a URL so the first non-empty paint is registered.
+    ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/"));
+    if (do_attack_) {
+      EXPECT_EQ(GetTrackedStartupHistogramCount(
+                    "Startup.FirstWebContents.NonEmptyPaint"),
+                0);
+      EXPECT_EQ(GetTrackedStartupHistogramCount(
+                    "Startup.FirstWebContents.MainFrameLoad"),
+                0);
+    } else {
+      EXPECT_GT(GetTrackedStartupHistogramCount(
+                    "Startup.FirstWebContents.NonEmptyPaint"),
+                0);
+      EXPECT_GT(GetTrackedStartupHistogramCount(
+                    "Startup.FirstWebContents.MainFrameLoad"),
+                0);
+    }
+  }
+
+  bool do_attack_;
+
+ private:
+  // Returns true if this is the PRE_ phase of the test.
+  bool IsPRETest() {
+    return StartsWithASCII(
+        testing::UnitTest::GetInstance()->current_test_info()->name(), "PRE_",
+        true /* case_sensitive */);
+  }
+};
+
+}  // namespace
+
+#if defined(OS_CHROMEOS)
+#define MAYBE_CorruptedProfile DISABLED_CorruptedProfile
+#define PRE_MAYBE_CorruptedProfile PRE_DISABLED_CorruptedProfile
+#else
+#define MAYBE_CorruptedProfile CorruptedProfile
+#define PRE_MAYBE_CorruptedProfile PRE_CorruptedProfile
+#endif
+// Disable the test on chromos since kernel controls the user profile we
+// won't be able to attack.

[gab, 2015/06/09 15:52:28]

This comment should go on line 130 between the if def(CHROME_OS) and the
#defines.

+PROFILE_ERROR_BROWSER_TEST(ProfileErrorBrowserTest, MAYBE_CorruptedProfile);