[Fallback icons] Redoing http://crrev.com/988863002/, fixing use-after-free bug.

[Fallback icons] Redoing http://crrev.com/988863002/, fixing use-after-free bug.

http://crrev.com/988863002/ triggered use-after-free in ASAN because it passes
  ("#" + color_str).c_str()
to FindColor() (which takes char*), then dereferences the returned pointer, which
has been deallocated.  This is a redo of the CL that fixes the problem.

BUG=455063
Committed: https://crrev.com/bf33cc08aa8b255e082d0b0a242950d45693435d
Cr-Commit-Position: refs/heads/master@{#319875}

[huangs, 2015-03-09 20:04:02]

huangs@chromium.org changed reviewers:
+ jhawkins@chromium.org

[huangs, 2015-03-09 20:04:02]

Attempt to land this after rollback, PTAL.

The only changes are in

fallback_icon_url_parser.cc: bool ParsedFallbackIconPath::ParseColor().

This is the minimal-work fix.  But perhaps it's better to add a safer interface
in Skia, and move the logic there?

[James Hawkins, 2015-03-09 20:27:38]

https://codereview.chromium.org/989313002/diff/1/chrome/common/favicon/fallba...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/989313002/diff/1/chrome/common/favicon/fallba...
chrome/common/favicon/fallback_icon_url_parser.cc:20: if (len != 3 && len != 4
&& len != 6 && len != 8)
This could be cleaner as:

std::vector<int> lengths = {3, 4, 6, 8};
if (std::find(lengths.begin(), lengths.end(), len) != lengths.end()) {
  return false;
}

What do you think?

https://codereview.chromium.org/989313002/diff/1/chrome/common/favicon/fallba...
chrome/common/favicon/fallback_icon_url_parser.cc:110: DCHECK(end && !*end);  //
Ensure call succees and string is consumed.
nit: s/succees/succeeds/

[huangs, 2015-03-09 20:44:56]

Updated, PTAL.

https://codereview.chromium.org/989313002/diff/1/chrome/common/favicon/fallba...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/989313002/diff/1/chrome/common/favicon/fallba...
chrome/common/favicon/fallback_icon_url_parser.cc:20: if (len != 3 && len != 4
&& len != 6 && len != 8)
I prefer to list all the numbers, but I guess having the numbers together
improves readability.  Compromising by unsigned int array with std::find() so we
won't have to involves std::vector and memory allocation.

https://codereview.chromium.org/989313002/diff/1/chrome/common/favicon/fallba...
chrome/common/favicon/fallback_icon_url_parser.cc:110: DCHECK(end && !*end);  //
Ensure call succees and string is consumed.
Done, also updated comments.

[James Hawkins, 2015-03-09 21:06:48]

LGTM with nit.

https://codereview.chromium.org/989313002/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/989313002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:20: size_t
kValidHexColorSizes[] = {3, 4, 6, 8};
nit: const

[huangs, 2015-03-09 21:11:54]

Thanks!  I'll commit after more tests.

https://codereview.chromium.org/989313002/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/989313002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:20: size_t
kValidHexColorSizes[] = {3, 4, 6, 8};
On 2015/03/09 21:06:48, James Hawkins wrote:
> nit: const

Done.

[huangs, 2015-03-10 14:02:34]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-10 14:02:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/989313002/#ps60001 (title:
"Fix compile error.")

[commit-bot: I haz the power, 2015-03-10 14:02:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/989313002/60001

[commit-bot: I haz the power, 2015-03-10 14:05:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-10 14:06:25]

Patchset 4 (id:??) landed as
https://crrev.com/bf33cc08aa8b255e082d0b0a242950d45693435d
Cr-Commit-Position: refs/heads/master@{#319875}