Add size checks to extension icons to prevent out of memory conditions

Add size checks to extension icons and favicons to prevent out of memory conditions

BUG=165423
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173415

[Mustafa Acer, 2012-12-14 08:00:06]

Hello, can you please take a look at this code?

crbug.com/165423

[Matt Perry, 2012-12-14 18:57:08]

https://codereview.chromium.org/11576030/diff/1003/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_icon_source.cc (right):

https://codereview.chromium.org/11576030/diff/1003/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_icon_source.cc:127: if
(request->size>kMaxIconSize) {
Move these ifs below.

Also, familiarize yourself with the style guide.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi...
says you should put spaces around operators.

https://codereview.chromium.org/11576030/diff/1003/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_icon_source.cc:293: return false;
This is where the size is parsed. Just move all the bounds checking here.

https://codereview.chromium.org/11576030/diff/1003/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/11576030/diff/1003/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:87: DCHECK(size_in_dip == 64 ||
size_in_dip == 32 || size_in_dip == 16) <<
This DCHECK suggests that we only expect 16, 32, and 64 sized icons. Turn it
into an if statement and just set the size to 16 if it doesn't match one of
these.

[Matt Perry, 2012-12-14 18:57:25]

Please update the CL description with a BUG=<bug number> line.

[Mustafa Acer, 2012-12-14 19:28:40]

On 2012/12/14 18:57:25, Matt Perry wrote:
> Please update the CL description with a BUG=<bug number> line.

Thanks, please take another look.

[Mustafa Acer, 2012-12-14 19:29:25]

Thanks, please take another look.

https://codereview.chromium.org/11576030/diff/1004/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/11576030/diff/1004/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:87: size_in_dip = 16;
I'm hoping this won't break any case where a size 48 or any other nonstandard
favicon may be used somewhere in the ui.

[Matt Perry, 2012-12-14 19:33:44]

lgtm

https://codereview.chromium.org/11576030/diff/1004/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_icon_source.cc (right):

https://codereview.chromium.org/11576030/diff/1004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_icon_source.cc:277: size =
extension_misc::EXTENSION_ICON_GIGANTOR;
Let's return false instead.

https://codereview.chromium.org/11576030/diff/1004/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/11576030/diff/1004/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:87: size_in_dip = 16;
On 2012/12/14 19:29:25, Mustafa Acer wrote:
> I'm hoping this won't break any case where a size 48 or any other nonstandard
> favicon may be used somewhere in the ui.

If we used sizes like that anywhere, we should have been hitting this DCHECK.
Since no one has noticed, I'm guessing we don't.

[commit-bot: I haz the power, 2012-12-14 21:15:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@google.com/11576030/6007

[commit-bot: I haz the power, 2012-12-14 21:15:14]

Presubmit check for 11576030-6007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

Presubmit checks took 4.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-12-14 21:17:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@google.com/11576030/6007

[commit-bot: I haz the power, 2012-12-14 21:17:31]

Presubmit check for 11576030-6007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

[Matt Perry, 2012-12-14 21:37:56]

+estade for OWNERS review

[Evan Stade, 2012-12-15 02:25:59]

lgtm

[commit-bot: I haz the power, 2012-12-17 02:27:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@google.com/11576030/6007

[commit-bot: I haz the power, 2012-12-17 04:20:43]

Change committed as 173415