Check zero-length icon files during extension validation.

Check zero-length icon files during extension validation.

BUG=133748
TEST=included


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149721

[mitchellwrosen, 2012-06-27 19:53:48]

I currently am only checking for zero-length page action and browser action
icons, because I'm not exactly sure what other icons exist.

Actually, it just occurred to me what an "app icon" is (discussed in the issue
page)... it's the app's icon. =(

Anyways, I'll push this preliminary CL with a question: are there any other
icons this bug should cover, besides app icon, page action, and browser action?

Thanks guys.

[mitchellwrosen, 2012-06-27 20:38:26]

Whoops, I think I got it now. I'll delete that first patch because it was wrong,
ignore my first comment, and sorry about the email spam.

[asargent_no_longer_on_chrome, 2012-06-29 00:24:04]

Apologies for slow response time - I was busy with Google IO stuff and am on
vacation tomorrow, so I probably won't be able to look at this until Monday.
Feel free to send to another reviewer in chrome/common/extensions/OWNERS if you
want.

[mitchellwrosen, 2012-06-29 01:08:22]

On 2012/06/29 00:24:04, Antony Sargent wrote:
> Apologies for slow response time - I was busy with Google IO stuff and am on
> vacation tomorrow, so I probably won't be able to look at this until Monday.
> Feel free to send to another reviewer in chrome/common/extensions/OWNERS if
you
> want.

No worries. Have a good vacation.

[asargent_no_longer_on_chrome, 2012-07-03 19:46:48]

LGTM

https://chromiumcodereview.appspot.com/10690016/diff/5001/chrome/common/exten...
File chrome/common/extensions/extension_file_util.cc (right):

https://chromiumcodereview.appspot.com/10690016/diff/5001/chrome/common/exten...
chrome/common/extensions/extension_file_util.cc:233: int64 size;
(pet peeve nit) please initialize |size| to 0

https://chromiumcodereview.appspot.com/10690016/diff/5001/chrome/common/exten...
chrome/common/extensions/extension_file_util.cc:323: int64 size;
initialize to 0

https://chromiumcodereview.appspot.com/10690016/diff/5001/chrome/common/exten...
chrome/common/extensions/extension_file_util.cc:346: size == 0) {
nit: looks like you have this chunk of "does file exist and have >0 size" code
repeated 3 times. Can you factor it out into a helper method named
ValidateFilePath or FileHasContent or something along those lines?

[commit-bot: I haz the power, 2012-07-03 20:21:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/9002

[commit-bot: I haz the power, 2012-07-03 20:26:43]

Try job failure for 10690016-9002 (retry) on mac_rel for step "update".
It's a second try, previously, step "update" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-07-03 21:09:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/2004

[commit-bot: I haz the power, 2012-07-03 21:11:33]

Try job failure for 10690016-2004 on mac_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-07-09 21:21:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/1...

[commit-bot: I haz the power, 2012-07-09 22:15:32]

Try job failure for 10690016-14001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-11 19:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/1...

[commit-bot: I haz the power, 2012-07-11 20:36:13]

Try job failure for 10690016-13003 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-16 19:19:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/3...

[commit-bot: I haz the power, 2012-07-16 21:14:16]

Try job failure for 10690016-31001 (retry) (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-31 19:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/3...

[commit-bot: I haz the power, 2012-07-31 20:16:34]

Try job failure for 10690016-31001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-31 23:34:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/3...

[commit-bot: I haz the power, 2012-08-01 01:31:18]

Try job failure for 10690016-31001 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-02 17:39:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/3...

[Mihai Parparita -not on Chrome, 2012-08-02 17:41:10]

Actually, this isn't a matter of CQ flakiness. It looks like one of the
existing tests is failing (most likely a dummy icon needs to be removed):

ProcessManagementTest.ProcessOverflow:
Did not complete.

RETRY OUTPUT:

HTTP server started on 127.0.0.1:36100...
sending server_data: {"host": "127.0.0.1", "port": 36100} (36 bytes)
[24534:24534:0731/131451:1638893132:ERROR:extension_error_reporter.cc(56)]
Extension error: Could not load extension from
'/mnt/data/b/build/slave/linux/build/src/chrome/test/data/extensions/api_test/browser_action/close_background'.
Could not load icon 'icon.png' for browser action.
Killed (timed out).

Mihai


On Thu, Aug 2, 2012 at 10:39 AM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/mitchellwrosen@**
>
chromium.org/10690016/31001<https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/31001>
>
>
>
https://chromiumcodereview.**appspot.com/10690016/<https://chromiumcodereview...
>

[mitchellwrosen, 2012-08-02 17:42:49]

On 2012/08/02 17:41:10, Mihai Parparita wrote:
> Actually, this isn't a matter of CQ flakiness. It looks like one of the
> existing tests is failing (most likely a dummy icon needs to be removed):
> 
> ProcessManagementTest.ProcessOverflow:
> Did not complete.
> 
> RETRY OUTPUT:
> 
> HTTP server started on 127.0.0.1:36100...
> sending server_data: {"host": "127.0.0.1", "port": 36100} (36 bytes)
> [24534:24534:0731/131451:1638893132:ERROR:extension_error_reporter.cc(56)]
> Extension error: Could not load extension from
>
'/mnt/data/b/build/slave/linux/build/src/chrome/test/data/extensions/api_test/browser_action/close_background'.
> Could not load icon 'icon.png' for browser action.
> Killed (timed out).
> 
> Mihai
> 
> 
> On Thu, Aug 2, 2012 at 10:39 AM, <mailto:commit-bot@chromium.org> wrote:
> 
> > CQ is trying da patch. Follow status at
> > https://chromium-status.**appspot.com/cq/mitchellwrosen%40**
> >
>
chromium.org/10690016/31001<https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/31001>
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/10690016/%3Chttps://chromiumcoderevi...>
> >

I saw that. I kept passing the trybots and failing the CQ (inexplicable, I
know). I'm passing on Linux and we have a Mac in the office so I'll check it out
hopefully later today.

[Mihai Parparita -not on Chrome, 2012-08-02 18:00:27]

On Thu, Aug 2, 2012 at 10:42 AM, <mitchellwrosen@chromium.org> wrote:

> I saw that. I kept passing the trybots and failing the CQ (inexplicable, I
> know). I'm passing on Linux and we have a Mac in the office so I'll check
> it out
> hopefully later today.
>

Huh, the file appears to be empty (on Linux):

$ ls -al
chrome/test/data/extensions/api_test/browser_action/close_background/icon.png
-rw-r--r-- 1 mihaip eng 0 2011-09-07 10:19
chrome/test/data/extensions/api_test/browser_action/close_background/icon.png

Mihai

[mitchellwrosen, 2012-08-02 18:02:17]

On 2012/08/02 18:00:27, Mihai Parparita wrote:
> On Thu, Aug 2, 2012 at 10:42 AM, <mailto:mitchellwrosen@chromium.org> wrote:
> 
> > I saw that. I kept passing the trybots and failing the CQ (inexplicable, I
> > know). I'm passing on Linux and we have a Mac in the office so I'll check
> > it out
> > hopefully later today.
> >
> 
> Huh, the file appears to be empty (on Linux):
> 
> $ ls -al
> chrome/test/data/extensions/api_test/browser_action/close_background/icon.png
> -rw-r--r-- 1 mihaip eng 0 2011-09-07 10:19
> chrome/test/data/extensions/api_test/browser_action/close_background/icon.png
> 
> Mihai

Ya, right now it is. It has to be non-empty for this patch to land, though,
which is why I modified it =)

[Mihai Parparita -not on Chrome, 2012-08-02 18:07:35]

On Thu, Aug 2, 2012 at 11:02 AM, <mitchellwrosen@chromium.org> wrote:

> Ya, right now it is. It has to be non-empty for this patch to land, though,
> which is why I modified it =)
>

I thought binary files can't be landed through the CQ, hence stuff like
http://crrev.com/145743.

Mihai

[mitchellwrosen, 2012-08-02 18:09:00]

On 2012/08/02 18:07:35, Mihai Parparita wrote:
> On Thu, Aug 2, 2012 at 11:02 AM, <mailto:mitchellwrosen@chromium.org> wrote:
> 
> > Ya, right now it is. It has to be non-empty for this patch to land, though,
> > which is why I modified it =)
> >
> 
> I thought binary files can't be landed through the CQ, hence stuff like
> http://crrev.com/145743.
> 
> Mihai

Ugh, do you think that's the problem? It was my understanding that only empty
files and binary files can't be landed through the CQ, but ascii files can
regardless of the file extension. (glares at coworker) (yes you... CLARK)

[Mihai Parparita -not on Chrome, 2012-08-02 18:21:07]

On Thu, Aug 2, 2012 at 11:09 AM, <mitchellwrosen@chromium.org> wrote:

> Ugh, do you think that's the problem? It was my understanding that only
> empty
> files and binary files can't be landed through the CQ, but ascii files can
> regardless of the file extension. (glares at coworker) (yes you... CLARK)
>

Based on
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/44689/...,
it looks like the file is getting interpreted as binary anyway (most likely
based on the extension):

Index:
chrome/test/data/extensions/api_test/browser_action/close_background/icon.png
===================================================================
Cannot display: file marked as a binary type.
svn:mime-type = image/png

Since I'm pretty sure the test doesn't depend on the browser action having
an icon, it might be easier to just delete the file and update the manifest
to not refer to it.

Mihai

[commit-bot: I haz the power, 2012-08-02 20:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10690016/4...

[commit-bot: I haz the power, 2012-08-02 22:15:19]

Change committed as 149721