Use a FrameAssociatedLoader when downloading image resources.

Use a FrameAssociatedLoader when downloading image resources.

It makes MultiResolutionImageResourceFetcher uses FRAME_ASSOCIATED_LOADER
instead of PLATFORM_LOADER and set the loader options to allow
cross origin fetching and passes credentials.

Amongst other things, using the Blink loader will show the load in the
developer tools. For example, it does expose a bug while trying to fetch
the favicon in https://www.gmail.com/intl/en/mail/help/about.html

The load allows cross origin because many websites uses CDN for favicon
and it allows credentials because the last attempt to not pass cookies
to the favicon request failed, see bug 114082. This CL is not
changing any credential/cross-origin behaviour but makes it
explicit.

Furthermore, this CL changes some special favicon requests handling in
resource_dispatcher_host_impl.cc and instead explicitly bypass service
workers when setting up the request.

BUG=110449
Committed: https://crrev.com/f484b43ae8679aadeac8cc593b5cb7d5cd9a55c8
Cr-Commit-Position: refs/heads/master@{#311519}

[mlamouri (slow - plz ping), 2015-01-06 17:17:24]

mlamouri@chromium.org changed reviewers:
+ avi@chromium.org

[mlamouri (slow - plz ping), 2015-01-06 17:17:24]

avi@, can you take a look?

Sorry for asking you to review something deep into content/ but fetchers/
doesn't have an OWNERS file :( Except me, mkwst@, jochen@ are the last persons
to have done significant changes in there but they are both OOO for the moment.

[Avi (use Gerrit), 2015-01-06 17:20:01]

I don't know enough about the loader code to comment intelligently. Try japhet@
or nasko@.

[mlamouri (slow - plz ping), 2015-01-06 17:26:04]

mlamouri@chromium.org changed reviewers:
+ nasko@chromium.org
- avi@chromium.org

[mlamouri (slow - plz ping), 2015-01-06 17:26:05]

-avi@, +nasko@

[mlamouri (slow - plz ping), 2015-01-06 17:32:50]

https://codereview.chromium.org/840553003/diff/1/content/renderer/fetchers/mu...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/840553003/diff/1/content/renderer/fetchers/mu...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:46:
ResourceFetcher::FRAME_ASSOCIATED_LOADER,
If exposing all image loading in the devtools can be a problem, we could use the
FRAME_ASSOCIATED_LOADER when the request type is FAVICON.

[nasko, 2015-01-06 18:08:29]

nasko@chromium.org changed reviewers:
+ japhet@chromium.org

[nasko, 2015-01-06 18:08:29]

Nate is much more versed in loading than I am.

[Nate Chapin, 2015-01-06 18:18:42]

This seems reasonable in general, though I'm not sure about the cookie behavior
change.

Also, note that I'm not an OWNER, so nasko will still need to approve this.

https://codereview.chromium.org/840553003/diff/1/content/renderer/fetchers/mu...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/840553003/diff/1/content/renderer/fetchers/mu...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:37:
options.allowCredentials = true;
I don't feel qualified to approve a change to favicon credential behavior. Would
you mind doing this as part of a separate patch, with a broader set of
reviewers?

[mlamouri (slow - plz ping), 2015-01-07 10:08:06]

https://codereview.chromium.org/840553003/diff/1/content/renderer/fetchers/mu...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/840553003/diff/1/content/renderer/fetchers/mu...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:37:
options.allowCredentials = true;
On 2015/01/06 18:18:42, Nate Chapin wrote:
> I don't feel qualified to approve a change to favicon credential behavior.
Would
> you mind doing this as part of a separate patch, with a broader set of
> reviewers?

This is actually not changing any behaviour. The previous loader
(PLATFORM_LOADER) would send credentials and allow cross origin requests. This
is why I'm passing those options to the FrameAssociatedLoader. If you look at
bug 114082 you will see attempts to make favicon requests not using credentials.

I understand that my CL description is a bit confusing and might suggest that I
change the behaviour. I will update that.

[Nate Chapin, 2015-01-07 18:56:45]

Ok, thanks for clarifying. LGTM

[nasko, 2015-01-07 19:02:55]

Rubberstamp LGTM

[mlamouri (slow - plz ping), 2015-01-07 20:18:37]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-07 20:19:41]

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

[commit-bot: I haz the power, 2015-01-07 21:39:01]

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

[commit-bot: I haz the power, 2015-01-07 21:39:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2015-01-08 14:32:45]

mlamouri@chromium.org changed reviewers:
+ tyoshino@chromium.org

[mlamouri (slow - plz ping), 2015-01-08 14:32:46]

tyoshino@, it seems that this CL raises one ASSERT that you added in
https://chromium.googlesource.com/chromium/blink/+/430b65c4e33c08d34f67e212eb...

Do you have an idea why? I understand why we end up having
m_fallbackRequestForServiceWorker set but I don't know how it usually end up
reset to nullptr.

[tyoshino (SeeGerritForStatus), 2015-01-09 04:41:26]

On 2015/01/08 14:32:46, Mounir Lamouri wrote:
> tyoshino@, it seems that this CL raises one ASSERT that you added in
>
https://chromium.googlesource.com/chromium/blink/+/430b65c4e33c08d34f67e212eb...
> 
> Do you have an idea why? I understand why we end up having
> m_fallbackRequestForServiceWorker set but I don't know how it usually end up
> reset to nullptr.

Do you know which ASSERT is hit?

[tyoshino (SeeGerritForStatus), 2015-01-09 07:38:43]

On 2015/01/09 04:41:26, tyoshino wrote:
> Do you know which ASSERT is hit?

OK. I see them on the try bots. Looking.

[tyoshino (SeeGerritForStatus), 2015-01-09 08:41:50]

On 2015/01/09 07:38:43, tyoshino wrote:
> On 2015/01/09 04:41:26, tyoshino wrote:
> > Do you know which ASSERT is hit?
> 
> OK. I see them on the try bots. Looking.

I'm sorry. The ASSERT you hit is incorrect. I'll remove it.

[tyoshino (SeeGerritForStatus), 2015-01-09 09:59:54]

On 2015/01/09 08:41:50, tyoshino wrote:
> On 2015/01/09 07:38:43, tyoshino wrote:
> > On 2015/01/09 04:41:26, tyoshino wrote:
> > > Do you know which ASSERT is hit?
> > 
> > OK. I see them on the try bots. Looking.
> 
> I'm sorry. The ASSERT you hit is incorrect. I'll remove it.

https://codereview.chromium.org/819513004/

[horo, 2015-01-13 03:53:46]

horo@chromium.org changed reviewers:
+ horo@chromium.org

[horo, 2015-01-13 03:53:46]

I think we should not remove this ASSERT(!m_fallbackRequestForServiceWorker) in
DocumentThreadableLoader.

To avoid the favicon cache tainting, we made a change in
ResourceDispatcherHostImpl to skip ServiceWorker for the favicon request.
https://codereview.chromium.org/654713002
But I think ResourceDispatcherHostImpl in the browser process is not the correct
position to check this.
We should set skipServiceWorker flag while requesting the favicon in the
renderer process.

So please do the followings:
- Add skipServiceWorker flag to WebURLLoaderOptions.
- Set this flag in the constructor of MultiResolutionImageResourceFetcher.
- Pass m_options's skipServiceWorker flag to webcoreRequest's skipServiceWorker
flag in AssociatedURLLoader::loadAsynchronously.

[mlamouri (slow - plz ping), 2015-01-13 13:49:40]

On 2015/01/13 at 03:53:46, horo wrote:
> I think we should not remove this ASSERT(!m_fallbackRequestForServiceWorker)
in DocumentThreadableLoader.
> 
> To avoid the favicon cache tainting, we made a change in
ResourceDispatcherHostImpl to skip ServiceWorker for the favicon request.
> https://codereview.chromium.org/654713002
> But I think ResourceDispatcherHostImpl in the browser process is not the
correct position to check this.
> We should set skipServiceWorker flag while requesting the favicon in the
renderer process.
> 
> So please do the followings:
> - Add skipServiceWorker flag to WebURLLoaderOptions.
> - Set this flag in the constructor of MultiResolutionImageResourceFetcher.
> - Pass m_options's skipServiceWorker flag to webcoreRequest's
skipServiceWorker flag in AssociatedURLLoader::loadAsynchronously.

Thanks for the comments Horo. I understand the concerns voiced in the CL you
pointed. However, I wonder why we shouldn't allow a SW to handle a favicon
request if it is same origin. It sounds like a fairly valid behaviour to me.

Also, the changes would apply to all images fetched from the browser process. Do
we want all of them to bypass the SWs?

[tyoshino (SeeGerritForStatus), 2015-01-13 14:47:58]

On 2015/01/13 13:49:40, Mounir Lamouri wrote:
> On 2015/01/13 at 03:53:46, horo wrote:
> > I think we should not remove this ASSERT(!m_fallbackRequestForServiceWorker)
> in DocumentThreadableLoader.
> > 
> > To avoid the favicon cache tainting, we made a change in
> ResourceDispatcherHostImpl to skip ServiceWorker for the favicon request.
> > https://codereview.chromium.org/654713002
> > But I think ResourceDispatcherHostImpl in the browser process is not the
> correct position to check this.
> > We should set skipServiceWorker flag while requesting the favicon in the
> renderer process.
> > 
> > So please do the followings:
> > - Add skipServiceWorker flag to WebURLLoaderOptions.
> > - Set this flag in the constructor of MultiResolutionImageResourceFetcher.
> > - Pass m_options's skipServiceWorker flag to webcoreRequest's
> skipServiceWorker flag in AssociatedURLLoader::loadAsynchronously.
> 
> Thanks for the comments Horo. I understand the concerns voiced in the CL you
> pointed. However, I wonder why we shouldn't allow a SW to handle a favicon
> request if it is same origin. It sounds like a fairly valid behaviour to me.
> 
> Also, the changes would apply to all images fetched from the browser process.
Do
> we want all of them to bypass the SWs?

It's ad-hoc but how about

1. determine whether the ResourceDispatcherHostImpl considers the request as
favicon or not in the same way (it's done by WebURLLoaderImpl setting the
request_type using WebURLRequestToResourceType() and comparing it with favicon
request type in ResourceDispatcherHostImpl) using WebURLRequestToResourceType()

2. set the skipServiceWorker flag based on the result of the 1st step

[mlamouri (slow - plz ping), 2015-01-13 14:52:59]

On 2015/01/13 at 14:47:58, tyoshino wrote:
> On 2015/01/13 13:49:40, Mounir Lamouri wrote:
> > On 2015/01/13 at 03:53:46, horo wrote:
> > > I think we should not remove this
ASSERT(!m_fallbackRequestForServiceWorker)
> > in DocumentThreadableLoader.
> > > 
> > > To avoid the favicon cache tainting, we made a change in
> > ResourceDispatcherHostImpl to skip ServiceWorker for the favicon request.
> > > https://codereview.chromium.org/654713002
> > > But I think ResourceDispatcherHostImpl in the browser process is not the
> > correct position to check this.
> > > We should set skipServiceWorker flag while requesting the favicon in the
> > renderer process.
> > > 
> > > So please do the followings:
> > > - Add skipServiceWorker flag to WebURLLoaderOptions.
> > > - Set this flag in the constructor of MultiResolutionImageResourceFetcher.
> > > - Pass m_options's skipServiceWorker flag to webcoreRequest's
> > skipServiceWorker flag in AssociatedURLLoader::loadAsynchronously.
> > 
> > Thanks for the comments Horo. I understand the concerns voiced in the CL you
> > pointed. However, I wonder why we shouldn't allow a SW to handle a favicon
> > request if it is same origin. It sounds like a fairly valid behaviour to me.
> > 
> > Also, the changes would apply to all images fetched from the browser
process. Do
> > we want all of them to bypass the SWs?
> 
> It's ad-hoc but how about
> 
> 1. determine whether the ResourceDispatcherHostImpl considers the request as
favicon or not in the same way (it's done by WebURLLoaderImpl setting the
request_type using WebURLRequestToResourceType() and comparing it with favicon
request type in ResourceDispatcherHostImpl) using WebURLRequestToResourceType()
> 
> 2. set the skipServiceWorker flag based on the result of the 1st step

Sure. I could do that but would the other requests trigger that ASSERT? It's
still not clear to me why that ASSERT is triggered actually...

[tyoshino (SeeGerritForStatus), 2015-01-13 17:09:06]

On 2015/01/13 14:52:59, Mounir Lamouri wrote:
> On 2015/01/13 at 14:47:58, tyoshino wrote:
> > On 2015/01/13 13:49:40, Mounir Lamouri wrote:
> > > On 2015/01/13 at 03:53:46, horo wrote:
> > > > I think we should not remove this
> ASSERT(!m_fallbackRequestForServiceWorker)
> > > in DocumentThreadableLoader.
> > > > 
> > > > To avoid the favicon cache tainting, we made a change in
> > > ResourceDispatcherHostImpl to skip ServiceWorker for the favicon request.
> > > > https://codereview.chromium.org/654713002
> > > > But I think ResourceDispatcherHostImpl in the browser process is not the
> > > correct position to check this.
> > > > We should set skipServiceWorker flag while requesting the favicon in the
> > > renderer process.
> > > > 
> > > > So please do the followings:
> > > > - Add skipServiceWorker flag to WebURLLoaderOptions.
> > > > - Set this flag in the constructor of
MultiResolutionImageResourceFetcher.
> > > > - Pass m_options's skipServiceWorker flag to webcoreRequest's
> > > skipServiceWorker flag in AssociatedURLLoader::loadAsynchronously.
> > > 
> > > Thanks for the comments Horo. I understand the concerns voiced in the CL
you
> > > pointed. However, I wonder why we shouldn't allow a SW to handle a favicon
> > > request if it is same origin. It sounds like a fairly valid behaviour to
me.
> > > 
> > > Also, the changes would apply to all images fetched from the browser
> process. Do
> > > we want all of them to bypass the SWs?
> > 
> > It's ad-hoc but how about
> > 
> > 1. determine whether the ResourceDispatcherHostImpl considers the request as
> favicon or not in the same way (it's done by WebURLLoaderImpl setting the
> request_type using WebURLRequestToResourceType() and comparing it with favicon
> request type in ResourceDispatcherHostImpl) using
WebURLRequestToResourceType()
> > 
> > 2. set the skipServiceWorker flag based on the result of the 1st step
> 
> Sure. I could do that but would the other requests trigger that ASSERT? It's
> still not clear to me why that ASSERT is triggered actually...

Please take a look at the 1st bullet point in
https://codereview.chromium.org/819513004/#msg5

ResourceDispatcherHostImpl skips SW for favicon requests while
DocumentThreadableLoader expects that to go through SW.

DocumentThreadableLoader thinks that the request it issues goes through SW if
skipServiceWorker is not set && the request is async (i.e. if
!(skipServiceWorker is set || the request is sync)). The contraposition of this
is "if the request has gone through SW (i.e. wasFetchedViaServiceWorker is set),
skipServiceWorker was set || the request was sync". So,
m_fallbackRequestForServiceWorker shouldn't have been set in the
DocumentThreadableLoader ctor. The ASSERT checks this.

[mlamouri (slow - plz ping), 2015-01-13 18:06:50]

I've updated the CL to have the special favicon SW happening in the fetcher
instead of resource_dispatcher_host_impl.cc

I have also opened a bug and added it in a comment. horo@ and tyoshino@, PTAL

[horo, 2015-01-14 03:39:51]

lgtm

Thank you!

[tyoshino (SeeGerritForStatus), 2015-01-14 04:09:41]

lgtm

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
File content/renderer/fetchers/resource_fetcher_impl.cc (right):

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.cc:13: #include
"third_party/WebKit/public/platform/WebURLError.h"
[optional] remove this unused include using this opportunity and include
WebString.h instead

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.cc:25: using blink::WebURLError;
[optional] remove this unused using using this opportunity

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.cc:27: using
blink::WebURLLoaderOptions;
just one occurrence. maybe it's just fine to add blink:: to L88?

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
File content/renderer/fetchers/resource_fetcher_impl.h (right):

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.h:27: struct
WebURLLoaderOptions;
Remove?

[mlamouri (slow - plz ping), 2015-01-14 11:58:11]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-14 11:58:20]

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

[commit-bot: I haz the power, 2015-01-14 13:13:29]

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

[commit-bot: I haz the power, 2015-01-14 13:13:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2015-01-14 14:31:17]

I've also added a couple of empty favicon.ico because some tests read the
console and expect no errors. With this CL, missing favicons are advertised.

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
File content/renderer/fetchers/resource_fetcher_impl.cc (right):

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.cc:13: #include
"third_party/WebKit/public/platform/WebURLError.h"
On 2015/01/14 at 04:09:41, tyoshino wrote:
> [optional] remove this unused include using this opportunity and include
WebString.h instead

Done.

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.cc:25: using blink::WebURLError;
On 2015/01/14 at 04:09:41, tyoshino wrote:
> [optional] remove this unused using using this opportunity

Done.

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.cc:27: using
blink::WebURLLoaderOptions;
On 2015/01/14 at 04:09:41, tyoshino wrote:
> just one occurrence. maybe it's just fine to add blink:: to L88?

I removed all the |using| while I was at it. Most of them where used once. One
was not used. One used twice.

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
File content/renderer/fetchers/resource_fetcher_impl.h (right):

https://codereview.chromium.org/840553003/diff/20001/content/renderer/fetcher...
content/renderer/fetchers/resource_fetcher_impl.h:27: struct
WebURLLoaderOptions;
On 2015/01/14 at 04:09:41, tyoshino wrote:
> Remove?

Indeed. WebURLError too ;)

[mlamouri (slow - plz ping), 2015-01-14 14:44:31]

mlamouri@chromium.org changed reviewers:
+ marja@chromium.org

[mlamouri (slow - plz ping), 2015-01-14 14:44:32]

Marja, could you review the addition of favicon.ico in tools/telemetry/? I've
added it to prevent one of the tests to fail because of a new console message
(about a 404 to fetch favicon.ico). I could fix the test but adding the file
sounds like a pretty simple fix and will benefit everyone.

[Sami, 2015-01-14 15:04:46]

On 2015/01/14 14:44:32, Mounir Lamouri wrote:
> Marja, could you review the addition of favicon.ico in tools/telemetry/? I've
> added it to prevent one of the tests to fail because of a new console message
> (about a 404 to fetch favicon.ico). I could fix the test but adding the file
> sounds like a pretty simple fix and will benefit everyone.

tools/telemetry/unittest_data/favicon.ico lgtm since it looks like we're already
doing similar things for other tests too, and the alternative of messing with
suppressions or the expectations of this particular test seems messier. The icon
itself could use a little more artistic vision.

[mlamouri (slow - plz ping), 2015-01-14 15:14:23]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-14 15:15:19]

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

[commit-bot: I haz the power, 2015-01-14 16:27:09]

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

[commit-bot: I haz the power, 2015-01-14 16:27:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2015-01-14 17:44:33]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-14 17:46:08]

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

[commit-bot: I haz the power, 2015-01-14 19:04:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-14 19:04:58]

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