Handle empty string in GetSavePasswordDialogTitleTextAndLinkRange gracefully

Handle empty string in GetSavePasswordDialogTitleTextAndLinkRange gracefully

GetSavePasswordDialogTitleTextAndLinkRange currently assumes that localised
strings for particular IDs contain placeholders, and attempts to access the
vector of offsets corresponding to such placeholders. However, in some
situations (http://crbug.com/658902#c18) the localised string may be empty.
This led to an out-of-bounds access and crash on Android.

This CL is the smallest change to handle such situation gracefully -- it checks
the length of the vector with offsets and gives up accessing it if it is empty.
This avoids the crash but results in displaying a UI with empty strings.
Ideally, the UI would also be supressed on these occasions, but that is left
for follow-up CLs. The current situation is already an improvement (password
manager is still not very useful, but at least the browser has a chance to
continue).

The CL also removes the temporary logging put in for the crash investigation.

BUG=658902
Committed: https://crrev.com/49ebcf7d64e71c7d3ac71bca66f65d61180bfb8c
Cr-Commit-Position: refs/heads/master@{#434482}

[vabr (Chromium), 2016-11-24 15:43:39]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 15:43:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-11-24 16:00:23]

vabr@chromium.org changed reviewers:
+ vasilii@chromium.org

[vabr (Chromium), 2016-11-24 16:00:24]

Hi Vasilii,

Could you please review? I might also bother you to discuss how/whether to
suppress UI if the localised strings are missing, although I'd like to keep that
out of this CL.

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-11-24 16:12:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 16:12:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
(right):

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:59: return
true;
It's strange to return true in production if the read failed. I think for the
sake of testing it's OK.

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:178:
ui::ResourceBundle::InitSharedInstanceWithLocale(
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.

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:184: const
GURL kExample = GURL("http://example.org");
Why not const GURL kExample("http://example.org");

[vabr (Chromium), 2016-11-25 09:57:02]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 09:57:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks for the review, Vasilii.

Please have a look at my answers.

Cheers,
Vaclav

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
(right):

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:59: return
true;
On 2016/11/24 17:26:28, vasilii wrote:
> It's strange to return true in production if the read failed. I think for the
> sake of testing it's OK.

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. :))

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:178:
ui::ResourceBundle::InitSharedInstanceWithLocale(
On 2016/11/24 17:26:28, vasilii wrote:
> 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.

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.

https://codereview.chromium.org/2526283002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:184: const
GURL kExample = GURL("http://example.org");
On 2016/11/24 17:26:28, vasilii wrote:
> Why not const GURL kExample("http://example.org");

My oversight during copy-pasting.
Fixed now.

[commit-bot: I haz the power, 2016-11-25 10:33:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-25 10:33:11]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-11-25 10:47:24]

lgtm

https://codereview.chromium.org/2526283002/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
(right):

https://codereview.chromium.org/2526283002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:200:
EXPECT_TRUE(title.empty()) << "|title| is non-empty: " << title;
You can say EXPECT_THAT(title, IsEmpty()); to get the same result.

[vabr (Chromium), 2016-11-25 11:22:41]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-11-25 11:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2526283002/#ps40001
(title: "EXPECT_THAT")

[vabr (Chromium), 2016-11-25 11:22:53]

Thanks for the review!
Vaclav

https://codereview.chromium.org/2526283002/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
(right):

https://codereview.chromium.org/2526283002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:200:
EXPECT_TRUE(title.empty()) << "|title| is non-empty: " << title;
On 2016/11/25 10:47:24, vasilii wrote:
> You can say EXPECT_THAT(title, IsEmpty()); to get the same result.

Done.

[commit-bot: I haz the power, 2016-11-25 11:23:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 12:02:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480072961813580,
"parent_rev": "7456c3243f53d9d9581d1b9398f7360de495928b", "commit_rev":
"76281a9e27ee2feae06cb9c9a6c37e8decbd8136"}

[commit-bot: I haz the power, 2016-11-25 12:02:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-25 12:05:47]

Description was changed from

==========
Handle empty string in GetSavePasswordDialogTitleTextAndLinkRange gracefully

GetSavePasswordDialogTitleTextAndLinkRange currently assumes that localised
strings for particular IDs contain placeholders, and attempts to access the
vector of offsets corresponding to such placeholders. However, in some
situations (http://crbug.com/658902#c18) the localised string may be empty.
This led to an out-of-bounds access and crash on Android.

This CL is the smallest change to handle such situation gracefully -- it checks
the length of the vector with offsets and gives up accessing it if it is empty.
This avoids the crash but results in displaying a UI with empty strings.
Ideally, the UI would also be supressed on these occasions, but that is left
for follow-up CLs. The current situation is already an improvement (password
manager is still not very useful, but at least the browser has a chance to
continue).

The CL also removes the temporary logging put in for the crash investigation.

BUG=658902
==========

to

==========
Handle empty string in GetSavePasswordDialogTitleTextAndLinkRange gracefully

GetSavePasswordDialogTitleTextAndLinkRange currently assumes that localised
strings for particular IDs contain placeholders, and attempts to access the
vector of offsets corresponding to such placeholders. However, in some
situations (http://crbug.com/658902#c18) the localised string may be empty.
This led to an out-of-bounds access and crash on Android.

This CL is the smallest change to handle such situation gracefully -- it checks
the length of the vector with offsets and gives up accessing it if it is empty.
This avoids the crash but results in displaying a UI with empty strings.
Ideally, the UI would also be supressed on these occasions, but that is left
for follow-up CLs. The current situation is already an improvement (password
manager is still not very useful, but at least the browser has a chance to
continue).

The CL also removes the temporary logging put in for the crash investigation.

BUG=658902

Committed: https://crrev.com/49ebcf7d64e71c7d3ac71bca66f65d61180bfb8c
Cr-Commit-Position: refs/heads/master@{#434482}
==========

[commit-bot: I haz the power, 2016-11-25 12:05:48]

Patchset 3 (id:??) landed as
https://crrev.com/49ebcf7d64e71c7d3ac71bca66f65d61180bfb8c
Cr-Commit-Position: refs/heads/master@{#434482}