Added a new case in unit test, and fixed some post-commit comments

from last review.

BUG=none
TEST=ran unit test.

Review URL: http://codereview.chromium.org/3848001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64512 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 87 insertions, 60 deletions

diff --git a/chrome/browser/chromeos/plugin_selection_policy.cc b/chrome/browser/chromeos/plugin_selection_policy.cc
index b27bc42..477965a 100644
--- a/chrome/browser/chromeos/plugin_selection_policy.cc
+++ b/chrome/browser/chromeos/plugin_selection_policy.cc
@@ -33,7 +33,8 @@ namespace chromeos {
 static const char kPluginSelectionPolicyFile[] =
     "/usr/share/chromeos-assets/flash/plugin_policy";
 
-PluginSelectionPolicy::PluginSelectionPolicy() : initialized_(false) {
+PluginSelectionPolicy::PluginSelectionPolicy()
+    : init_from_file_finished_(false) {
 }
 
 void PluginSelectionPolicy::StartInit() {
@@ -52,15 +53,13 @@ bool PluginSelectionPolicy::InitFromFile(const FilePath& policy_file) {
   // This must always be called from the FILE thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  // Note: We're "initialized" even if loading the file fails.
-  initialized_ = true;
-
   string data;
   // This should be a really small file, so we're OK with just
   // slurping it.
   if (!file_util::ReadFileToString(policy_file, &data)) {
     LOG(ERROR) << "Unable to read plugin policy file \""
                << policy_file.value() << "\".";
+    init_from_file_finished_ = true;
     return false;
   }
 
@@ -81,6 +80,7 @@ bool PluginSelectionPolicy::InitFromFile(const FilePath& policy_file) {
       // Has to be preceeded by a "plugin" statement.
       if (last_plugin.empty()) {
         LOG(ERROR) << "Plugin policy file error: 'allow' out of context.";
+        init_from_file_finished_ = true;
         return false;
       }
       line = line.substr(6);
@@ -92,6 +92,7 @@ bool PluginSelectionPolicy::InitFromFile(const FilePath& policy_file) {
       // Has to be preceeded by a "plugin" statement.
       if (last_plugin.empty()) {
         LOG(ERROR) << "Plugin policy file error: 'deny' out of context.";
+        init_from_file_finished_ = true;
         return false;
       }
       line = line.substr(5);
@@ -113,6 +114,7 @@ bool PluginSelectionPolicy::InitFromFile(const FilePath& policy_file) {
     policies.insert(make_pair(last_plugin, policy));
 
   policies_.swap(policies);
+  init_from_file_finished_ = true;
   return true;
 }
 
@@ -136,7 +138,8 @@ bool PluginSelectionPolicy::IsAllowed(const GURL& url,
   // initialization is complete.  Right now it is guaranteed only by
   // the startup order and the fact that InitFromFile runs on the FILE
   // thread too.
-  DCHECK(initialized_) << "Tried to check policy before policy is initialized.";
+  DCHECK(init_from_file_finished_)
+      << "Tried to check policy before policy is initialized.";
 
   string name = path.BaseName().value();
 
diff --git a/chrome/browser/chromeos/plugin_selection_policy.h b/chrome/browser/chromeos/plugin_selection_policy.h
index 4786e3c..1bf60a0 100644
--- a/chrome/browser/chromeos/plugin_selection_policy.h
+++ b/chrome/browser/chromeos/plugin_selection_policy.h
@@ -56,9 +56,10 @@ class PluginSelectionPolicy
  private:
   // To allow access to InitFromFile
   FRIEND_TEST_ALL_PREFIXES(PluginSelectionPolicyTest, Basic);
+  FRIEND_TEST_ALL_PREFIXES(PluginSelectionPolicyTest, FindFirstAllowed);
   FRIEND_TEST_ALL_PREFIXES(PluginSelectionPolicyTest, InitFromFile);
   FRIEND_TEST_ALL_PREFIXES(PluginSelectionPolicyTest, IsAllowed);
-  FRIEND_TEST_ALL_PREFIXES(PluginSelectionPolicyTest, FindFirstAllowed);
+  FRIEND_TEST_ALL_PREFIXES(PluginSelectionPolicyTest, MissingFile);
 
   // Initializes from the default policy file.
   bool Init();
@@ -70,7 +71,14 @@ class PluginSelectionPolicy
   typedef std::map<std::string, Policy> PolicyMap;
 
   PolicyMap policies_;
-  bool initialized_;
+
+  // This is used to DCHECK if we try and call IsAllowed or
+  // FindFirstAllowed before we've finished executing InitFromFile.
+  // Note: We're "finished" even if loading the file fails -- the
+  // point of the DCHECK is to make sure we haven't violated our
+  // ordering/threading assumptions, not to make sure that we're
+  // properly initialized.
+  bool init_from_file_finished_;
 
   DISALLOW_COPY_AND_ASSIGN(PluginSelectionPolicy);
 };
diff --git a/chrome/browser/chromeos/plugin_selection_policy_unittest.cc b/chrome/browser/chromeos/plugin_selection_policy_unittest.cc
index 455ed37..4bc4538 100644
--- a/chrome/browser/chromeos/plugin_selection_policy_unittest.cc
+++ b/chrome/browser/chromeos/plugin_selection_policy_unittest.cc
@@ -26,23 +26,23 @@ using std::vector;
 namespace chromeos {
 
 const char kBasicPolicy[] = "# This is a basic policy\n"
-                          "plugin test.so\n"
-                          "allow foo.com\n"
-                          "deny bar.com\n";
+                            "plugin test.so\n"
+                            "allow foo.com\n"
+                            "deny bar.com\n";
 
 const char kNoPluginPolicy[] = "# This is a policy with missing plugin.\n"
-                             "# Missing plugin test.so\n"
-                             "allow foo.com\n"
-                             "deny bar.com\n";
+                               "# Missing plugin test.so\n"
+                               "allow foo.com\n"
+                               "deny bar.com\n";
 const char kNoRulesPolicy[] = "# This is a policy with no rules\n"
-                            "plugin test.so\n";
+                              "plugin test.so\n";
 
 const char kEmptyPolicy[] = "# This is an empty policy\n";
 
 const char kCommentTestPolicy[] = "# This is a policy with inline comments.\n"
-                                "plugin test.so# like this\n"
-                                "allow foo.com # and this\n"
-                                "deny bar.com  # and this\n";
+                                  "plugin test.so# like this\n"
+                                  "allow foo.com # and this\n"
+                                  "deny bar.com  # and this\n";
 
 const char kMultiPluginTestPolicy[] =
     "# This is a policy with multiple plugins.\n"
@@ -59,9 +59,9 @@ const char kMultiPluginTestPolicy[] =
     "allow bim.com\n";
 
 const char kWhitespaceTestPolicy[] = "# This is a policy with odd whitespace.\n"
-                                   "  plugin\ttest.so# like this\n"
-                                   "\n\n \n   allow\t\tfoo.com # and this\n"
-                                   "\tdeny     bar.com\t\t\t# and this    \n";
+                                     "  plugin\ttest.so# like this\n"
+                                     "\n\n \n   allow\t\tfoo.com # and this\n"
+                                     "\tdeny     bar.com\t\t\t# and this    \n";
 
 class PluginSelectionPolicyTest : public PlatformTest {
  public:
@@ -79,14 +79,19 @@ class PluginSelectionPolicyTest : public PlatformTest {
   bool CreatePolicy(const std::string& name,
                     const std::string& contents,
                     FilePath* path) {
-    FilePath policy_file(temp_dir_.path());
-    policy_file = policy_file.Append(FilePath(name));
-    int bytes_written = file_util::WriteFile(policy_file,
-                                             contents.c_str(),
-                                             contents.size());
+    FilePath policy_file = GetPolicyPath(name);
+    size_t bytes_written = file_util::WriteFile(policy_file,
+                                                contents.c_str(),
+                                                contents.size());
     if (path)
       *path = policy_file;
-    return bytes_written >= 0;
+
+    return bytes_written == contents.size();
+  }
+
+  FilePath GetPolicyPath(const std::string& name) {
+    FilePath policy_file(temp_dir_.path());
+    return policy_file.Append(FilePath(name));
   }
 
  private:
@@ -160,37 +165,48 @@ TEST_F(PluginSelectionPolicyTest, IsAllowed) {
   scoped_refptr<PluginSelectionPolicy> policy1 = new PluginSelectionPolicy;
   ASSERT_TRUE(policy1->InitFromFile(path));
   EXPECT_TRUE(policy1->IsAllowed(GURL("http://www.foo.com/blah.html"),
-                               FilePath("/usr/local/bin/test.so")));
+                                 FilePath("/usr/local/bin/test.so")));
   EXPECT_FALSE(policy1->IsAllowed(GURL("http://www.bar.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                  FilePath("/usr/local/bin/test.so")));
   EXPECT_FALSE(policy1->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                  FilePath("/usr/local/bin/test.so")));
   EXPECT_TRUE(policy1->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                               FilePath("/usr/local/bin/real.so")));
+                                 FilePath("/usr/local/bin/real.so")));
 
   scoped_refptr<PluginSelectionPolicy> policy2 = new PluginSelectionPolicy;
   ASSERT_TRUE(CreatePolicy("no_rules", kNoRulesPolicy, &path));
   ASSERT_TRUE(policy2->InitFromFile(path));
   EXPECT_FALSE(policy2->IsAllowed(GURL("http://www.foo.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                  FilePath("/usr/local/bin/test.so")));
   EXPECT_FALSE(policy2->IsAllowed(GURL("http://www.bar.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                  FilePath("/usr/local/bin/test.so")));
   EXPECT_FALSE(policy2->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                  FilePath("/usr/local/bin/test.so")));
   EXPECT_TRUE(policy2->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                               FilePath("/usr/local/bin/real.so")));
+                                 FilePath("/usr/local/bin/real.so")));
 
   scoped_refptr<PluginSelectionPolicy> policy3 = new PluginSelectionPolicy;
   ASSERT_TRUE(CreatePolicy("empty", kEmptyPolicy, &path));
   ASSERT_TRUE(policy3->InitFromFile(path));
   EXPECT_TRUE(policy3->IsAllowed(GURL("http://www.foo.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                 FilePath("/usr/local/bin/test.so")));
   EXPECT_TRUE(policy3->IsAllowed(GURL("http://www.bar.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                 FilePath("/usr/local/bin/test.so")));
   EXPECT_TRUE(policy3->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                                FilePath("/usr/local/bin/test.so")));
+                                 FilePath("/usr/local/bin/test.so")));
   EXPECT_TRUE(policy3->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                               FilePath("/usr/local/bin/real.so")));
+                                 FilePath("/usr/local/bin/real.so")));
+}
+
+TEST_F(PluginSelectionPolicyTest, MissingFile) {
+  // Don't create any policy file, just get the path to one.
+  FilePath path = GetPolicyPath("missing_file");
+  scoped_refptr<PluginSelectionPolicy> policy = new PluginSelectionPolicy;
+  ASSERT_FALSE(policy->InitFromFile(path));
+  EXPECT_TRUE(policy->IsAllowed(GURL("http://www.foo.com/blah.html"),
+                                FilePath("/usr/local/bin/allow_foo.so")));
+  EXPECT_TRUE(policy->IsAllowed(GURL("http://www.bar.com/blah.html"),
+                                FilePath("/usr/local/bin/allow_foo.so")));
 }
 
 TEST_F(PluginSelectionPolicyTest, FindFirstAllowed) {
@@ -199,33 +215,33 @@ TEST_F(PluginSelectionPolicyTest, FindFirstAllowed) {
   scoped_refptr<PluginSelectionPolicy> policy = new PluginSelectionPolicy;
   ASSERT_TRUE(policy->InitFromFile(path));
   EXPECT_TRUE(policy->IsAllowed(GURL("http://www.foo.com/blah.html"),
-                               FilePath("/usr/local/bin/allow_foo.so")));
-  EXPECT_FALSE(policy->IsAllowed(GURL("http://www.bar.com/blah.html"),
                                 FilePath("/usr/local/bin/allow_foo.so")));
+  EXPECT_FALSE(policy->IsAllowed(GURL("http://www.bar.com/blah.html"),
+                                 FilePath("/usr/local/bin/allow_foo.so")));
   EXPECT_FALSE(policy->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                                FilePath("/usr/local/bin/allow_foo.so")));
+                                 FilePath("/usr/local/bin/allow_foo.so")));
   EXPECT_FALSE(policy->IsAllowed(GURL("http://www.bim.com/blah.html"),
-                                FilePath("/usr/local/bin/allow_foo.so")));
+                                 FilePath("/usr/local/bin/allow_foo.so")));
   EXPECT_FALSE(policy->IsAllowed(GURL("http://www.foo.com/blah.html"),
-                                FilePath("/usr/local/bin/allow_baz_bim1.so")));
+                                 FilePath("/usr/local/bin/allow_baz_bim1.so")));
   EXPECT_FALSE(policy->IsAllowed(GURL("http://www.bar.com/blah.html"),
-                                FilePath("/usr/local/bin/allow_baz_bim1.so")));
+                                 FilePath("/usr/local/bin/allow_baz_bim1.so")));
   EXPECT_TRUE(policy->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                               FilePath("/usr/local/bin/allow_baz_bim1.so")));
+                                FilePath("/usr/local/bin/allow_baz_bim1.so")));
   EXPECT_TRUE(policy->IsAllowed(GURL("http://www.bim.com/blah.html"),
-                               FilePath("/usr/local/bin/allow_baz_bim1.so")));
-  EXPECT_FALSE(policy->IsAllowed(GURL("http://www.google.com/blah.html"),
                                 FilePath("/usr/local/bin/allow_baz_bim1.so")));
+  EXPECT_FALSE(policy->IsAllowed(GURL("http://www.google.com/blah.html"),
+                                 FilePath("/usr/local/bin/allow_baz_bim1.so")));
   EXPECT_FALSE(policy->IsAllowed(GURL("http://www.foo.com/blah.html"),
-                                FilePath("/usr/local/bin/allow_baz_bim2.so")));
+                                 FilePath("/usr/local/bin/allow_baz_bim2.so")));
   EXPECT_FALSE(policy->IsAllowed(GURL("http://www.bar.com/blah.html"),
-                                FilePath("/usr/local/bin/allow_baz_bim2.so")));
+                                 FilePath("/usr/local/bin/allow_baz_bim2.so")));
   EXPECT_TRUE(policy->IsAllowed(GURL("http://www.baz.com/blah.html"),
-                               FilePath("/usr/local/bin/allow_baz_bim2.so")));
+                                FilePath("/usr/local/bin/allow_baz_bim2.so")));
   EXPECT_TRUE(policy->IsAllowed(GURL("http://www.bim.com/blah.html"),
-                               FilePath("/usr/local/bin/allow_baz_bim2.so")));
-  EXPECT_FALSE(policy->IsAllowed(GURL("http://www.google.com/blah.html"),
                                 FilePath("/usr/local/bin/allow_baz_bim2.so")));
+  EXPECT_FALSE(policy->IsAllowed(GURL("http://www.google.com/blah.html"),
+                                 FilePath("/usr/local/bin/allow_baz_bim2.so")));
   std::vector<WebPluginInfo> info_vector;
   WebPluginInfo info;
   // First we test that the one without any policy gets
@@ -239,13 +255,13 @@ TEST_F(PluginSelectionPolicyTest, FindFirstAllowed) {
   info.path = FilePath("/usr/local/bin/allow_baz_bim2.so");
   info_vector.push_back(info);
   EXPECT_EQ(0, policy->FindFirstAllowed(GURL("http://www.baz.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
   EXPECT_EQ(0, policy->FindFirstAllowed(GURL("http://www.foo.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
   EXPECT_EQ(0, policy->FindFirstAllowed(GURL("http://www.bling.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
   EXPECT_EQ(0, policy->FindFirstAllowed(GURL("http://www.google.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
 
   // Now move the plugin without any policy to the end.
   info_vector.clear();
@@ -258,13 +274,13 @@ TEST_F(PluginSelectionPolicyTest, FindFirstAllowed) {
   info.path = FilePath("/usr/local/bin/no_policy.so");
   info_vector.push_back(info);
   EXPECT_EQ(1, policy->FindFirstAllowed(GURL("http://www.baz.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
   EXPECT_EQ(0, policy->FindFirstAllowed(GURL("http://www.foo.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
   EXPECT_EQ(3, policy->FindFirstAllowed(GURL("http://www.bling.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
   EXPECT_EQ(3, policy->FindFirstAllowed(GURL("http://www.google.com/blah.html"),
-                                       info_vector));
+                                        info_vector));
 }
 
 }  // namespace chromeos