Bypass Service Worker for favicon loads

Bypass Service Worker for favicon loads

Since Service Workers can intercept off-origin requests for controlled
pages, a controlled page at http://a.example.com/ that requests a
favicon from http://b.example.com would give the SW the opportunity
to taint the browser's favicon cache for http://b.example.com,

While we determine how (or if) we should restructure the favicon cache,
just bypass the SW for favicon loads.

R=horo@chromium.org,mkwest@chromium.org,jochen@chromium.org
BUG=422250
Committed: https://crrev.com/f82b4f6830ad16a3dd0c1bfa04f095426aa6ea25
Cr-Commit-Position: refs/heads/master@{#299744}

[horo, 2014-10-14 02:13:51]

lgtm

[jsbell, 2014-10-14 21:02:28]

jsbell@chromium.org changed reviewers:
+ mmenke@chromium.org
- mkwest@chromium.org

[jsbell, 2014-10-14 21:07:56]

jochen@ or mmenke@ - please take a look? (and cq if ok?)

I haven't come up with a reasonable way to author a test this. Neither layout
tests (expected) nor content_browsertests (unexpected!) cause favicons to
actually be fetched. :(

[falken, 2014-10-15 02:14:56]

On 2014/10/14 21:07:56, jsbell wrote:
> jochen@ or mmenke@ - please take a look? (and cq if ok?)
> 
> I haven't come up with a reasonable way to author a test this. Neither layout
> tests (expected) nor content_browsertests (unexpected!) cause favicons to
> actually be fetched. :(

I'm curious, does the patch work for both default "/favicon.ico" favicons and
<link rel="icon"> favicons?

[mmenke, 2014-10-15 13:54:14]

On 2014/10/15 02:14:56, falken wrote:
> On 2014/10/14 21:07:56, jsbell wrote:
> > jochen@ or mmenke@ - please take a look? (and cq if ok?)
> > 
> > I haven't come up with a reasonable way to author a test this. Neither
layout
> > tests (expected) nor content_browsertests (unexpected!) cause favicons to
> > actually be fetched. :(
> 
> I'm curious, does the patch work for both default "/favicon.ico" favicons and
> <link rel="icon"> favicons?

LGTM, not CQing due to falken's question.

[jsbell, 2014-10-15 19:23:06]

On 2014/10/15 02:14:56, falken wrote:
> I'm curious, does the patch work for both default "/favicon.ico" favicons and
> <link rel="icon"> favicons?

Great question! I've now confirmed (via local testing) that:

(1) both the default and an explicit (link rel="icon") favicon loads go through
the same path
(2) without the patch, the SW is able to intercept/overwrite both (kenji
everywhere)
(3) with the patch, the SW is bypassed (no kenji)

[jsbell, 2014-10-15 19:23:13]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:24:18]

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

[commit-bot: I haz the power, 2014-10-15 19:39:38]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-15 19:40:31]

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

[mmenke, 2014-10-15 20:23:04]

On 2014/10/15 19:40:31, I haz the power (commit-bot) wrote:
> Patchset 1 (id:??) landed as
> https://crrev.com/f82b4f6830ad16a3dd0c1bfa04f095426aa6ea25
> Cr-Commit-Position: refs/heads/master@{#299744}

Oh, also, we should probably have a test for this - this is the kind of subtle
thing that can easily regress during a refactoring, and go for decades with no
one noticing it.

[jsbell, 2014-10-15 20:33:21]

On 2014/10/15 20:23:04, mmenke wrote:
> On 2014/10/15 19:40:31, I haz the power (commit-bot) wrote:
> > Patchset 1 (id:??) landed as
> > https://crrev.com/f82b4f6830ad16a3dd0c1bfa04f095426aa6ea25
> > Cr-Commit-Position: refs/heads/master@{#299744}
> 
> Oh, also, we should probably have a test for this - this is the kind of subtle
> thing that can easily regress during a refactoring, and go for decades with no
> one noticing it.

Agreed - see #3. I implemented a content_browsertest but the favicons weren't
fetched at all in that case :P. I'll see if a full blown browser_test exercises
things appropriately.