Handle empty string in GetSavePasswordDialogTitleTextAndLinkRange gracefully

chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc

 // Copyright 2015 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/ui/passwords/manage_passwords_view_utils.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
+#include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/range/range.h"
 #include "url/gurl.h"
 
 namespace {
 
+class EmptyDelegate : public ui::ResourceBundle::Delegate {
+ public:
+  EmptyDelegate() {}
+  ~EmptyDelegate() override {}
+
+  // ui::ResourceBundle::Delegate
+  base::FilePath GetPathForResourcePack(const base::FilePath& pack_path,
+                                        ui::ScaleFactor scale_factor) override {
+    return base::FilePath();
+  }
+
+  base::FilePath GetPathForLocalePack(const base::FilePath& pack_path,
+                                      const std::string& locale) override {
+    return base::FilePath();
+  }
+
+  gfx::Image GetImageNamed(int resource_id) override { return gfx::Image(); }
+
+  gfx::Image GetNativeImageNamed(int resource_id) override {
+    return gfx::Image();
+  }
+
+  base::RefCountedMemory* LoadDataResourceBytes(
+      int resource_id,
+      ui::ScaleFactor scale_factor) override {
+    return nullptr;
+  }
+
+  bool GetRawDataResource(int resource_id,
+                          ui::ScaleFactor scale_factor,
+                          base::StringPiece* value) override {
+    return false;
+  }
+
+  bool GetLocalizedString(int message_id, base::string16* value) override {
+    value->clear();
+    return true;

[vasilii, 2016/11/24 17:26:28]

It's strange to return true in production if the read failed. I think for the
sake of testing it's OK.

[vabr (Chromium), 2016/11/25 09:57:28]

This is now removed.
(I agree this was strange. In the real bug the empty string did not come from a
delegate, I suspect, but at that version of my test, the delegate seemed the
best way to emulate the situation. Not any longer. :))

+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EmptyDelegate);
+};
+
 const struct {
   const char* const user_visible_url;
   const char* const form_origin_url;
   bool is_smartlock_branding_enabled;
   PasswordTitleType bubble_type;
   const char* const expected_domain_placeholder;  // "this site" or domain name
   size_t expected_link_range_start;
   size_t expected_link_range_end;
 } kDomainsTestCases[] = {
     // Same domains.
@@ skipping 86 matching lines @@
         PasswordTitleType::UPDATE_PASSWORD) {
       EXPECT_TRUE(title.find(base::ASCIIToUTF16("update")) !=
                   base::string16::npos);
     } else {
       EXPECT_TRUE(title.find(base::ASCIIToUTF16("save")) !=
                   base::string16::npos);
     }
   }
 }
 
+// Check that empty localised strings do not cause a crash.
+TEST(ManagePasswordsViewUtilTest,
+     GetSavePasswordDialogTitleTextAndLinkRange_EmptyStrings) {
+  EmptyDelegate delegate;
+
+  ui::ResourceBundle::CleanupSharedInstance();
+  ui::ResourceBundle::InitSharedInstanceWithLocale(

[vasilii, 2016/11/24 17:26:28]

I'm afraid that your test doesn't leave the global object in the same state when
it started. It will lead to incomprehensible flakiness of random tests.

[vabr (Chromium), 2016/11/25 09:57:28]

That's a good point. Also, I realised that the delegate is unnecessarily heavy
for just overriding string definitions. So I created a new scoper for that,
which should address the clean-up concern and use the simpler way of overriding.

+      std::string(), &delegate,
+      ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
+
+  base::string16 title;
+  gfx::Range title_link_range;
+  const GURL kExample = GURL("http://example.org");

[vasilii, 2016/11/24 17:26:28]

Why not const GURL kExample("http://example.org");

[vabr (Chromium), 2016/11/25 09:57:28]

My oversight during copy-pasting.
Fixed now.

+  GetSavePasswordDialogTitleTextAndLinkRange(kExample, kExample, true,
+                                             PasswordTitleType::SAVE_PASSWORD,
+                                             &title, &title_link_range);
+}
+
 TEST(ManagePasswordsViewUtilTest, GetManagePasswordsDialogTitleText) {
   for (size_t i = 0; i < arraysize(kDomainsTestCases); ++i) {
     SCOPED_TRACE(testing::Message() << "user_visible_url = "
                                     << kDomainsTestCases[i].user_visible_url
                                     << ", password_origin_url = "
                                     << kDomainsTestCases[i].form_origin_url);
 
     base::string16 title;
     gfx::Range title_link_range;
     GetManagePasswordsDialogTitleText(
@@ skipping 41 matching lines @@
       false /* is_smartlock_branding_enabled */,
       GetParam(),
       &title, &title_link_range);
   EXPECT_GE(title.find(branding, 0), title.size());
   EXPECT_EQ(0U, title_link_range.start());
   EXPECT_EQ(0U, title_link_range.end());
 }
 
 INSTANTIATE_TEST_CASE_P(, AccountChooserDialogTitleTest,
                         ::testing::Bool());